From 1eaa0384992ca94801815d1f61fbfe3e9dcda65f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 9 Apr 2021 16:06:17 -0400 Subject: [PATCH] Port cl/365835301: Fix flaky tests in firestore_test.cc. 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 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. --- .../src/firestore_test.cc | 191 ++++++++++-------- 1 file changed, 106 insertions(+), 85 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 6cfdd91bf3..e1a7a5d778 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1,5 +1,7 @@ #include "firebase/firestore.h" +#include + #if !defined(__ANDROID__) #include // NOLINT(build/c++11) #endif @@ -23,6 +25,7 @@ #include "util/future_test_util.h" #include "app/memory/unique_ptr.h" +#include "app/src/mutex.h" #include "auth/src/include/firebase/auth.h" #include "firestore/src/common/macros.h" #include "gmock/gmock.h" @@ -619,91 +622,105 @@ TEST_F(FirestoreIntegrationTest, TEST_F(FirestoreIntegrationTest, TestSnapshotsInSyncListenerFiresAfterListenersInSync) { + class TestData { + public: + void AddEvent(const std::string& event) { + MutexLock lock(mutex_); + events_.push_back(event); + } + + int GetEventCount() const { + MutexLock lock(mutex_); + return events_.size(); + } + + void ClearEvents() { + MutexLock lock(mutex_); + events_.clear(); + } + + void WaitForEventCount(const std::string& event, int expected_count) { + while (true) { + if (GetEventCount(event) >= expected_count) { + break; + } + } + } + + int GetEventCount(const std::string& event) const { + MutexLock lock(mutex_); + return std::count_if(events_.begin(), events_.end(), + [&event](const std::string& current_event) { + return current_event == event; + }); + } + + std::vector GetEvents() const { + MutexLock lock(mutex_); + return events_; + } + + private: + mutable Mutex mutex_; + std::vector events_; + }; + + TestData test_data; + DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{{"foo", FieldValue::Double(1.0)}})); - std::vector events; class SnapshotTestEventListener : public TestEventListener { public: - SnapshotTestEventListener(std::string name, - std::vector* events) - : TestEventListener(std::move(name)), events_(events) {} + SnapshotTestEventListener(std::string name, TestData& test_data) + : TestEventListener(std::move(name)), test_data_(test_data) {} void OnEvent(const DocumentSnapshot& value, Error error_code, const std::string& error_message) override { TestEventListener::OnEvent(value, error_code, error_message); - events_->push_back("doc"); + test_data_.AddEvent("doc"); } private: - std::vector* events_; + TestData& test_data_; }; - SnapshotTestEventListener listener{"doc", &events}; + SnapshotTestEventListener listener{"doc", test_data}; ListenerRegistration doc_registration = listener.AttachTo(&document); // Wait for the initial event from the backend so that we know we'll get // exactly one snapshot event for our local write below. Await(listener); - EXPECT_EQ(1, events.size()); - events.clear(); - -#if defined(__APPLE__) - // TODO(varconst): the implementation of `Semaphore::Post()` on Apple - // platforms has a data race which may result in semaphore data being accessed - // on the listener thread after it was destroyed on the main thread. To work - // around this, use `std::promise`. - std::promise promise; -#else - Semaphore completed{0}; -#endif + EXPECT_EQ(1, test_data.GetEventCount()); + test_data.ClearEvents(); #if defined(FIREBASE_USE_STD_FUNCTION) ListenerRegistration sync_registration = - TestFirestore()->AddSnapshotsInSyncListener([&] { - events.push_back("snapshots-in-sync"); - if (events.size() == 3) { -#if defined(__APPLE__) - promise.set_value(); -#else - completed.Post(); -#endif - } - }); + TestFirestore()->AddSnapshotsInSyncListener( + [&test_data] { test_data.AddEvent("snapshots-in-sync"); }); #else class SyncEventListener : public EventListener { public: - explicit SyncEventListener(std::vector* events, - Semaphore* completed) - : events_(events), completed_(completed) {} - - void OnEvent(Error) override { - events_->push_back("snapshots-in-sync"); - if (events.size() == 3) { - completed_->Post(); - } - } + explicit SyncEventListener(TestData& test_data) : test_data_(test_data) {} + + void OnEvent(Error) override { test_data_.AddEvent("snapshots-in-sync"); } private: - std::vector* events_ = nullptr; - Semaphore* completed_ = nullptr; + TestData& test_data_; }; - SyncEventListener sync_listener{&events, &completed}; + SyncEventListener sync_listener{test_data}; ListenerRegistration sync_registration = TestFirestore()->AddSnapshotsInSyncListener(sync_listener); #endif // defined(FIREBASE_USE_STD_FUNCTION) Await(document.Set(MapFieldValue{{"foo", FieldValue::Double(3.0)}})); // Wait for the snapshots-in-sync listener to fire afterwards. -#if defined(__APPLE__) - promise.get_future().wait(); -#else - completed.Wait(); -#endif + test_data.WaitForEventCount("snapshots-in-sync", 2); // We should have an initial snapshots-in-sync event, then a snapshot event // for set(), then another event to indicate we're in sync again. - EXPECT_EQ(events, std::vector( - {"snapshots-in-sync", "doc", "snapshots-in-sync"})); + EXPECT_EQ(test_data.GetEvents(), + std::vector( + {"snapshots-in-sync", "doc", "snapshots-in-sync"})); doc_registration.Remove(); sync_registration.Remove(); } @@ -741,45 +758,49 @@ TEST_F(FirestoreIntegrationTest, TestQueriesAreValidatedOnClient) { // The test harness will generate Java JUnit test regardless whether this is // inside a #if or not. So we move #if inside instead of enclose the whole case. TEST_F(FirestoreIntegrationTest, TestListenCanBeCalledMultipleTimes) { - // Note: this test is flaky -- the test case may finish, triggering the - // destruction of Firestore, before the async callback finishes. #if defined(FIREBASE_USE_STD_FUNCTION) + class TestData { + public: + void SetDocumentSnapshot(const DocumentSnapshot& document_snapshot) { + MutexLock lock(mutex_); + document_snapshot_ = document_snapshot; + is_document_snapshot_set_ = true; + } + + DocumentSnapshot WaitForDocumentSnapshot() { + while (true) { + MutexLock lock(mutex_); + if (is_document_snapshot_set_) { + return document_snapshot_; + } + } + } + + private: + Mutex mutex_; + DocumentSnapshot document_snapshot_; + bool is_document_snapshot_set_ = false; + }; + DocumentReference document = Collection("collection").Document(); WriteDocument(document, MapFieldValue{{"foo", FieldValue::String("bar")}}); -#if defined(__APPLE__) - // TODO(varconst): the implementation of `Semaphore::Post()` on Apple - // platforms has a data race which may result in semaphore data being accessed - // on the listener thread after it was destroyed on the main thread. To work - // around this, use `std::promise`. - std::promise promise; -#else - Semaphore completed{0}; -#endif - DocumentSnapshot resulting_data; - document.AddSnapshotListener([&](const DocumentSnapshot& snapshot, - Error error_code, - const std::string& error_message) { - EXPECT_EQ(Error::kErrorOk, error_code); - EXPECT_EQ(std::string(), error_message); - document.AddSnapshotListener([&](const DocumentSnapshot& snapshot, - Error error_code, - const std::string& error_message) { - EXPECT_EQ(Error::kErrorOk, error_code); - EXPECT_EQ(std::string(), error_message); - resulting_data = snapshot; -#if defined(__APPLE__) - promise.set_value(); -#else - completed.Post(); -#endif - }); - }); -#if defined(__APPLE__) - promise.get_future().wait(); -#else - completed.Wait(); -#endif - EXPECT_THAT(resulting_data.GetData(), + TestData test_data; + document.AddSnapshotListener( + [&document, &test_data](const DocumentSnapshot& snapshot, + Error error_code, + const std::string& error_message) { + EXPECT_EQ(Error::kErrorOk, error_code); + EXPECT_EQ(std::string(), error_message); + document.AddSnapshotListener( + [&test_data](const DocumentSnapshot& snapshot, Error error_code, + const std::string& error_message) { + EXPECT_EQ(Error::kErrorOk, error_code); + EXPECT_EQ(std::string(), error_message); + test_data.SetDocumentSnapshot(snapshot); + }); + }); + + EXPECT_THAT(test_data.WaitForDocumentSnapshot().GetData(), ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); #endif // defined(FIREBASE_USE_STD_FUNCTION) }