-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Firestore: Fix memory leak in Query.whereField() #14300
Open
dconeybe
wants to merge
104
commits into
main
Choose a base branch
from
dconeybe/ThreadSafeMemoizerMemoryLeakFix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ss it along so that its lifetime gets extended to the full-expression that uses FormatArg, as would have happened if `AlphaNum` was used as intended (i.e. as an argument to StrCat and StrAppend).
…extension of the StringifySink object
…r than just ASSERT_NE
…feMemoizerMemoryLeakFix This PR is now based on #14306
dconeybe
changed the base branch from
main
to
dconeybe/StringFormatStringifySinkUseAfterFreeFix
January 6, 2025 21:30
AI(dconeybe): Verify that the implementation that uses std::shared_ptr internally still fixes the memory leak. Update: It does NOT fix the memory leak. I'm reverting back to the std::atomic implementation. |
…they're doing what I think they're doing.
…t.py _ignores_ (rather than fails) when this lint warning is suppressed.
This reverts commit 11b5324.
…ven function _at most once_, rather than _any number of times_.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an apparent memory leak that is detected by the "Xcode Leaks" tool. As of today, this leak has been reported at least twice: #13978 and #12613. I don't completely understand why this is flagged as a memory leak because, to me, I cannot find any "leaks"; however, I fixed the detected leak anyways. The investigation also uncovered an unrelated use-after-free bug that was fixed as well: #14306.
The leaks originated from
std::shared_ptr<ThreadSafeMemoizer>
usages, which manifested many public APIs, includingQuery.whereField()
,queryWhereFieldPath:arrayContainsAny
, andFIRQuery.addFields(using:)
.Before this PR, all instances of
ThreadSafeMemoizer
were held in anstd::shared_ptr
so that the memoized value could be shared among copies of the object of which thestd::shared_ptr
is a member, and also to work around the limitation thatThreadSafeMemoizer
itself was neither copyable nor moveable.This PR completely rewrites
ThreadSafeMemoizer<T>
to make it both moveable and copyable, thus removing the need to store instances of it in astd::shared_ptr
. The re-write has a single member variable of typestd::shared_ptr<T>
rather than a member variable ofT
directly. In order to make accesses to thestd::shared_ptr<T>
thread-safe, all access are done using thestd::atomic_XXX()
free functions, such asstd::atomic_store()
,std::atomic_load()
, andstd::atomic_compare_exchange_weak()
. Thesestd::atomic_XXX()
free functions are deprecated in C++20, at which time the implementation should be changed to usestd::atomic<std::shared_ptr<T>>
, which is less error-prone.The API of the re-written
ThreadSafeMemoizer<T>
also changes theconst T& memoize(std::function<T()>)
member function toconst T& value(const std::function<std::shared_ptr<T>()>&)
; that is, it was renamed from "memoize" to "value" and the given function is expected to returnstd::shared_ptr<T>
instead of justT
. Doing this avoids unnecessarily copying or moving theT
object into a newly-createdstd::shared_ptr<T>
byThreadSafeMemoizer<T>
, thus supportingT
objects that are not copyable and/or moveable.The test suite for
ThreadSafeMemoizer<T>
was also vastly improved to test things like thread safety, copy and move operations, and invocation semantics. Various utility functions and classes were added intothread_safe_memoizer_testing.h
(and implemented in the corresponding .cc file), with tests added for those utility classes and functions too.Fixes: #13978