Port cl/365835301: Fix flaky tests in firestore_test.cc. #362
Conversation
This file contains hidden or 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
TestSnapshotsInSyncListenerFiresAfterListenersInSync and TestListenCanBeCalledMultipleTimes were flaky for three reasons. Reason 1: The Semaphore was incorrectly initialized to zero, causing the subsequent call to Wait() to return immediately (instead of blocking, waiting for the call to Post()). The Semaphore should have been initialized to 1 to achieve this desired behavior. Reason 2: The `std::vector<std::string> events` object was being accessed concurrently from multiple threads without any protection (e.g. from a Mutex), causing a race condition. I don't think that this race condition contributed to crashes; however, it is technically undefined behavior and is prudent to fix. Reason 3: Both the Semaphore and the vector could potentially be used by the listener even after these variables went out of scope, causing the call to Semaphore::Post() to fail and crash the application. The fix was to create function-local `TestData` classes that encapsulate the vector and protect it from concurrent access with a `Mutex`. There was a comment in the code, which I have since deleted, stating that "the implementation of Semaphore::Post() on Apple platforms has a data race". I believe that this apparent data race was actually due to "reason #1" listed above, where the Semaphore was incorrectly being initialized to zero.
dconeybe
added
the
tests-requested: quick
github-actions
bot
added
tests: in-progress
✅ Integration test succeeded!Requested by @dconeybe on commit 1eaa038 |
firebase-workflow-trigger
bot
removed
the
tests: in-progress
github-actions
bot
added
the
tests: succeeded
dconeybe
requested review from
jonsimantov and
schmidt-sebastian
and removed request for
jonsimantov
schmidt-sebastian
approved these changes
Apr 12, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
TestSnapshotsInSyncListenerFiresAfterListenersInSync and TestListenCanBeCalledMultipleTimes were flaky for three reasons.
Reason 1: The Semaphore was incorrectly initialized to zero, causing the subsequent call to Wait() to return immediately (instead of blocking, waiting for the call to Post()). The Semaphore should have been initialized to 1 to achieve this desired behavior.
Reason 2: The
std::vector<std::string> eventsobject was being accessed concurrently from multiple threads without any protection (e.g. from a Mutex), causing a race condition. I don't think that this race condition contributed to crashes; however, it is technically undefined behavior and is prudent to fix.Reason 3: Both the Semaphore and the vector could potentially be used by the listener even after these variables went out of scope, causing the call to Semaphore::Post() to fail and crash the application.
The fix was to create function-local
TestDataclasses that encapsulate the vector and protect it from concurrent access with aMutex.There was a comment in the code, which I have since deleted, stating that "the implementation of Semaphore::Post() on Apple platforms has a data race". I believe that this apparent data race was actually due to "reason 1" listed above, where the Semaphore was incorrectly being initialized to zero.