From a9b55675cdc1a7f59b735d19ea4ee691e675ca56 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 22 Aug 2017 17:13:18 -0400 Subject: [PATCH 1/2] Add assert to ensure that we don't use write transactions before sync database view registration is complete. // FREEBIE --- .../src/Storage/TSStorageManager.m | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/SignalServiceKit/src/Storage/TSStorageManager.m b/SignalServiceKit/src/Storage/TSStorageManager.m index 5b3534805..fb8f53b19 100644 --- a/SignalServiceKit/src/Storage/TSStorageManager.m +++ b/SignalServiceKit/src/Storage/TSStorageManager.m @@ -33,6 +33,91 @@ static const NSString *const databaseName = @"Signal.sqlite"; static NSString *keychainService = @"TSKeyChainService"; static NSString *keychainDBPassAccount = @"TSDatabasePass"; +static BOOL isDatabaseInitializedFlag = NO; + +NSObject *isDatabaseInitializedFlagLock() +{ + static NSObject *instance = nil; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + instance = [NSObject new]; + }); + return instance; +} + +BOOL isDatabaseInitialized() +{ + @synchronized(isDatabaseInitializedFlagLock()) + { + return isDatabaseInitializedFlag; + } +} + +void setDatabaseInitialized() +{ + @synchronized(isDatabaseInitializedFlagLock()) + { + isDatabaseInitializedFlag = YES; + } +} + +#pragma mark - + +@interface YapDatabaseConnection () + +- (id)initWithDatabase:(YapDatabase *)inDatabase; + +@end + +#pragma mark - + +@interface OWSDatabaseConnection : YapDatabaseConnection + +@end + +#pragma mark - + +@implementation OWSDatabaseConnection + +- (void)readWriteWithBlock:(void (^)(YapDatabaseReadWriteTransaction *transaction))block +{ + OWSAssert(isDatabaseInitialized()); + + [super readWriteWithBlock:block]; +} + +@end + +#pragma mark - + +@interface YapDatabase () + +- (void)addConnection:(YapDatabaseConnection *)connection; + +@end + +#pragma mark - + +@interface OWSDatabase : YapDatabase + +@end + +#pragma mark - + +@implementation OWSDatabase + +- (YapDatabaseConnection *)newConnection +{ + YapDatabaseConnection *connection = [[OWSDatabaseConnection alloc] initWithDatabase:self]; + + [self addConnection:connection]; + return connection; +} + +@end + +#pragma mark - + @interface TSStorageManager () @property (nullable, atomic) YapDatabase *database; @@ -152,10 +237,18 @@ static NSString *keychainDBPassAccount = @"TSDatabasePass"; return databasePassword; }; +#ifdef DEBUG + _database = [[OWSDatabase alloc] initWithPath:[self dbPath] + serializer:NULL + deserializer:[[self class] logOnFailureDeserializer] + options:options]; +#else _database = [[YapDatabase alloc] initWithPath:[self dbPath] serializer:NULL deserializer:[[self class] logOnFailureDeserializer] options:options]; +#endif + if (!_database) { return NO; } @@ -198,6 +291,8 @@ static NSString *keychainDBPassAccount = @"TSDatabasePass"; [self.database registerExtension:[TSDatabaseSecondaryIndexes registerTimeStampIndex] withName:@"idx"]; [OWSMessageReceiver syncRegisterDatabaseExtension:self.database]; + setDatabaseInitialized(); + // Run the blocking migrations. // // These need to run _before_ the async registered database views or From 703b348091e77598c99135594bebc48f723aca41 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 23 Aug 2017 09:55:53 -0400 Subject: [PATCH 2/2] Respond to CR. // FREEBIE --- .../src/Storage/TSStorageManager.m | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/SignalServiceKit/src/Storage/TSStorageManager.m b/SignalServiceKit/src/Storage/TSStorageManager.m index fb8f53b19..d24250850 100644 --- a/SignalServiceKit/src/Storage/TSStorageManager.m +++ b/SignalServiceKit/src/Storage/TSStorageManager.m @@ -33,6 +33,9 @@ static const NSString *const databaseName = @"Signal.sqlite"; static NSString *keychainService = @"TSKeyChainService"; static NSString *keychainDBPassAccount = @"TSDatabasePass"; +#pragma mark - + +// This flag is only used in DEBUG builds. static BOOL isDatabaseInitializedFlag = NO; NSObject *isDatabaseInitializedFlagLock() @@ -71,6 +74,7 @@ void setDatabaseInitialized() #pragma mark - +// This class is only used in DEBUG builds. @interface OWSDatabaseConnection : YapDatabaseConnection @end @@ -79,6 +83,12 @@ void setDatabaseInitialized() @implementation OWSDatabaseConnection +// This clobbers the superclass implementation to include an assert which +// ensures that the database is in a ready state before creating write transactions. +// +// Creating write transactions before the _sync_ database views are registered +// causes YapDatabase to rebuild all of our database views, which is catastrophic. +// We're not sure why, but it causes YDB's "view version" checks to fail. - (void)readWriteWithBlock:(void (^)(YapDatabaseReadWriteTransaction *transaction))block { OWSAssert(isDatabaseInitialized()); @@ -90,6 +100,7 @@ void setDatabaseInitialized() #pragma mark - +// This class is only used in DEBUG builds. @interface YapDatabase () - (void)addConnection:(YapDatabaseConnection *)connection; @@ -106,6 +117,10 @@ void setDatabaseInitialized() @implementation OWSDatabase +// This clobbers the superclass implementation to include asserts which +// ensure that the database is in a ready state before creating write transactions. +// +// See comments in OWSDatabaseConnection. - (YapDatabaseConnection *)newConnection { YapDatabaseConnection *connection = [[OWSDatabaseConnection alloc] initWithDatabase:self]; @@ -291,6 +306,12 @@ void setDatabaseInitialized() [self.database registerExtension:[TSDatabaseSecondaryIndexes registerTimeStampIndex] withName:@"idx"]; [OWSMessageReceiver syncRegisterDatabaseExtension:self.database]; + // See comments on OWSDatabaseConnection. + // + // In the absence of finding documentation that can shed light on the issue we've been + // seeing, this issue only seems to affect sync and not async registrations. We've always + // been opening write transactions before the async registrations complete without negative + // consequences. setDatabaseInitialized(); // Run the blocking migrations.