From 98fd15fae78f20a2a12a8a178af1720088528f07 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 6 Nov 2017 10:56:45 -0500 Subject: [PATCH 1/3] Avoid groupsync deadlock - pass in transaction // FREEBIE --- .../src/Messages/DeviceSyncing/OWSSyncGroupsMessage.h | 8 ++++++-- .../src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m | 2 +- SignalServiceKit/src/Messages/OWSMessageManager.m | 4 ++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.h b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.h index 530997f53..212d3051b 100644 --- a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.h +++ b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.h @@ -1,12 +1,16 @@ -// Copyright © 2016 Open Whisper Systems. All rights reserved. +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// #import "OWSOutgoingSyncMessage.h" NS_ASSUME_NONNULL_BEGIN +@class YapDatabaseReadTransaction; + @interface OWSSyncGroupsMessage : OWSOutgoingSyncMessage -- (NSData *)buildPlainTextAttachmentData; +- (NSData *)buildPlainTextAttachmentDataWithTransaction:(YapDatabaseReadTransaction *)transaction; @end diff --git a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m index 575f3d9eb..9e5a719c0 100644 --- a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m +++ b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m @@ -40,7 +40,7 @@ NS_ASSUME_NONNULL_BEGIN return syncMessageBuilder; } -- (NSData *)buildPlainTextAttachmentData +- (NSData *)buildPlainTextAttachmentDataWithTransaction:(YapDatabaseReadTransaction *)transaction { // TODO use temp file stream to avoid loading everything into memory at once // First though, we need to re-engineer our attachment process to accept streams (encrypting with stream, diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index 4b146b86b..5eb9e024b 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -615,8 +615,8 @@ NS_ASSUME_NONNULL_BEGIN }]; } else if (syncMessage.request.type == OWSSignalServiceProtosSyncMessageRequestTypeGroups) { OWSSyncGroupsMessage *syncGroupsMessage = [[OWSSyncGroupsMessage alloc] init]; - DataSource *dataSource = - [DataSourceValue dataSourceWithSyncMessage:[syncGroupsMessage buildPlainTextAttachmentData]]; + DataSource *dataSource = [DataSourceValue + dataSourceWithSyncMessage:[syncGroupsMessage buildPlainTextAttachmentDataWithTransaction:transaction]]; [self.messageSender sendTemporaryAttachmentData:dataSource contentType:OWSMimeTypeApplicationOctetStream inMessage:syncGroupsMessage From 8ef9e96b91dad2cd1eeb7f22e15c8fe7a969a285 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 6 Nov 2017 11:26:04 -0500 Subject: [PATCH 2/3] Avoid group-sync deadlock by making post-upload save async // FREEBIE --- SignalServiceKit/src/Network/API/OWSUploadingService.m | 4 +--- SignalServiceKit/src/Storage/TSYapDatabaseObject.h | 3 ++- SignalServiceKit/src/Storage/TSYapDatabaseObject.m | 8 ++++++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/SignalServiceKit/src/Network/API/OWSUploadingService.m b/SignalServiceKit/src/Network/API/OWSUploadingService.m index 97127bdb2..3cc067802 100644 --- a/SignalServiceKit/src/Network/API/OWSUploadingService.m +++ b/SignalServiceKit/src/Network/API/OWSUploadingService.m @@ -107,9 +107,7 @@ static const CGFloat kAttachmentUploadProgressTheta = 0.001f; DDLogInfo(@"%@ Uploaded attachment: %p.", self.tag, attachmentStream); attachmentStream.serverId = serverId; attachmentStream.isUploaded = YES; - [attachmentStream save]; - - successHandlerWrapper(); + [attachmentStream saveAsyncWithCompletionBlock:successHandlerWrapper]; } failure:failureHandlerWrapper]; diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h index 07dbcc6f8..f77f36b29 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h @@ -81,9 +81,10 @@ + (instancetype)fetchObjectWithUniqueID:(NSString *)uniqueID NS_SWIFT_NAME(fetch(uniqueId:)); /** - * Saves the object with a new YapDatabaseConnection + * Saves the object with shared readWrite connection. */ - (void)save; +- (void)saveAsyncWithCompletionBlock:(void (^_Nullable)(void))completionBlock; /** * Saves the object with the provided transaction diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.m b/SignalServiceKit/src/Storage/TSYapDatabaseObject.m index 032d55b48..ec392ccde 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.m +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.m @@ -37,6 +37,14 @@ }]; } +- (void)saveAsyncWithCompletionBlock:(void (^_Nullable)(void))completionBlock +{ + [[self dbReadWriteConnection] asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { + [self saveWithTransaction:transaction]; + } + completionBlock:completionBlock]; +} + - (void)touchWithTransaction:(YapDatabaseReadWriteTransaction *)transaction { [transaction touchObjectForKey:self.uniqueId inCollection:[self.class collection]]; From e82a3f3ddf4def4d34f8db344bbdb6b6b1c01d50 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 6 Nov 2017 11:52:28 -0500 Subject: [PATCH 3/3] respond to CR // FREEBIE --- .../DeviceSyncing/OWSSyncGroupsMessage.m | 18 ++++++++++-------- .../src/Storage/TSYapDatabaseObject.h | 13 ++++++++++++- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m index 9e5a719c0..651a7edb7 100644 --- a/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m +++ b/SignalServiceKit/src/Messages/DeviceSyncing/OWSSyncGroupsMessage.m @@ -49,14 +49,16 @@ NS_ASSUME_NONNULL_BEGIN [dataOutputStream open]; OWSGroupsOutputStream *groupsOutputStream = [OWSGroupsOutputStream streamWithOutputStream:dataOutputStream]; - [TSGroupThread enumerateCollectionObjectsUsingBlock:^(id obj, BOOL *stop) { - if (![obj isKindOfClass:[TSGroupThread class]]) { - DDLogVerbose(@"Ignoring non group thread in thread collection: %@", obj); - return; - } - TSGroupModel *group = ((TSGroupThread *)obj).groupModel; - [groupsOutputStream writeGroup:group]; - }]; + [TSGroupThread + enumerateCollectionObjectsWithTransaction:transaction + usingBlock:^(id obj, BOOL *stop) { + if (![obj isKindOfClass:[TSGroupThread class]]) { + DDLogVerbose(@"Ignoring non group thread in thread collection: %@", obj); + return; + } + TSGroupModel *group = ((TSGroupThread *)obj).groupModel; + [groupsOutputStream writeGroup:group]; + }]; [groupsOutputStream flush]; [dataOutputStream close]; diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h index f77f36b29..cb68f9496 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h @@ -81,9 +81,20 @@ + (instancetype)fetchObjectWithUniqueID:(NSString *)uniqueID NS_SWIFT_NAME(fetch(uniqueId:)); /** - * Saves the object with shared readWrite connection. + * Saves the object with the shared readWrite connection. + * + * This method will block if another readWrite transaction is open. */ - (void)save; + +/** + * Saves the object with the shared readWrite connection - does not block. + * + * Be mindful that this method may clobber other changes persisted + * while waiting to open the readWrite transaction. + * + * @param completionBlock is called on the main thread + */ - (void)saveAsyncWithCompletionBlock:(void (^_Nullable)(void))completionBlock; /**