From 8c1dfe7ee60e72b3ecdd3531c2e63a928922bf0e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 23 Oct 2017 15:37:37 -0400 Subject: [PATCH 1/5] Only one system contacts fetch at a time. // FREEBIE --- .../src/contact/SystemContactsFetcher.swift | 42 ++++++++++++------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 947ed8611..9d6a4b343 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -344,6 +344,8 @@ class SystemContactsFetcher: NSObject { private var systemContactsHaveBeenRequestedAtLeastOnce = false private var hasSetupObservation = false + private var isFetchingContacts = false + private static let serialQueue = DispatchQueue(label: "org.whispersystems.system.contacts") override init() { self.contactStoreAdapter = ContactStoreAdapter() @@ -432,20 +434,32 @@ class SystemContactsFetcher: NSObject { systemContactsHaveBeenRequestedAtLeastOnce = true setupObservationIfNecessary() - DispatchQueue.global().async { + SystemContactsFetcher.serialQueue.async { [weak self] _ in + guard let strongSelf = self else { + return + } var fetchedContacts: [Contact]? - switch self.contactStoreAdapter.fetchContacts() { + guard !strongSelf.isFetchingContacts else { + Logger.info("\(strongSelf.TAG) ignoring redundant system contacts fetch.") + return + } + strongSelf.isFetchingContacts = true + + switch strongSelf.contactStoreAdapter.fetchContacts() { case .success(let result): fetchedContacts = result case .error(let error): completion?(error) + strongSelf.isFetchingContacts = false return } + strongSelf.isFetchingContacts = false + guard let contacts = fetchedContacts else { - owsFail("\(self.TAG) contacts was unexpectedly not set.") + owsFail("\(strongSelf.TAG) contacts was unexpectedly not set.") completion?(nil) } @@ -454,43 +468,43 @@ class SystemContactsFetcher: NSObject { DispatchQueue.main.async { var shouldNotifyDelegate = false - if self.lastContactUpdateHash != contactsHash { - Logger.info("\(self.TAG) contact hash changed. new contactsHash: \(contactsHash)") + if strongSelf.lastContactUpdateHash != contactsHash { + Logger.info("\(strongSelf.TAG) contact hash changed. new contactsHash: \(contactsHash)") shouldNotifyDelegate = true } else if ignoreDebounce { - Logger.info("\(self.TAG) ignoring debounce.") + Logger.info("\(strongSelf.TAG) ignoring debounce.") shouldNotifyDelegate = true } else { // If nothing has changed, only notify delegate (to perform contact intersection) every N hours - if let lastDelegateNotificationDate = self.lastDelegateNotificationDate { + if let lastDelegateNotificationDate = strongSelf.lastDelegateNotificationDate { let kDebounceInterval = TimeInterval(12 * 60 * 60) let expiresAtDate = Date(timeInterval: kDebounceInterval, since:lastDelegateNotificationDate) if Date() > expiresAtDate { - Logger.info("\(self.TAG) debounce interval expired at: \(expiresAtDate)") + Logger.info("\(strongSelf.TAG) debounce interval expired at: \(expiresAtDate)") shouldNotifyDelegate = true } else { - Logger.info("\(self.TAG) ignoring since debounce interval hasn't expired") + Logger.info("\(strongSelf.TAG) ignoring since debounce interval hasn't expired") } } else { - Logger.info("\(self.TAG) first contact fetch. contactsHash: \(contactsHash)") + Logger.info("\(strongSelf.TAG) first contact fetch. contactsHash: \(contactsHash)") shouldNotifyDelegate = true } } guard shouldNotifyDelegate else { - Logger.info("\(self.TAG) no reason to notify delegate.") + Logger.info("\(strongSelf.TAG) no reason to notify delegate.") completion?(nil) return } - self.lastDelegateNotificationDate = Date() - self.lastContactUpdateHash = contactsHash + strongSelf.lastDelegateNotificationDate = Date() + strongSelf.lastContactUpdateHash = contactsHash - self.delegate?.systemContactsFetcher(self, updatedContacts: contacts) + strongSelf.delegate?.systemContactsFetcher(strongSelf, updatedContacts: contacts) completion?(nil) } } From 878fd3d8425cdfa2591bb333180678976dee6e23 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 23 Oct 2017 15:50:44 -0400 Subject: [PATCH 2/5] Only one system contacts fetch at a time. // FREEBIE --- .../src/contact/SystemContactsFetcher.swift | 59 +++++++++++-------- 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 9d6a4b343..9b80df975 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -428,38 +428,51 @@ class SystemContactsFetcher: NSObject { updateContacts(completion: nil, ignoreDebounce:ignoreDebounce) } + private func tryToAcquireContactFetchLock() -> Bool { + var didAcquireLock = false + SystemContactsFetcher.serialQueue.sync { + guard !self.isFetchingContacts else { + return + } + self.isFetchingContacts = true + didAcquireLock = true + } + return didAcquireLock + } + + private func releaseContactFetchLock() { + SystemContactsFetcher.serialQueue.sync { + self.isFetchingContacts = false + } + } + private func updateContacts(completion: ((Error?) -> Void)?, ignoreDebounce: Bool = false) { AssertIsOnMainThread() systemContactsHaveBeenRequestedAtLeastOnce = true setupObservationIfNecessary() - SystemContactsFetcher.serialQueue.async { [weak self] _ in - guard let strongSelf = self else { - return - } + DispatchQueue.global().async { var fetchedContacts: [Contact]? - guard !strongSelf.isFetchingContacts else { - Logger.info("\(strongSelf.TAG) ignoring redundant system contacts fetch.") + guard self.tryToAcquireContactFetchLock() else { + Logger.info("\(self.TAG) ignoring redundant system contacts fetch.") return } - strongSelf.isFetchingContacts = true - switch strongSelf.contactStoreAdapter.fetchContacts() { + switch self.contactStoreAdapter.fetchContacts() { case .success(let result): fetchedContacts = result case .error(let error): completion?(error) - strongSelf.isFetchingContacts = false + self.releaseContactFetchLock() return } - - strongSelf.isFetchingContacts = false + self.releaseContactFetchLock() guard let contacts = fetchedContacts else { - owsFail("\(strongSelf.TAG) contacts was unexpectedly not set.") + owsFail("\(self.TAG) contacts was unexpectedly not set.") completion?(nil) } @@ -468,43 +481,43 @@ class SystemContactsFetcher: NSObject { DispatchQueue.main.async { var shouldNotifyDelegate = false - if strongSelf.lastContactUpdateHash != contactsHash { - Logger.info("\(strongSelf.TAG) contact hash changed. new contactsHash: \(contactsHash)") + if self.lastContactUpdateHash != contactsHash { + Logger.info("\(self.TAG) contact hash changed. new contactsHash: \(contactsHash)") shouldNotifyDelegate = true } else if ignoreDebounce { - Logger.info("\(strongSelf.TAG) ignoring debounce.") + Logger.info("\(self.TAG) ignoring debounce.") shouldNotifyDelegate = true } else { // If nothing has changed, only notify delegate (to perform contact intersection) every N hours - if let lastDelegateNotificationDate = strongSelf.lastDelegateNotificationDate { + if let lastDelegateNotificationDate = self.lastDelegateNotificationDate { let kDebounceInterval = TimeInterval(12 * 60 * 60) let expiresAtDate = Date(timeInterval: kDebounceInterval, since:lastDelegateNotificationDate) if Date() > expiresAtDate { - Logger.info("\(strongSelf.TAG) debounce interval expired at: \(expiresAtDate)") + Logger.info("\(self.TAG) debounce interval expired at: \(expiresAtDate)") shouldNotifyDelegate = true } else { - Logger.info("\(strongSelf.TAG) ignoring since debounce interval hasn't expired") + Logger.info("\(self.TAG) ignoring since debounce interval hasn't expired") } } else { - Logger.info("\(strongSelf.TAG) first contact fetch. contactsHash: \(contactsHash)") + Logger.info("\(self.TAG) first contact fetch. contactsHash: \(contactsHash)") shouldNotifyDelegate = true } } guard shouldNotifyDelegate else { - Logger.info("\(strongSelf.TAG) no reason to notify delegate.") + Logger.info("\(self.TAG) no reason to notify delegate.") completion?(nil) return } - strongSelf.lastDelegateNotificationDate = Date() - strongSelf.lastContactUpdateHash = contactsHash + self.lastDelegateNotificationDate = Date() + self.lastContactUpdateHash = contactsHash - strongSelf.delegate?.systemContactsFetcher(strongSelf, updatedContacts: contacts) + self.delegate?.systemContactsFetcher(self, updatedContacts: contacts) completion?(nil) } } From d1141581de9455ffd1e1d002782c81d7798ff905 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 23 Oct 2017 15:56:06 -0400 Subject: [PATCH 3/5] Only one system contacts fetch at a time. // FREEBIE --- Signal/src/contact/SystemContactsFetcher.swift | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 9b80df975..00664d475 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -345,7 +345,6 @@ class SystemContactsFetcher: NSObject { private var systemContactsHaveBeenRequestedAtLeastOnce = false private var hasSetupObservation = false private var isFetchingContacts = false - private static let serialQueue = DispatchQueue(label: "org.whispersystems.system.contacts") override init() { self.contactStoreAdapter = ContactStoreAdapter() @@ -430,20 +429,19 @@ class SystemContactsFetcher: NSObject { private func tryToAcquireContactFetchLock() -> Bool { var didAcquireLock = false - SystemContactsFetcher.serialQueue.sync { - guard !self.isFetchingContacts else { - return - } + objc_sync_enter(self) + if !self.isFetchingContacts { self.isFetchingContacts = true didAcquireLock = true } + objc_sync_exit(self) return didAcquireLock } private func releaseContactFetchLock() { - SystemContactsFetcher.serialQueue.sync { - self.isFetchingContacts = false - } + objc_sync_enter(self) + self.isFetchingContacts = false + objc_sync_exit(self) } private func updateContacts(completion: ((Error?) -> Void)?, ignoreDebounce: Bool = false) { @@ -454,13 +452,12 @@ class SystemContactsFetcher: NSObject { DispatchQueue.global().async { - var fetchedContacts: [Contact]? - guard self.tryToAcquireContactFetchLock() else { Logger.info("\(self.TAG) ignoring redundant system contacts fetch.") return } + var fetchedContacts: [Contact]? switch self.contactStoreAdapter.fetchContacts() { case .success(let result): fetchedContacts = result From 1b3b5fc9e54cc2cfdfaf20b31de6e36c0350701f Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 24 Oct 2017 10:53:15 -0400 Subject: [PATCH 4/5] Respond to CR. // FREEBIE --- .../NewContactThreadViewController.m | 2 +- Signal/src/contact/OWSContactsManager.h | 2 +- Signal/src/contact/OWSContactsManager.m | 7 ++--- .../src/contact/SystemContactsFetcher.swift | 28 ++++++++++++------- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/Signal/src/ViewControllers/NewContactThreadViewController.m b/Signal/src/ViewControllers/NewContactThreadViewController.m index db580df2f..556a55945 100644 --- a/Signal/src/ViewControllers/NewContactThreadViewController.m +++ b/Signal/src/ViewControllers/NewContactThreadViewController.m @@ -146,7 +146,7 @@ NS_ASSUME_NONNULL_BEGIN { OWSAssert([NSThread isMainThread]); - [self.contactsViewHelper.contactsManager fetchSystemContactsIfAlreadyAuthorizedAndIgnoreDebounce]; + [self.contactsViewHelper.contactsManager fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify]; [refreshControl endRefreshing]; } diff --git a/Signal/src/contact/OWSContactsManager.h b/Signal/src/contact/OWSContactsManager.h index 76e4fa4a8..10b965175 100644 --- a/Signal/src/contact/OWSContactsManager.h +++ b/Signal/src/contact/OWSContactsManager.h @@ -72,7 +72,7 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; // Ensure's the app has the latest contacts, but won't prompt the user for contact // access if they haven't granted it. - (void)fetchSystemContactsIfAlreadyAuthorized; -- (void)fetchSystemContactsIfAlreadyAuthorizedAndIgnoreDebounce; +- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify; #pragma mark - Util diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 0e3b279a7..230951820 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -93,15 +93,14 @@ NSString *const kTSStorageManager_lastKnownContactRecipientIds = @"lastKnownCont [self.systemContactsFetcher requestOnceWithCompletion:completion]; } - - (void)fetchSystemContactsIfAlreadyAuthorized { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedWithIgnoreDebounce:NO]; + [self.systemContactsFetcher fetchIfAlreadyAuthorizedWithAlwaysNotify:NO]; } -- (void)fetchSystemContactsIfAlreadyAuthorizedAndIgnoreDebounce +- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedWithIgnoreDebounce:YES]; + [self.systemContactsFetcher fetchIfAlreadyAuthorizedWithAlwaysNotify:YES]; } - (BOOL)isSystemContactsAuthorized diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 00664d475..ba5d0b847 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -362,7 +362,8 @@ class SystemContactsFetcher: NSObject { hasSetupObservation = true self.contactStoreAdapter.startObservingChanges { [weak self] in DispatchQueue.main.async { - self?.updateContacts(completion: nil) + // If contacts have changed, don't de-bounce. + self?.updateContacts(completion: nil, alwaysNotify: false, shouldDebounce: false) } } } @@ -418,13 +419,13 @@ class SystemContactsFetcher: NSObject { } } - public func fetchIfAlreadyAuthorized(ignoreDebounce: Bool = false) { + public func fetchIfAlreadyAuthorized(alwaysNotify: Bool = false) { AssertIsOnMainThread() guard authorizationStatus == .authorized else { return } - updateContacts(completion: nil, ignoreDebounce:ignoreDebounce) + updateContacts(completion: nil, alwaysNotify:alwaysNotify) } private func tryToAcquireContactFetchLock() -> Bool { @@ -444,7 +445,7 @@ class SystemContactsFetcher: NSObject { objc_sync_exit(self) } - private func updateContacts(completion: ((Error?) -> Void)?, ignoreDebounce: Bool = false) { + private func updateContacts(completion: ((Error?) -> Void)?, alwaysNotify: Bool = false, shouldDebounce: Bool = true) { AssertIsOnMainThread() systemContactsHaveBeenRequestedAtLeastOnce = true @@ -452,10 +453,19 @@ class SystemContactsFetcher: NSObject { DispatchQueue.global().async { - guard self.tryToAcquireContactFetchLock() else { - Logger.info("\(self.TAG) ignoring redundant system contacts fetch.") - return + if shouldDebounce { + guard self.tryToAcquireContactFetchLock() else { + Logger.info("\(self.TAG) ignoring redundant system contacts fetch.") + return + } } + defer { + if shouldDebounce { + self.releaseContactFetchLock() + } + } + + Logger.info("\(self.TAG) fetching contacts") var fetchedContacts: [Contact]? switch self.contactStoreAdapter.fetchContacts() { @@ -463,10 +473,8 @@ class SystemContactsFetcher: NSObject { fetchedContacts = result case .error(let error): completion?(error) - self.releaseContactFetchLock() return } - self.releaseContactFetchLock() guard let contacts = fetchedContacts else { owsFail("\(self.TAG) contacts was unexpectedly not set.") @@ -481,7 +489,7 @@ class SystemContactsFetcher: NSObject { if self.lastContactUpdateHash != contactsHash { Logger.info("\(self.TAG) contact hash changed. new contactsHash: \(contactsHash)") shouldNotifyDelegate = true - } else if ignoreDebounce { + } else if alwaysNotify { Logger.info("\(self.TAG) ignoring debounce.") shouldNotifyDelegate = true } else { From 5af6b6f213c64306a9dda64dda70d8bf478244ca Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 25 Oct 2017 09:18:27 -0400 Subject: [PATCH 5/5] Respond to CR. // FREEBIE --- Signal/src/AppDelegate.m | 2 +- Signal/src/contact/OWSContactsManager.h | 4 +- Signal/src/contact/OWSContactsManager.m | 6 +-- .../src/contact/SystemContactsFetcher.swift | 44 +++++-------------- 4 files changed, 19 insertions(+), 37 deletions(-) diff --git a/Signal/src/AppDelegate.m b/Signal/src/AppDelegate.m index bc8348e08..9be20ed8a 100644 --- a/Signal/src/AppDelegate.m +++ b/Signal/src/AppDelegate.m @@ -523,7 +523,7 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; // Avoid blocking app launch by putting all further possible DB access in async block dispatch_async(dispatch_get_main_queue(), ^{ [TSSocketManager requestSocketOpen]; - [[Environment getCurrent].contactsManager fetchSystemContactsIfAlreadyAuthorized]; + [[Environment getCurrent].contactsManager fetchSystemContactsOnceIfAlreadyAuthorized]; // This will fetch new messages, if we're using domain fronting. [[PushManager sharedManager] applicationDidBecomeActive]; diff --git a/Signal/src/contact/OWSContactsManager.h b/Signal/src/contact/OWSContactsManager.h index 10b965175..c10f8c2de 100644 --- a/Signal/src/contact/OWSContactsManager.h +++ b/Signal/src/contact/OWSContactsManager.h @@ -71,7 +71,9 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; // Ensure's the app has the latest contacts, but won't prompt the user for contact // access if they haven't granted it. -- (void)fetchSystemContactsIfAlreadyAuthorized; +- (void)fetchSystemContactsOnceIfAlreadyAuthorized; +// This variant will fetch system contacts if contact access has already been granted, +// but not prompt for contact access. Also, it will always fire a notification. - (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify; #pragma mark - Util diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 230951820..3e4c3604a 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -93,14 +93,14 @@ NSString *const kTSStorageManager_lastKnownContactRecipientIds = @"lastKnownCont [self.systemContactsFetcher requestOnceWithCompletion:completion]; } -- (void)fetchSystemContactsIfAlreadyAuthorized +- (void)fetchSystemContactsOnceIfAlreadyAuthorized { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedWithAlwaysNotify:NO]; + [self.systemContactsFetcher fetchOnceIfAlreadyAuthorized]; } - (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedWithAlwaysNotify:YES]; + [self.systemContactsFetcher fetchIfAlreadyAuthorizedAndAlwaysNotify]; } - (BOOL)isSystemContactsAuthorized diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index ba5d0b847..b3d92aa9a 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -344,7 +344,6 @@ class SystemContactsFetcher: NSObject { private var systemContactsHaveBeenRequestedAtLeastOnce = false private var hasSetupObservation = false - private var isFetchingContacts = false override init() { self.contactStoreAdapter = ContactStoreAdapter() @@ -362,8 +361,7 @@ class SystemContactsFetcher: NSObject { hasSetupObservation = true self.contactStoreAdapter.startObservingChanges { [weak self] in DispatchQueue.main.async { - // If contacts have changed, don't de-bounce. - self?.updateContacts(completion: nil, alwaysNotify: false, shouldDebounce: false) + self?.updateContacts(completion: nil, alwaysNotify: false) } } } @@ -382,7 +380,6 @@ class SystemContactsFetcher: NSObject { completion?(nil) return } - systemContactsHaveBeenRequestedAtLeastOnce = true setupObservationIfNecessary() switch authorizationStatus { @@ -419,33 +416,28 @@ class SystemContactsFetcher: NSObject { } } - public func fetchIfAlreadyAuthorized(alwaysNotify: Bool = false) { + public func fetchOnceIfAlreadyAuthorized() { AssertIsOnMainThread() guard authorizationStatus == .authorized else { return } + guard !systemContactsHaveBeenRequestedAtLeastOnce else { + return + } - updateContacts(completion: nil, alwaysNotify:alwaysNotify) + updateContacts(completion: nil, alwaysNotify:false) } - private func tryToAcquireContactFetchLock() -> Bool { - var didAcquireLock = false - objc_sync_enter(self) - if !self.isFetchingContacts { - self.isFetchingContacts = true - didAcquireLock = true + public func fetchIfAlreadyAuthorizedAndAlwaysNotify() { + AssertIsOnMainThread() + guard authorizationStatus == .authorized else { + return } - objc_sync_exit(self) - return didAcquireLock - } - private func releaseContactFetchLock() { - objc_sync_enter(self) - self.isFetchingContacts = false - objc_sync_exit(self) + updateContacts(completion: nil, alwaysNotify:true) } - private func updateContacts(completion: ((Error?) -> Void)?, alwaysNotify: Bool = false, shouldDebounce: Bool = true) { + private func updateContacts(completion: ((Error?) -> Void)?, alwaysNotify: Bool = false) { AssertIsOnMainThread() systemContactsHaveBeenRequestedAtLeastOnce = true @@ -453,18 +445,6 @@ class SystemContactsFetcher: NSObject { DispatchQueue.global().async { - if shouldDebounce { - guard self.tryToAcquireContactFetchLock() else { - Logger.info("\(self.TAG) ignoring redundant system contacts fetch.") - return - } - } - defer { - if shouldDebounce { - self.releaseContactFetchLock() - } - } - Logger.info("\(self.TAG) fetching contacts") var fetchedContacts: [Contact]?