From edd6f60a7385f9280c371df273703f6e104e325f Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Thu, 10 May 2018 20:16:50 -0400 Subject: Firestore: use C++ Executor for user queue. (#1238) FSTDispatchQueue enforces serial execution, which is inappropriate for user queue, because a user may configure usage of a concurrent queue in settings, breaking FSTDispatchQueue invariants. Instead, use C++ ExecutorLibdispatch directly. --- .../Example/Tests/Core/FSTQueryListenerTests.mm | 33 +++++++++------- Firestore/Source/API/FIRDocumentReference.mm | 4 +- Firestore/Source/API/FIRFirestore.mm | 9 ++++- Firestore/Source/API/FIRQuery.mm | 4 +- Firestore/Source/Core/FSTFirestoreClient.h | 19 +++++---- Firestore/Source/Core/FSTFirestoreClient.mm | 45 ++++++++++------------ Firestore/Source/Util/FSTAsyncQueryListener.h | 7 ++-- Firestore/Source/Util/FSTAsyncQueryListener.mm | 14 ++++--- 8 files changed, 74 insertions(+), 61 deletions(-) diff --git a/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm b/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm index 7ae9704..5629075 100644 --- a/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm +++ b/Firestore/Example/Tests/Core/FSTQueryListenerTests.mm @@ -15,6 +15,7 @@ */ #import +#include #import "Firestore/Source/Core/FSTEventManager.h" #import "Firestore/Source/Core/FSTQuery.h" @@ -23,23 +24,29 @@ #import "Firestore/Source/Model/FSTDocumentSet.h" #import "Firestore/Source/Remote/FSTRemoteEvent.h" #import "Firestore/Source/Util/FSTAsyncQueryListener.h" -#import "Firestore/Source/Util/FSTDispatchQueue.h" #import "Firestore/Example/Tests/Util/FSTHelpers.h" +#include "Firestore/core/src/firebase/firestore/util/executor_libdispatch.h" +#include "absl/memory/memory.h" + +using firebase::firestore::util::internal::ExecutorLibdispatch; using firebase::firestore::model::DocumentKeySet; NS_ASSUME_NONNULL_BEGIN @interface FSTQueryListenerTests : XCTestCase -@property(nonatomic, strong, readonly) FSTDispatchQueue *asyncQueue; @end -@implementation FSTQueryListenerTests +@implementation FSTQueryListenerTests { + std::unique_ptr _executor; +} - (void)setUp { - _asyncQueue = [FSTDispatchQueue - queueWith:dispatch_queue_create("FSTQueryListenerTests Queue", DISPATCH_QUEUE_SERIAL)]; + // TODO(varconst): moving this test to C++, it should be possible to store Executor as a value, + // not a pointer, and initialize it in the constructor. + _executor = absl::make_unique( + dispatch_queue_create("FSTQueryListenerTests Queue", DISPATCH_QUEUE_SERIAL)); } - (void)testRaisesCollectionEvents { @@ -131,12 +138,12 @@ NS_ASSUME_NONNULL_BEGIN FSTDocument *doc1 = FSTTestDoc("rooms/Eros", 3, @{@"name" : @"Eros"}, NO); FSTDocument *doc2 = FSTTestDoc("rooms/Eros", 4, @{@"name" : @"Eros2"}, NO); - __block FSTAsyncQueryListener *listener = [[FSTAsyncQueryListener alloc] - initWithDispatchQueue:self.asyncQueue - snapshotHandler:^(FSTViewSnapshot *snapshot, NSError *error) { - [accum addObject:snapshot]; - [listener mute]; - }]; + __block FSTAsyncQueryListener *listener = + [[FSTAsyncQueryListener alloc] initWithExecutor:_executor.get() + snapshotHandler:^(FSTViewSnapshot *snapshot, NSError *error) { + [accum addObject:snapshot]; + [listener mute]; + }]; FSTView *view = [[FSTView alloc] initWithQuery:query remoteDocuments:DocumentKeySet{}]; FSTViewSnapshot *viewSnapshot1 = FSTTestApplyChanges(view, @[ doc1 ], nil); @@ -148,9 +155,7 @@ NS_ASSUME_NONNULL_BEGIN // Drain queue XCTestExpectation *expectation = [self expectationWithDescription:@"Queue drained"]; - [self.asyncQueue dispatchAsync:^{ - [expectation fulfill]; - }]; + _executor->Execute([=] { [expectation fulfill]; }); [self waitForExpectationsWithTimeout:4.0 handler:^(NSError *_Nullable expectationError) { diff --git a/Firestore/Source/API/FIRDocumentReference.mm b/Firestore/Source/API/FIRDocumentReference.mm index da67a5b..5ad606c 100644 --- a/Firestore/Source/API/FIRDocumentReference.mm +++ b/Firestore/Source/API/FIRDocumentReference.mm @@ -280,8 +280,8 @@ addSnapshotListenerInternalWithOptions:(FSTListenOptions *)internalOptions }; FSTAsyncQueryListener *asyncListener = - [[FSTAsyncQueryListener alloc] initWithDispatchQueue:self.firestore.client.userDispatchQueue - snapshotHandler:snapshotHandler]; + [[FSTAsyncQueryListener alloc] initWithExecutor:self.firestore.client.userExecutor + snapshotHandler:snapshotHandler]; FSTQueryListener *internalListener = [firestore.client listenToQuery:query diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm index fe461d6..e5f0c12 100644 --- a/Firestore/Source/API/FIRFirestore.mm +++ b/Firestore/Source/API/FIRFirestore.mm @@ -45,12 +45,16 @@ #include "Firestore/core/src/firebase/firestore/util/string_apple.h" #include "absl/memory/memory.h" +#include "Firestore/core/src/firebase/firestore/util/executor_libdispatch.h" + namespace util = firebase::firestore::util; using firebase::firestore::auth::CredentialsProvider; using firebase::firestore::auth::FirebaseCredentialsProvider; using firebase::firestore::core::DatabaseInfo; using firebase::firestore::model::DatabaseId; using firebase::firestore::model::ResourcePath; +using util::internal::Executor; +using util::internal::ExecutorLibdispatch; NS_ASSUME_NONNULL_BEGIN @@ -272,12 +276,13 @@ extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain"; const DatabaseInfo database_info(*self.databaseID, util::MakeStringView(_persistenceKey), util::MakeStringView(_settings.host), _settings.sslEnabled); - FSTDispatchQueue *userDispatchQueue = [FSTDispatchQueue queueWith:_settings.dispatchQueue]; + std::unique_ptr userExecutor = + absl::make_unique(_settings.dispatchQueue); _client = [FSTFirestoreClient clientWithDatabaseInfo:database_info usePersistence:_settings.persistenceEnabled credentialsProvider:_credentialsProvider.get() - userDispatchQueue:userDispatchQueue + userExecutor:std::move(userExecutor) workerDispatchQueue:_workerDispatchQueue]; } } diff --git a/Firestore/Source/API/FIRQuery.mm b/Firestore/Source/API/FIRQuery.mm index c83af9c..596f6ac 100644 --- a/Firestore/Source/API/FIRQuery.mm +++ b/Firestore/Source/API/FIRQuery.mm @@ -183,8 +183,8 @@ addSnapshotListenerInternalWithOptions:(FSTListenOptions *)internalOptions }; FSTAsyncQueryListener *asyncListener = - [[FSTAsyncQueryListener alloc] initWithDispatchQueue:self.firestore.client.userDispatchQueue - snapshotHandler:snapshotHandler]; + [[FSTAsyncQueryListener alloc] initWithExecutor:self.firestore.client.userExecutor + snapshotHandler:snapshotHandler]; FSTQueryListener *internalListener = [firestore.client listenToQuery:query diff --git a/Firestore/Source/Core/FSTFirestoreClient.h b/Firestore/Source/Core/FSTFirestoreClient.h index 7285e65..94c2284 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.h +++ b/Firestore/Source/Core/FSTFirestoreClient.h @@ -15,6 +15,7 @@ */ #import +#include #import "Firestore/Source/Core/FSTTypes.h" #import "Firestore/Source/Core/FSTViewSnapshot.h" @@ -23,6 +24,7 @@ #include "Firestore/core/src/firebase/firestore/auth/credentials_provider.h" #include "Firestore/core/src/firebase/firestore/core/database_info.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" +#include "Firestore/core/src/firebase/firestore/util/executor.h" @class FIRDocumentReference; @class FIRDocumentSnapshot; @@ -50,14 +52,15 @@ NS_ASSUME_NONNULL_BEGIN /** * Creates and returns a FSTFirestoreClient with the given parameters. * - * All callbacks and events will be triggered on the provided userDispatchQueue. + * All callbacks and events will be triggered on the provided userExecutor. */ -+ (instancetype)clientWithDatabaseInfo:(const firebase::firestore::core::DatabaseInfo &)databaseInfo - usePersistence:(BOOL)usePersistence - credentialsProvider:(firebase::firestore::auth::CredentialsProvider *) - credentialsProvider // no passing ownership - userDispatchQueue:(FSTDispatchQueue *)userDispatchQueue - workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue; ++ (instancetype) +clientWithDatabaseInfo:(const firebase::firestore::core::DatabaseInfo &)databaseInfo + usePersistence:(BOOL)usePersistence + credentialsProvider:(firebase::firestore::auth::CredentialsProvider *) + credentialsProvider // no passing ownership + userExecutor:(std::unique_ptr)userExecutor + workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue; - (instancetype)init __attribute__((unavailable("Use static constructor method."))); @@ -111,7 +114,7 @@ NS_ASSUME_NONNULL_BEGIN * Dispatch queue for user callbacks / events. This will often be the "Main Dispatch Queue" of the * app but the developer can configure it to a different queue if they so choose. */ -@property(nonatomic, strong, readonly) FSTDispatchQueue *userDispatchQueue; +- (firebase::firestore::util::internal::Executor *)userExecutor; @end diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 658cf57..93d1f7c 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -18,6 +18,7 @@ #include // NOLINT(build/c++11) #include +#include #import "FIRFirestoreErrors.h" #import "Firestore/Source/API/FIRDocumentReference+Internal.h" @@ -58,6 +59,8 @@ using firebase::firestore::core::DatabaseInfo; using firebase::firestore::model::DatabaseId; using firebase::firestore::model::DocumentKeySet; +using firebase::firestore::util::internal::Executor; + NS_ASSUME_NONNULL_BEGIN @interface FSTFirestoreClient () { @@ -68,7 +71,7 @@ NS_ASSUME_NONNULL_BEGIN usePersistence:(BOOL)usePersistence credentialsProvider: (CredentialsProvider *)credentialsProvider // no passing ownership - userDispatchQueue:(FSTDispatchQueue *)userDispatchQueue + userExecutor:(std::unique_ptr)userExecutor workerDispatchQueue:(FSTDispatchQueue *)queue NS_DESIGNATED_INITIALIZER; @property(nonatomic, assign, readonly) const DatabaseInfo *databaseInfo; @@ -91,18 +94,24 @@ NS_ASSUME_NONNULL_BEGIN @end -@implementation FSTFirestoreClient +@implementation FSTFirestoreClient { + std::unique_ptr _userExecutor; +} + +- (Executor *)userExecutor { + return _userExecutor.get(); +} + (instancetype)clientWithDatabaseInfo:(const DatabaseInfo &)databaseInfo usePersistence:(BOOL)usePersistence credentialsProvider: (CredentialsProvider *)credentialsProvider // no passing ownership - userDispatchQueue:(FSTDispatchQueue *)userDispatchQueue + userExecutor:(std::unique_ptr)userExecutor workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue { return [[FSTFirestoreClient alloc] initWithDatabaseInfo:databaseInfo usePersistence:usePersistence credentialsProvider:credentialsProvider - userDispatchQueue:userDispatchQueue + userExecutor:std::move(userExecutor) workerDispatchQueue:workerDispatchQueue]; } @@ -110,12 +119,12 @@ NS_ASSUME_NONNULL_BEGIN usePersistence:(BOOL)usePersistence credentialsProvider: (CredentialsProvider *)credentialsProvider // no passing ownership - userDispatchQueue:(FSTDispatchQueue *)userDispatchQueue + userExecutor:(std::unique_ptr)userExecutor workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue { if (self = [super init]) { _databaseInfo = databaseInfo; _credentialsProvider = credentialsProvider; - _userDispatchQueue = userDispatchQueue; + _userExecutor = std::move(userExecutor); _workerDispatchQueue = workerDispatchQueue; auto userPromise = std::make_shared>(); @@ -230,9 +239,7 @@ NS_ASSUME_NONNULL_BEGIN [self.workerDispatchQueue dispatchAsync:^{ [self.remoteStore disableNetwork]; if (completion) { - [self.userDispatchQueue dispatchAsync:^{ - completion(nil); - }]; + _userExecutor->Execute([=] { completion(nil); }); } }]; } @@ -241,9 +248,7 @@ NS_ASSUME_NONNULL_BEGIN [self.workerDispatchQueue dispatchAsync:^{ [self.remoteStore enableNetwork]; if (completion) { - [self.userDispatchQueue dispatchAsync:^{ - completion(nil); - }]; + _userExecutor->Execute([=] { completion(nil); }); } }]; } @@ -255,9 +260,7 @@ NS_ASSUME_NONNULL_BEGIN [self.remoteStore shutdown]; [self.persistence shutdown]; if (completion) { - [self.userDispatchQueue dispatchAsync:^{ - completion(nil); - }]; + _userExecutor->Execute([=] { completion(nil); }); } }]; } @@ -338,18 +341,14 @@ NS_ASSUME_NONNULL_BEGIN [self.workerDispatchQueue dispatchAsync:^{ if (mutations.count == 0) { if (completion) { - [self.userDispatchQueue dispatchAsync:^{ - completion(nil); - }]; + _userExecutor->Execute([=] { completion(nil); }); } } else { [self.syncEngine writeMutations:mutations completion:^(NSError *error) { // Dispatch the result back onto the user dispatch queue. if (completion) { - [self.userDispatchQueue dispatchAsync:^{ - completion(error); - }]; + _userExecutor->Execute([=] { completion(error); }); } }]; } @@ -366,9 +365,7 @@ NS_ASSUME_NONNULL_BEGIN completion:^(id _Nullable result, NSError *_Nullable error) { // Dispatch the result back onto the user dispatch queue. if (completion) { - [self.userDispatchQueue dispatchAsync:^{ - completion(result, error); - }]; + _userExecutor->Execute([=] { completion(result, error); }); } }]; }]; diff --git a/Firestore/Source/Util/FSTAsyncQueryListener.h b/Firestore/Source/Util/FSTAsyncQueryListener.h index 4888268..06471d8 100644 --- a/Firestore/Source/Util/FSTAsyncQueryListener.h +++ b/Firestore/Source/Util/FSTAsyncQueryListener.h @@ -18,6 +18,8 @@ #import "Firestore/Source/Core/FSTViewSnapshot.h" +#include "Firestore/core/src/firebase/firestore/util/executor.h" + NS_ASSUME_NONNULL_BEGIN @class FSTDispatchQueue; @@ -28,9 +30,8 @@ NS_ASSUME_NONNULL_BEGIN */ @interface FSTAsyncQueryListener : NSObject -- (instancetype)initWithDispatchQueue:(FSTDispatchQueue *)dispatchQueue - snapshotHandler:(FSTViewSnapshotHandler)snapshotHandler - NS_DESIGNATED_INITIALIZER; +- (instancetype)initWithExecutor:(firebase::firestore::util::internal::Executor*)executor + snapshotHandler:(FSTViewSnapshotHandler)snapshotHandler NS_DESIGNATED_INITIALIZER; - (instancetype)init NS_UNAVAILABLE; diff --git a/Firestore/Source/Util/FSTAsyncQueryListener.mm b/Firestore/Source/Util/FSTAsyncQueryListener.mm index b72ac57..426594b 100644 --- a/Firestore/Source/Util/FSTAsyncQueryListener.mm +++ b/Firestore/Source/Util/FSTAsyncQueryListener.mm @@ -18,16 +18,18 @@ #import "Firestore/Source/Util/FSTDispatchQueue.h" +using firebase::firestore::util::internal::Executor; + @implementation FSTAsyncQueryListener { volatile BOOL _muted; FSTViewSnapshotHandler _snapshotHandler; - FSTDispatchQueue *_dispatchQueue; + Executor *_executor; } -- (instancetype)initWithDispatchQueue:(FSTDispatchQueue *)dispatchQueue - snapshotHandler:(FSTViewSnapshotHandler)snapshotHandler { +- (instancetype)initWithExecutor:(Executor *)executor + snapshotHandler:(FSTViewSnapshotHandler)snapshotHandler { if (self = [super init]) { - _dispatchQueue = dispatchQueue; + _executor = executor; _snapshotHandler = snapshotHandler; } return self; @@ -40,11 +42,11 @@ // users just want to turn on notifications "forever" and don't want to have // to keep track of our handle to keep them going. return ^(FSTViewSnapshot *_Nullable snapshot, NSError *_Nullable error) { - [self->_dispatchQueue dispatchAsync:^{ + _executor->Execute([self, snapshot, error] { if (!self->_muted) { self->_snapshotHandler(snapshot, error); } - }]; + }); }; } -- cgit v1.2.3