Skip to content

Commit b80fcc5

Browse files
committed
Make FutureManager not delete futures if they are still referenced externally.
A recent change to reference counted future impl made IsSafeToDelete only check if the underlying logic determined if it was safe, which was needed when deleting Auth futures directly. This made FutureManager a bit hasty in deleting things, which instead should be checking if those Futures are still being referenced by someone else. PiperOrigin-RevId: 264503406
1 parent fe2463f commit b80fcc5

File tree

3 files changed

+28
-1
lines changed

3 files changed

+28
-1
lines changed

app/src/future_manager.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,8 @@ void FutureManager::CleanupOrphanedFutureApis(bool force_delete_all) {
120120

121121
bool FutureManager::IsSafeToDeleteFutureApi(ReferenceCountedFutureImpl* api) {
122122
MutexLock lock(future_api_mutex_);
123-
return api ? api->IsSafeToDelete() : false;
123+
return api ? api->IsSafeToDelete() && !api->IsReferencedExternally()
124+
: false;
124125
}
125126

126127
// NOLINTNEXTLINE - allow namespace overridden

app/src/reference_counted_future_impl.cc

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include "app/src/assert.h"
2323
#include "app/src/log.h"
24+
#include "app/src/mutex.h"
2425

2526
// Set this to 1 to enable verbose logging in this module.
2627
#if !defined(FIREBASE_FUTURE_TRACE_ENABLE)
@@ -510,6 +511,27 @@ bool ReferenceCountedFutureImpl::IsSafeToDelete() const {
510511
return true;
511512
}
512513

514+
bool ReferenceCountedFutureImpl::IsReferencedExternally() const {
515+
MutexLock lock(mutex_);
516+
517+
int total_references = 0;
518+
int internal_references = 0;
519+
for (auto i = backings_.begin(); i != backings_.end(); ++i) {
520+
// Count the total number of references to all valid Futures.
521+
total_references += i->second->reference_count;
522+
}
523+
for (int i = 0; i < last_results_.size(); i++) {
524+
if (last_results_[i].status() != kFutureStatusInvalid) {
525+
// If the status is not invalid, this entry is using up a reference.
526+
// Count up the internal references.
527+
internal_references++;
528+
}
529+
}
530+
// If there are more references than the internal ones, someone is holding
531+
// onto a Future.
532+
return total_references > internal_references;
533+
}
534+
513535
void ReferenceCountedFutureImpl::SetContextData(
514536
FutureHandle handle, void* context_data,
515537
void (*delete_context_data_fn)(void* data_to_delete)) {

app/src/reference_counted_future_impl.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,10 @@ class ReferenceCountedFutureImpl : public detail::FutureApiInterface {
369369
/// no futures are Pending.
370370
bool IsSafeToDelete() const;
371371

372+
/// Check if the Future is being referenced by something other than
373+
/// last_results_.
374+
bool IsReferencedExternally() const;
375+
372376
/// Sets temporary context data associated with a FutureHandle that will be
373377
/// deallocated alongside the FutureBackingData. This will occur when there
374378
/// are no more Futures referencing it.

0 commit comments

Comments
 (0)