From 3eaeb4e0ecc6d4ccbf9e4dc7a869dd81886f9788 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 14 Sep 2017 17:06:35 -0400 Subject: [PATCH 1/2] Add read receipts manager. * Simplify read receipts plumbing. * Rework incoming read receipts handling. * Rework outgoing read receipts handling. * Make "database view registration complete" check thread-safe. * Don't send sync messages to self if no linked devices. // FREEBIE --- Signal.xcodeproj/project.pbxproj | 16 -- Signal/src/AppDelegate.m | 11 - .../Observers/OWSStaleNotificationObserver.h | 15 -- .../Observers/OWSStaleNotificationObserver.m | 71 ------- Signal/src/network/PushManager.m | 16 ++ SignalServiceKit/Utilities/precommit.py | 12 +- .../src/Devices/OWSReadReceiptsProcessor.h | 1 + .../src/Devices/OWSSendReadReceiptsJob.h | 18 -- .../src/Devices/OWSSendReadReceiptsJob.m | 108 ---------- .../Messages/Interactions/TSIncomingMessage.h | 2 - .../Messages/Interactions/TSIncomingMessage.m | 8 +- .../Messages/OWSIncomingMessageReadObserver.h | 19 -- .../Messages/OWSIncomingMessageReadObserver.m | 79 -------- .../src/Messages/OWSMessageSender.m | 19 ++ .../src/Messages/OWSReadReceiptManager.h | 43 ++++ .../src/Messages/OWSReadReceiptManager.m | 191 ++++++++++++++++++ 16 files changed, 279 insertions(+), 350 deletions(-) delete mode 100644 Signal/src/Observers/OWSStaleNotificationObserver.h delete mode 100644 Signal/src/Observers/OWSStaleNotificationObserver.m delete mode 100644 SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.h delete mode 100644 SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.m delete mode 100644 SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.h delete mode 100644 SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.m create mode 100644 SignalServiceKit/src/Messages/OWSReadReceiptManager.h create mode 100644 SignalServiceKit/src/Messages/OWSReadReceiptManager.m diff --git a/Signal.xcodeproj/project.pbxproj b/Signal.xcodeproj/project.pbxproj index e961a5c71..b8abedc39 100644 --- a/Signal.xcodeproj/project.pbxproj +++ b/Signal.xcodeproj/project.pbxproj @@ -203,8 +203,6 @@ 45BB93381E688E14001E3939 /* UIDevice+featureSupport.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45BB93371E688E14001E3939 /* UIDevice+featureSupport.swift */; }; 45BB93391E688E14001E3939 /* UIDevice+featureSupport.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45BB93371E688E14001E3939 /* UIDevice+featureSupport.swift */; }; 45BD60821DE9547E00A8F436 /* Contacts.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 45BD60811DE9547E00A8F436 /* Contacts.framework */; settings = {ATTRIBUTES = (Weak, ); }; }; - 45BFFFA81D898AF0004A12A7 /* OWSStaleNotificationObserver.m in Sources */ = {isa = PBXBuildFile; fileRef = 45BFFFA71D898AF0004A12A7 /* OWSStaleNotificationObserver.m */; }; - 45BFFFA91D898AF0004A12A7 /* OWSStaleNotificationObserver.m in Sources */ = {isa = PBXBuildFile; fileRef = 45BFFFA71D898AF0004A12A7 /* OWSStaleNotificationObserver.m */; }; 45C0DC1B1E68FE9000E04C47 /* UIApplication+OWS.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45C0DC1A1E68FE9000E04C47 /* UIApplication+OWS.swift */; }; 45C0DC1C1E68FE9000E04C47 /* UIApplication+OWS.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45C0DC1A1E68FE9000E04C47 /* UIApplication+OWS.swift */; }; 45C0DC1E1E69011F00E04C47 /* UIStoryboard+OWS.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45C0DC1D1E69011F00E04C47 /* UIStoryboard+OWS.swift */; }; @@ -668,8 +666,6 @@ 45B201741DAECBFD00C461E0 /* Signal-Bridging-Header.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "Signal-Bridging-Header.h"; sourceTree = ""; }; 45BB93371E688E14001E3939 /* UIDevice+featureSupport.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "UIDevice+featureSupport.swift"; sourceTree = ""; }; 45BD60811DE9547E00A8F436 /* Contacts.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Contacts.framework; path = System/Library/Frameworks/Contacts.framework; sourceTree = SDKROOT; }; - 45BFFFA61D898AF0004A12A7 /* OWSStaleNotificationObserver.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = OWSStaleNotificationObserver.h; path = Observers/OWSStaleNotificationObserver.h; sourceTree = ""; }; - 45BFFFA71D898AF0004A12A7 /* OWSStaleNotificationObserver.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = OWSStaleNotificationObserver.m; path = Observers/OWSStaleNotificationObserver.m; sourceTree = ""; }; 45C0DC1A1E68FE9000E04C47 /* UIApplication+OWS.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "UIApplication+OWS.swift"; sourceTree = ""; }; 45C0DC1D1E69011F00E04C47 /* UIStoryboard+OWS.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "UIStoryboard+OWS.swift"; sourceTree = ""; }; 45C681B51D305A580050903A /* OWSCall.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OWSCall.h; sourceTree = ""; }; @@ -1260,15 +1256,6 @@ name = Models; sourceTree = ""; }; - 45BFFFA51D898AB8004A12A7 /* Observers */ = { - isa = PBXGroup; - children = ( - 45BFFFA61D898AF0004A12A7 /* OWSStaleNotificationObserver.h */, - 45BFFFA71D898AF0004A12A7 /* OWSStaleNotificationObserver.m */, - ); - name = Observers; - sourceTree = ""; - }; 45CD81A41DBFF8CF004C9430 /* Storyboards */ = { isa = PBXGroup; children = ( @@ -1339,7 +1326,6 @@ 45D231751DC7E8C50034FA89 /* Jobs */, 457F3AC01D14A0F700C51351 /* Models */, 76EB041D18170B33006006FC /* network */, - 45BFFFA51D898AB8004A12A7 /* Observers */, 34CE88E81F3237260098030F /* Profiles */, B60959791C2C0FA9004E8797 /* rating */, 45B201741DAECBFD00C461E0 /* Signal-Bridging-Header.h */, @@ -2260,7 +2246,6 @@ 76EB064218170B33006006FC /* StringUtil.m in Sources */, 452037D11EE84975004E4CDF /* DebugUISessionState.m in Sources */, 342FCE6B1EF9C375002690AD /* OWS105AttachmentFilePaths.m in Sources */, - 45BFFFA81D898AF0004A12A7 /* OWSStaleNotificationObserver.m in Sources */, 45C681B71D305A580050903A /* OWSCall.m in Sources */, D221A09A169C9E5E00537ABF /* main.m in Sources */, 345671011E89A5F1006EE662 /* ThreadUtil.m in Sources */, @@ -2423,7 +2408,6 @@ 456AC8351E3A776300A3C7FC /* WeakTimer.swift in Sources */, 458E383A1D6699FA0094BD24 /* OWSDeviceProvisioningURLParserTest.m in Sources */, 452D1EE81DCA90D100A57EC4 /* MesssagesBubblesSizeCalculatorTest.swift in Sources */, - 45BFFFA91D898AF0004A12A7 /* OWSStaleNotificationObserver.m in Sources */, 451DE9FE1DC1A28200810E42 /* SyncPushTokensJob.swift in Sources */, 45F170AF1E2F0393003FC1F2 /* CallAudioSessionTest.swift in Sources */, 34B3F8991E8DF1B90035BE1A /* TSMessageAdapterTest.m in Sources */, diff --git a/Signal/src/AppDelegate.m b/Signal/src/AppDelegate.m index dbaf598cf..ba4b0f0bc 100644 --- a/Signal/src/AppDelegate.m +++ b/Signal/src/AppDelegate.m @@ -14,7 +14,6 @@ #import "OWSNavigationController.h" #import "OWSPreferences.h" #import "OWSProfileManager.h" -#import "OWSStaleNotificationObserver.h" #import "Pastelog.h" #import "PushManager.h" #import "RegistrationViewController.h" @@ -29,7 +28,6 @@ #import #import #import -#import #import #import #import @@ -51,8 +49,6 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; @interface AppDelegate () @property (nonatomic) UIWindow *screenProtectionWindow; -@property (nonatomic) OWSIncomingMessageReadObserver *incomingMessageReadObserver; -@property (nonatomic) OWSStaleNotificationObserver *staleNotificationObserver; @property (nonatomic) OWSContactsSyncing *contactsSyncing; @property (nonatomic) BOOL hasInitialRootViewController; @@ -270,13 +266,6 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; [VersionMigrations runSafeBlockingMigrations]; }]; [[Environment getCurrent].contactsManager startObserving]; - - self.incomingMessageReadObserver = - [[OWSIncomingMessageReadObserver alloc] initWithMessageSender:[Environment getCurrent].messageSender]; - [self.incomingMessageReadObserver startObserving]; - - self.staleNotificationObserver = [OWSStaleNotificationObserver new]; - [self.staleNotificationObserver startObserving]; } - (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken diff --git a/Signal/src/Observers/OWSStaleNotificationObserver.h b/Signal/src/Observers/OWSStaleNotificationObserver.h deleted file mode 100644 index 833a47aaa..000000000 --- a/Signal/src/Observers/OWSStaleNotificationObserver.h +++ /dev/null @@ -1,15 +0,0 @@ -// Created by Michael Kirk on 9/14/16. -// Copyright © 2016 Open Whisper Systems. All rights reserved. - -NS_ASSUME_NONNULL_BEGIN - -@class PushManager; - -@interface OWSStaleNotificationObserver : NSObject - -- (instancetype)initWithPushManager:(PushManager *)pushManager NS_DESIGNATED_INITIALIZER; -- (void)startObserving; - -@end - -NS_ASSUME_NONNULL_END diff --git a/Signal/src/Observers/OWSStaleNotificationObserver.m b/Signal/src/Observers/OWSStaleNotificationObserver.m deleted file mode 100644 index b4576d369..000000000 --- a/Signal/src/Observers/OWSStaleNotificationObserver.m +++ /dev/null @@ -1,71 +0,0 @@ -// Created by Michael Kirk on 9/14/16. -// Copyright © 2016 Open Whisper Systems. All rights reserved. - -#import "OWSStaleNotificationObserver.h" -#import "PushManager.h" -#import -#import - -NS_ASSUME_NONNULL_BEGIN - -@interface OWSStaleNotificationObserver () - -@property (nonatomic, readonly) PushManager *pushManager; - -@end - -@implementation OWSStaleNotificationObserver - -- (void)dealloc -{ - [[NSNotificationCenter defaultCenter] removeObserver:self]; -} - -- (instancetype)init -{ - return [self initWithPushManager:[PushManager sharedManager]]; -} - -- (instancetype)initWithPushManager:(PushManager *)pushManager -{ - self = [super init]; - if (!self) { - return self; - } - - _pushManager = pushManager; - - return self; -} - -- (void)startObserving -{ - [[NSNotificationCenter defaultCenter] addObserver:self - selector:@selector(handleMessageRead:) - name:OWSReadReceiptsProcessorMarkedMessageAsReadNotification - object:nil]; -} - -- (void)handleMessageRead:(NSNotification *)notification -{ - if ([notification.object isKindOfClass:[TSIncomingMessage class]]) { - TSIncomingMessage *message = (TSIncomingMessage *)notification.object; - - DDLogDebug(@"%@ canceled notification for message:%@", self.tag, message); - [self.pushManager cancelNotificationsWithThreadId:message.uniqueThreadId]; - } -} - -+ (NSString *)tag -{ - return [NSString stringWithFormat:@"[%@]", self.class]; -} - -- (NSString *)tag -{ - return self.class.tag; -} - -@end - -NS_ASSUME_NONNULL_END diff --git a/Signal/src/network/PushManager.m b/Signal/src/network/PushManager.m index 12e69bdc4..2cd4cca8d 100644 --- a/Signal/src/network/PushManager.m +++ b/Signal/src/network/PushManager.m @@ -11,6 +11,7 @@ #import #import #import +#import #import #import #import @@ -81,9 +82,24 @@ NSString *const Signal_Message_MarkAsRead_Identifier = @"Signal_Message_MarkAsRe OWSSingletonAssert(); + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(handleMessageRead:) + name:OWSReadReceiptsProcessorMarkedMessageAsReadNotification + object:nil]; + return self; } +- (void)handleMessageRead:(NSNotification *)notification +{ + if ([notification.object isKindOfClass:[TSIncomingMessage class]]) { + TSIncomingMessage *message = (TSIncomingMessage *)notification.object; + + DDLogDebug(@"%@ canceled notification for message:%@", self.tag, message); + [self cancelNotificationsWithThreadId:message.uniqueThreadId]; + } +} + #pragma mark Manage Incoming Push - (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)userInfo { diff --git a/SignalServiceKit/Utilities/precommit.py b/SignalServiceKit/Utilities/precommit.py index f69953381..b1cc9640e 100755 --- a/SignalServiceKit/Utilities/precommit.py +++ b/SignalServiceKit/Utilities/precommit.py @@ -189,7 +189,7 @@ def find_matching_section(text, match_test): return text0, text1, text2 -def sort_matching_blocks(filepath, filename, file_extension, text, match_func, sort_func): +def sort_matching_blocks(sort_name, filepath, filename, file_extension, text, match_func, sort_func): unprocessed = text processed = None while True: @@ -230,6 +230,8 @@ def sort_matching_blocks(filepath, filename, file_extension, text, match_func, s else: break + if text != processed: + print sort_name, filepath return processed @@ -249,17 +251,17 @@ def find_include_section(text): def sort_includes(filepath, filename, file_extension, text): - print 'sort_includes', filepath + # print 'sort_includes', filepath if file_extension not in ('.h', '.m', '.mm'): return text - return sort_matching_blocks(filepath, filename, file_extension, text, find_include_section, sort_include_block) + return sort_matching_blocks('sort_includes', filepath, filename, file_extension, text, find_include_section, sort_include_block) def sort_class_statements(filepath, filename, file_extension, text): - print 'sort_class_statements', filepath + # print 'sort_class_statements', filepath if file_extension not in ('.h', '.m', '.mm'): return text - return sort_matching_blocks(filepath, filename, file_extension, text, find_class_statement_section, sort_class_statement_block) + return sort_matching_blocks('sort_class_statements', filepath, filename, file_extension, text, find_class_statement_section, sort_class_statement_block) def splitall(path): diff --git a/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.h b/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.h index 2af1075f7..ded956f09 100644 --- a/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.h +++ b/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.h @@ -12,6 +12,7 @@ NS_ASSUME_NONNULL_BEGIN extern NSString *const OWSReadReceiptsProcessorMarkedMessageAsReadNotification; +// TODO: @interface OWSReadReceiptsProcessor : NSObject /** diff --git a/SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.h b/SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.h deleted file mode 100644 index 860694bcd..000000000 --- a/SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.h +++ /dev/null @@ -1,18 +0,0 @@ -// Created by Michael Kirk on 9/24/16. -// Copyright © 2016 Open Whisper Systems. All rights reserved. - -NS_ASSUME_NONNULL_BEGIN - -@class OWSMessageSender; -@class TSIncomingMessage; - -@interface OWSSendReadReceiptsJob : NSObject - -- (instancetype)init NS_UNAVAILABLE; -- (instancetype)initWithMessageSender:(OWSMessageSender *)messageSender NS_DESIGNATED_INITIALIZER; -- (void)runWith:(TSIncomingMessage *)message; - - -@end - -NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.m b/SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.m deleted file mode 100644 index f88b4cd3d..000000000 --- a/SignalServiceKit/src/Devices/OWSSendReadReceiptsJob.m +++ /dev/null @@ -1,108 +0,0 @@ -// -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. -// - -#import "OWSSendReadReceiptsJob.h" -#import "OWSMessageSender.h" -#import "OWSReadReceipt.h" -#import "OWSReadReceiptsMessage.h" -#import "TSContactThread.h" -#import "TSIncomingMessage.h" - -NS_ASSUME_NONNULL_BEGIN - -@interface OWSSendReadReceiptsJob () - -@property (atomic) NSMutableArray *readReceiptsQueue; -@property (nonatomic, readonly) OWSMessageSender *messageSender; -@property BOOL isObserving; - -@end - -@implementation OWSSendReadReceiptsJob - -- (void)dealloc -{ - [[NSNotificationCenter defaultCenter] removeObserver:self]; -} - -- (instancetype)initWithMessageSender:(OWSMessageSender *)messageSender -{ - self = [super init]; - if (!self) { - return self; - } - - _readReceiptsQueue = [NSMutableArray new]; - _messageSender = messageSender; - _isObserving = NO; - - return self; -} - -- (void)runWith:(TSIncomingMessage *)message -{ - // authorId isn't set on all legacy messages, so we take - // extra measures to ensure we obtain a valid value. - NSString *messageAuthorId; - if (message.authorId) { // Group Thread - messageAuthorId = message.authorId; - } else { // Contact Thread - messageAuthorId = [TSContactThread contactIdFromThreadId:message.uniqueThreadId]; - } - - OWSReadReceipt *readReceipt = [[OWSReadReceipt alloc] initWithSenderId:messageAuthorId timestamp:message.timestamp]; - [self.readReceiptsQueue addObject:readReceipt]; - - // Wait a bit to bundle up read receipts into one request. - __weak typeof(self) weakSelf = self; - [weakSelf performSelector:@selector(sendAllReadReceiptsInQueue) withObject:nil afterDelay:2.0]; -} - -- (void)sendAllReadReceiptsInQueue -{ - // Synchronized so we don't lose any read receipts while replacing the queue - __block NSArray *_Nullable receiptsToSend; - @synchronized(self) - { - if (self.readReceiptsQueue.count > 0) { - receiptsToSend = self.readReceiptsQueue; - self.readReceiptsQueue = [NSMutableArray new]; - } - } - - if (receiptsToSend) { - [self sendReadReceipts:receiptsToSend]; - } else { - DDLogVerbose(@"Read receipts queue already drained."); - } -} - -- (void)sendReadReceipts:(NSArray *)readReceipts -{ - OWSReadReceiptsMessage *message = [[OWSReadReceiptsMessage alloc] initWithReadReceipts:readReceipts]; - - [self.messageSender sendMessage:message - success:^{ - DDLogInfo(@"%@ Successfully sent %ld read receipt", self.tag, (unsigned long)readReceipts.count); - } - failure:^(NSError *error) { - DDLogError(@"%@ Failed to send read receipt with error: %@", self.tag, error); - }]; -} - -#pragma mark - Logging - -+ (NSString *)tag -{ - return [NSString stringWithFormat:@"[%@]", self.class]; -} - -- (NSString *)tag -{ - return self.class.tag; -} - -@end - -NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h index b8b543a10..1f0098d31 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h @@ -10,8 +10,6 @@ NS_ASSUME_NONNULL_BEGIN @class TSContactThread; @class TSGroupThread; -extern NSString *const TSIncomingMessageWasReadOnThisDeviceNotification; - @interface TSIncomingMessage : TSMessage /** diff --git a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m index 4c690ba6e..f8585d074 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m @@ -5,6 +5,7 @@ #import "TSIncomingMessage.h" #import "OWSDisappearingMessagesConfiguration.h" #import "OWSDisappearingMessagesJob.h" +#import "OWSReadReceiptManager.h" #import "TSContactThread.h" #import "TSDatabaseSecondaryIndexes.h" #import "TSGroupThread.h" @@ -12,8 +13,6 @@ NS_ASSUME_NONNULL_BEGIN -NSString *const TSIncomingMessageWasReadOnThisDeviceNotification = @"TSIncomingMessageWasReadOnThisDeviceNotification"; - @interface TSIncomingMessage () @property (nonatomic, getter=wasRead) BOOL read; @@ -141,10 +140,7 @@ NSString *const TSIncomingMessageWasReadOnThisDeviceNotification = @"TSIncomingM if (sendReadReceipt) { // Notification must happen outside of the transaction, else we'll likely crash when the notification receiver // tries to do anything with the DB. - dispatch_async(dispatch_get_main_queue(), ^{ - [[NSNotificationCenter defaultCenter] postNotificationName:TSIncomingMessageWasReadOnThisDeviceNotification - object:self]; - }); + [OWSReadReceiptManager.sharedManager messageWasReadLocally:self]; } } diff --git a/SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.h b/SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.h deleted file mode 100644 index f33cf5480..000000000 --- a/SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.h +++ /dev/null @@ -1,19 +0,0 @@ -// -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. -// - -NS_ASSUME_NONNULL_BEGIN - -@class TSStorageManager; -@class OWSMessageSender; - -@interface OWSIncomingMessageReadObserver : NSObject - -- (instancetype)init NS_UNAVAILABLE; -- (instancetype)initWithMessageSender:(OWSMessageSender *)messageSender NS_DESIGNATED_INITIALIZER; - -- (void)startObserving; - -@end - -NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.m b/SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.m deleted file mode 100644 index 81c21de35..000000000 --- a/SignalServiceKit/src/Messages/OWSIncomingMessageReadObserver.m +++ /dev/null @@ -1,79 +0,0 @@ -// -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. -// - -#import "OWSIncomingMessageReadObserver.h" -#import "NSDate+millisecondTimeStamp.h" -#import "OWSSendReadReceiptsJob.h" -#import "TSIncomingMessage.h" - -NS_ASSUME_NONNULL_BEGIN - -@interface OWSIncomingMessageReadObserver () - -@property (nonatomic) BOOL isObserving; -@property (nonatomic, readonly) OWSSendReadReceiptsJob *sendReadReceiptsJob; - -@end - -@implementation OWSIncomingMessageReadObserver - -- (void)dealloc -{ - [[NSNotificationCenter defaultCenter] removeObserver:self]; -} - -- (instancetype)initWithMessageSender:(OWSMessageSender *)messageSender -{ - self = [super init]; - if (!self) { - return self; - } - - _isObserving = NO; - _sendReadReceiptsJob = [[OWSSendReadReceiptsJob alloc] initWithMessageSender:messageSender]; - - return self; -} - -- (void)startObserving -{ - OWSAssert([NSThread isMainThread]); - - if (self.isObserving) { - return; - } - - self.isObserving = true; - [[NSNotificationCenter defaultCenter] addObserver:self - selector:@selector(handleLocalReadNotification:) - name:TSIncomingMessageWasReadOnThisDeviceNotification - object:nil]; -} - -- (void)handleLocalReadNotification:(NSNotification *)notification -{ - if (![notification.object isKindOfClass:[TSIncomingMessage class]]) { - DDLogError(@"%@ Read receipt notifier got unexpected object: %@", self.tag, notification.object); - return; - } - - TSIncomingMessage *message = (TSIncomingMessage *)notification.object; - [self.sendReadReceiptsJob runWith:message]; -} - -#pragma mark - Logging - -+ (NSString *)tag -{ - return [NSString stringWithFormat:@"[%@]", self.class]; -} - -- (NSString *)tag -{ - return self.class.tag; -} - -@end - -NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 1695fcfd6..53b71953e 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -949,6 +949,25 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } } + NSString *localNumber = [TSAccountManager localNumber]; + if ([localNumber isEqualToString:recipient.uniqueId]) { + if (deviceMessages.count < 1) { + DDLogInfo(@"Ignoring sync message without linked devices: %@", [message class]); + OWSAssert([message isKindOfClass:[OWSOutgoingSyncMessage class]]); + + dispatch_async([OWSDispatch sendingQueue], ^{ + [recipient save]; + [self handleMessageSentLocally:message]; + successHandler(); + }); + + return; + } + } else { + OWSAssert(deviceMessages.count > 0); + } + + TSSubmitMessageRequest *request = [[TSSubmitMessageRequest alloc] initWithRecipient:recipient.uniqueId messages:deviceMessages relay:recipient.relay diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager.h b/SignalServiceKit/src/Messages/OWSReadReceiptManager.h new file mode 100644 index 000000000..8a3f2729b --- /dev/null +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.h @@ -0,0 +1,43 @@ +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// + +NS_ASSUME_NONNULL_BEGIN + +@class TSIncomingMessage; + +// There are four kinds of read receipts: +// +// * Read receipts that this client sends to linked +// devices to inform them that a message has been read. +// * Read receipts that this client receives from linked +// devices that inform this client that a message has been read. +// * These read receipts are saved so that they can be applied +// if they arrive before the corresponding message. +// * Read receipts that this client sends to other users +// to inform them that a message has been read. +// * Read receipts that this client receives from other users +// that inform this client that a message has been read. +// * These read receipts are saved so that they can be applied +// if they arrive before the corresponding message. +// +// TODO: Merge OWSReadReceiptsProcessor into this class. +@interface OWSReadReceiptManager : NSObject + +- (instancetype)init NS_UNAVAILABLE; ++ (instancetype)sharedManager; + +// This method can be called from any thread. +// +// It cues this manager: +// +// * ...to inform the sender that this message was read (if read receipts +// are enabled). +// * ...to inform the local user's other devices that this message was read. +// +// Both types of messages are deduplicated. +- (void)messageWasReadLocally:(TSIncomingMessage *)message; + +@end + +NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m new file mode 100644 index 000000000..3533a4bca --- /dev/null +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m @@ -0,0 +1,191 @@ +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// + +#import "OWSReadReceiptManager.h" +#import "OWSMessageSender.h" +#import "OWSReadReceipt.h" +#import "OWSReadReceiptsMessage.h" +#import "TSContactThread.h" +#import "TSDatabaseView.h" +#import "TSIncomingMessage.h" +#import "TextSecureKitEnv.h" +#import "Threading.h" + +NS_ASSUME_NONNULL_BEGIN + +@interface OWSReadReceiptManager () + +@property (nonatomic, readonly) TSStorageManager *storageManager; +@property (nonatomic, readonly) OWSMessageSender *messageSender; + +// A map of "thread unique id"-to-"read receipt" for read receipts that +// we will send to our linked devices. +// +// Should only be accessed while synchronized on the OWSReadReceiptManager. +@property (nonatomic, readonly) NSMutableDictionary *toLinkedDevicesReadReceiptMap; + +// Should only be accessed while synchronized on the OWSReadReceiptManager. +@property (nonatomic) BOOL isProcessing; + +@end + +#pragma mark - + +@implementation OWSReadReceiptManager + ++ (instancetype)sharedManager +{ + static OWSReadReceiptManager *sharedMyManager = nil; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + sharedMyManager = [[self alloc] initDefault]; + }); + return sharedMyManager; +} + +- (instancetype)initDefault +{ + OWSMessageSender *messageSender = [TextSecureKitEnv sharedEnv].messageSender; + + return [self initWithMessageSender:messageSender]; +} + +- (instancetype)initWithMessageSender:(OWSMessageSender *)messageSender +{ + self = [super init]; + + if (!self) { + return self; + } + + _messageSender = messageSender; + + _toLinkedDevicesReadReceiptMap = [NSMutableDictionary new]; + + OWSSingletonAssert(); + + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(databaseViewRegistrationComplete) + name:kNSNotificationName_DatabaseViewRegistrationComplete + object:nil]; + + return self; +} + +- (void)dealloc +{ + [[NSNotificationCenter defaultCenter] removeObserver:self]; +} + +- (void)databaseViewRegistrationComplete +{ + [self scheduleProcessing]; +} + +// Schedules a processing pass, unless one is already scheduled. +- (void)scheduleProcessing +{ + DispatchMainThreadSafe(^{ + @synchronized(self) + { + if ([TSDatabaseView hasPendingViewRegistrations]) { + return; + } + if (self.isProcessing) { + return; + } + + self.isProcessing = YES; + + // Process read receipts every N seconds. + // + // We want a value high enough to allow us to effectively deduplicate, + // read receipts without being so high that we risk not sending read + // receipts due to app exit. + const CGFloat kProcessingFrequencySeconds = 3.f; + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(kProcessingFrequencySeconds * NSEC_PER_SEC)), + dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + [self process]; + }); + } + }); +} + +- (void)process +{ + @synchronized(self) + { + self.isProcessing = NO; + + NSArray *readReceiptsToSend = [[self.toLinkedDevicesReadReceiptMap allValues] copy]; + [self.toLinkedDevicesReadReceiptMap removeAllObjects]; + if (readReceiptsToSend.count > 0) { + OWSReadReceiptsMessage *message = [[OWSReadReceiptsMessage alloc] initWithReadReceipts:readReceiptsToSend]; + + dispatch_async(dispatch_get_main_queue(), ^{ + [self.messageSender sendMessage:message + success:^{ + DDLogInfo(@"%@ Successfully sent %zd read receipt to linked devices.", + self.tag, + readReceiptsToSend.count); + } + failure:^(NSError *error) { + DDLogError(@"%@ Failed to send read receipt to linked devices with error: %@", self.tag, error); + }]; + }); + } + } +} + +- (void)messageWasReadLocally:(TSIncomingMessage *)message; +{ + @synchronized(self) + { + NSString *threadUniqueId = message.thread.uniqueId; + OWSAssert(threadUniqueId.length > 0); + + // Only groupthread sets authorId, thus this crappy code. + // TODO Refactor so that ALL incoming messages have an authorId. + NSString *messageAuthorId; + if (message.authorId) { + // Group Thread + messageAuthorId = message.authorId; + } else { + // Contact Thread + messageAuthorId = [TSContactThread contactIdFromThreadId:message.uniqueThreadId]; + } + OWSAssert(messageAuthorId.length > 0); + + OWSReadReceipt *newReadReceipt = + [[OWSReadReceipt alloc] initWithSenderId:messageAuthorId timestamp:message.timestamp]; + + OWSReadReceipt *_Nullable oldReadReceipt = self.toLinkedDevicesReadReceiptMap[threadUniqueId]; + if (oldReadReceipt && oldReadReceipt.timestamp > newReadReceipt.timestamp) { + // If there's an existing read receipt for the same thread with + // a newer timestamp, discard the new read receipt. + return; + } + + self.toLinkedDevicesReadReceiptMap[threadUniqueId] = newReadReceipt; + + [self scheduleProcessing]; + } +} + +#pragma mark - Logging + ++ (NSString *)tag +{ + return [NSString stringWithFormat:@"[%@]", self.class]; +} + +- (NSString *)tag +{ + return self.class.tag; +} + +@end + +NS_ASSUME_NONNULL_END From 1c8dbcd223d421d19978114de4c7d3d0ee86118f Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 21 Sep 2017 16:58:07 -0400 Subject: [PATCH 2/2] Respond to CR. // FREEBIE --- Signal/src/AppDelegate.m | 3 ++ SignalServiceKit/Utilities/precommit.py | 26 +++++++++++++++- .../src/Devices/OWSReadReceiptsProcessor.m | 10 ++---- .../Messages/Interactions/TSIncomingMessage.h | 4 +-- .../Messages/Interactions/TSIncomingMessage.m | 31 ++++++++++++------- .../src/Messages/OWSDisappearingMessagesJob.m | 2 +- .../src/Messages/OWSMessageSender.m | 19 ------------ .../src/Messages/OWSReadReceiptManager.m | 20 +++++------- 8 files changed, 61 insertions(+), 54 deletions(-) diff --git a/Signal/src/AppDelegate.m b/Signal/src/AppDelegate.m index ba4b0f0bc..0b71dad4a 100644 --- a/Signal/src/AppDelegate.m +++ b/Signal/src/AppDelegate.m @@ -30,6 +30,7 @@ #import #import #import +#import #import #import #import @@ -809,6 +810,8 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; [[OWSBatchMessageProcessor sharedInstance] handleAnyUnprocessedEnvelopesAsync]; [OWSProfileManager.sharedManager fetchLocalUsersProfile]; + // Make sure this manager is started. + [OWSReadReceiptManager sharedManager]; } - (void)registrationStateDidChange diff --git a/SignalServiceKit/Utilities/precommit.py b/SignalServiceKit/Utilities/precommit.py index b1cc9640e..73f642bb4 100755 --- a/SignalServiceKit/Utilities/precommit.py +++ b/SignalServiceKit/Utilities/precommit.py @@ -114,13 +114,35 @@ def sort_include_block(text, filepath, filename, file_extension): for include in includes: include.isInclude = False + # Make sure matching header is first. + matching_header_includes = [] + other_includes = [] + def is_matching_header(include): + filename_wo_ext = os.path.splitext(filename)[0] + include_filename_wo_ext = os.path.splitext(os.path.basename(include.body))[0] + return filename_wo_ext == include_filename_wo_ext + for include in includes: + if is_matching_header(include): + matching_header_includes.append(include) + else: + other_includes.append(include) + includes = other_includes def formatBlock(includes): lines = [include.format() for include in includes] lines = list(set(lines)) def include_sorter(a, b): - return cmp(a.lower(), b.lower()) + # return cmp(a.lower(), b.lower()) + return cmp(a, b) + # print 'before' + # for line in lines: + # print '\t', line + # print lines.sort(include_sorter) + # print 'after' + # for line in lines: + # print '\t', line + # print # print # print 'filepath' # for line in lines: @@ -132,6 +154,8 @@ def sort_include_block(text, filepath, filename, file_extension): includeQuotes = [include for include in includes if include.isInclude and include.isQuote] importAngles = [include for include in includes if (not include.isInclude) and not include.isQuote] importQuotes = [include for include in includes if (not include.isInclude) and include.isQuote] + if matching_header_includes: + blocks.append(formatBlock(matching_header_includes)) if includeQuotes: blocks.append(formatBlock(includeQuotes)) if includeAngles: diff --git a/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.m b/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.m index ff86c4598..4088c10bf 100644 --- a/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.m +++ b/SignalServiceKit/src/Devices/OWSReadReceiptsProcessor.m @@ -59,14 +59,8 @@ NSString *const OWSReadReceiptsProcessorMarkedMessageAsReadNotification = - (instancetype)initWithIncomingMessage:(TSIncomingMessage *)message storageManager:(TSStorageManager *)storageManager { - // authorId isn't set on all legacy messages, so we take - // extra measures to ensure we obtain a valid value. - NSString *messageAuthorId; - if (message.authorId) { // Group Thread - messageAuthorId = message.authorId; - } else { // Contact Thread - messageAuthorId = [TSContactThread contactIdFromThreadId:message.uniqueThreadId]; - } + NSString *messageAuthorId = message.messageAuthorId; + OWSAssert(messageAuthorId.length > 0); OWSReadReceipt *readReceipt = [OWSReadReceipt firstWithSenderId:messageAuthorId timestamp:message.timestamp]; if (readReceipt) { diff --git a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h index 1f0098d31..2eb3d4050 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.h @@ -103,11 +103,11 @@ NS_ASSUME_NONNULL_BEGIN timestamp:(uint64_t)timestamp transaction:(YapDatabaseReadWriteTransaction *)transaction; -@property (nonatomic, readonly) NSString *authorId; - // This will be 0 for messages created before we were tracking sourceDeviceId @property (nonatomic, readonly) UInt32 sourceDeviceId; +- (NSString *)messageAuthorId; + @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m index f8585d074..45225acb5 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSIncomingMessage.m @@ -15,6 +15,8 @@ NS_ASSUME_NONNULL_BEGIN @interface TSIncomingMessage () +@property (nonatomic, readonly) NSString *authorId; + @property (nonatomic, getter=wasRead) BOOL read; @end @@ -91,15 +93,8 @@ NS_ASSUME_NONNULL_BEGIN if ([interaction isKindOfClass:[TSIncomingMessage class]]) { TSIncomingMessage *message = (TSIncomingMessage *)interaction; - // authorId isn't set on all legacy messages, so we take - // extra measures to ensure we obtain a valid value. - NSString *messageAuthorId; - if (message.authorId) { // Group Thread - messageAuthorId = message.authorId; - } else { // Contact Thread - messageAuthorId = - [TSContactThread contactIdFromThreadId:message.uniqueThreadId]; - } + NSString *messageAuthorId = message.messageAuthorId; + OWSAssert(messageAuthorId.length > 0); if ([messageAuthorId isEqualToString:authorId]) { foundMessage = message; @@ -111,6 +106,22 @@ NS_ASSUME_NONNULL_BEGIN return foundMessage; } +- (NSString *)messageAuthorId +{ + // authorId isn't set on all legacy messages, so we take + // extra measures to ensure we obtain a valid value. + NSString *messageAuthorId; + if (self.authorId) { + // Group Thread + messageAuthorId = self.authorId; + } else { + // Contact Thread + messageAuthorId = [TSContactThread contactIdFromThreadId:self.uniqueThreadId]; + } + OWSAssert(messageAuthorId.length > 0); + return messageAuthorId; +} + #pragma mark - OWSReadTracking - (BOOL)shouldAffectUnreadCounts @@ -138,8 +149,6 @@ NS_ASSUME_NONNULL_BEGIN } if (sendReadReceipt) { - // Notification must happen outside of the transaction, else we'll likely crash when the notification receiver - // tries to do anything with the DB. [OWSReadReceiptManager.sharedManager messageWasReadLocally:self]; } } diff --git a/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m b/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m index c7ef8beb4..6f57812cf 100644 --- a/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m +++ b/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m @@ -264,7 +264,7 @@ NS_ASSUME_NONNULL_BEGIN if ([message isKindOfClass:[TSIncomingMessage class]]) { TSIncomingMessage *incomingMessage = (TSIncomingMessage *)message; - NSString *contactName = [contactsManager displayNameForPhoneIdentifier:incomingMessage.authorId]; + NSString *contactName = [contactsManager displayNameForPhoneIdentifier:incomingMessage.messageAuthorId]; [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp thread:message.thread diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 53b71953e..1695fcfd6 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -949,25 +949,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } } - NSString *localNumber = [TSAccountManager localNumber]; - if ([localNumber isEqualToString:recipient.uniqueId]) { - if (deviceMessages.count < 1) { - DDLogInfo(@"Ignoring sync message without linked devices: %@", [message class]); - OWSAssert([message isKindOfClass:[OWSOutgoingSyncMessage class]]); - - dispatch_async([OWSDispatch sendingQueue], ^{ - [recipient save]; - [self handleMessageSentLocally:message]; - successHandler(); - }); - - return; - } - } else { - OWSAssert(deviceMessages.count > 0); - } - - TSSubmitMessageRequest *request = [[TSSubmitMessageRequest alloc] initWithRecipient:recipient.uniqueId messages:deviceMessages relay:recipient.relay diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m index 3533a4bca..94b61392b 100644 --- a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m @@ -70,6 +70,11 @@ NS_ASSUME_NONNULL_BEGIN name:kNSNotificationName_DatabaseViewRegistrationComplete object:nil]; + // Try to start processing. + dispatch_async(dispatch_get_main_queue(), ^{ + [self scheduleProcessing]; + }); + return self; } @@ -119,7 +124,7 @@ NS_ASSUME_NONNULL_BEGIN { self.isProcessing = NO; - NSArray *readReceiptsToSend = [[self.toLinkedDevicesReadReceiptMap allValues] copy]; + NSArray *readReceiptsToSend = [self.toLinkedDevicesReadReceiptMap allValues]; [self.toLinkedDevicesReadReceiptMap removeAllObjects]; if (readReceiptsToSend.count > 0) { OWSReadReceiptsMessage *message = [[OWSReadReceiptsMessage alloc] initWithReadReceipts:readReceiptsToSend]; @@ -143,19 +148,10 @@ NS_ASSUME_NONNULL_BEGIN { @synchronized(self) { - NSString *threadUniqueId = message.thread.uniqueId; + NSString *threadUniqueId = message.uniqueThreadId; OWSAssert(threadUniqueId.length > 0); - // Only groupthread sets authorId, thus this crappy code. - // TODO Refactor so that ALL incoming messages have an authorId. - NSString *messageAuthorId; - if (message.authorId) { - // Group Thread - messageAuthorId = message.authorId; - } else { - // Contact Thread - messageAuthorId = [TSContactThread contactIdFromThreadId:message.uniqueThreadId]; - } + NSString *messageAuthorId = message.messageAuthorId; OWSAssert(messageAuthorId.length > 0); OWSReadReceipt *newReadReceipt =