From fc45ec8f635c56a762fcfd022e55f5ced776c009 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 24 Sep 2021 15:07:23 -0300 Subject: [PATCH] Refine de-duplication logic. --- .../NotificationService.swift | 17 ++++++++++++----- .../DebugUI/DebugUIMessages.swift | 6 ++++-- .../src/Messages/MessageFetcherJob.swift | 18 ++++++++++++++++-- .../src/Messages/MessageProcessor.swift | 7 ++++--- .../src/Network/WebSockets/OWSWebSocket.swift | 6 ++++-- .../MessageProcessingIntegrationTest.swift | 12 ++++++++---- 6 files changed, 48 insertions(+), 18 deletions(-) diff --git a/NotificationServiceExtension/NotificationService.swift b/NotificationServiceExtension/NotificationService.swift index d21eb12813..5d7fc6d24f 100644 --- a/NotificationServiceExtension/NotificationService.swift +++ b/NotificationServiceExtension/NotificationService.swift @@ -33,10 +33,11 @@ let hasShownFirstUnlockError = AtomicBool(false) class NotificationService: UNNotificationServiceExtension { - private var contentHandler: ((UNNotificationContent) -> Void)? + private typealias ContentHandler = (UNNotificationContent) -> Void + private var contentHandler = AtomicOptional(nil) func completeSilenty(timeHasExpired: Bool = false) { - guard let contentHandler = contentHandler else { + guard let contentHandler = contentHandler.swap(nil) else { if DebugFlags.internalLogging { Logger.info("No contentHandler.") } @@ -57,8 +58,14 @@ class NotificationService: UNNotificationServiceExtension { } Logger.flush() - contentHandler(content) - self.contentHandler = nil + let isSync = timeHasExpired + if isSync { + contentHandler(content) + } else { + DispatchQueue.global().asyncAfter(deadline: .now() + .milliseconds(1000)) { + contentHandler(content) + } + } } override func didReceive(_ request: UNNotificationRequest, withContentHandler contentHandler: @escaping (UNNotificationContent) -> Void) { @@ -94,7 +101,7 @@ class NotificationService: UNNotificationServiceExtension { fatalError("Posting error notification and skipping processing.") } - self.contentHandler = contentHandler + self.contentHandler.set(contentHandler) owsAssertDebug(FeatureFlags.notificationServiceExtension) diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.swift b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.swift index 69431e1b7f..26dc11d200 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.swift +++ b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.swift @@ -34,8 +34,10 @@ public extension DebugUIMessages { serverDeliveryTimestamp: 0, wasReceivedByUD: false) { error in switch error { - case MessageProcessingError.duplicateMessage?: - Logger.warn("Duplicate.") + case MessageProcessingError.duplicatePendingEnvelope?: + Logger.warn("duplicatePendingEnvelope.") + case MessageProcessingError.duplicateDecryption?: + Logger.warn("duplicateDecryption.") case let otherError?: owsFailDebug("Error: \(otherError)") case nil: diff --git a/SignalServiceKit/src/Messages/MessageFetcherJob.swift b/SignalServiceKit/src/Messages/MessageFetcherJob.swift index fce6c51735..a8fa7cb7ed 100644 --- a/SignalServiceKit/src/Messages/MessageFetcherJob.swift +++ b/SignalServiceKit/src/Messages/MessageFetcherJob.swift @@ -295,8 +295,22 @@ public class MessageFetcherJob: NSObject { let envelopeInfo = Self.buildEnvelopeInfo(envelope: envelope) do { let envelopeData = try envelope.serializedData() - return EnvelopeJob(encryptedEnvelopeData: envelopeData, encryptedEnvelope: envelope) {_ in - Self.messageFetcherJob.acknowledgeDelivery(envelopeInfo: envelopeInfo) + return EnvelopeJob(encryptedEnvelopeData: envelopeData, encryptedEnvelope: envelope) { processingError in + var shouldAck: Bool + switch processingError { + case nil: + shouldAck = true + case MessageProcessingError.duplicatePendingEnvelope?: + shouldAck = false + case MessageProcessingError.duplicateDecryption?: + shouldAck = true + default: + shouldAck = true + } + + if shouldAck { + Self.messageFetcherJob.acknowledgeDelivery(envelopeInfo: envelopeInfo) + } } } catch { owsFailDebug("failed to serialize envelope") diff --git a/SignalServiceKit/src/Messages/MessageProcessor.swift b/SignalServiceKit/src/Messages/MessageProcessor.swift index b67e01166b..76aa0c8fb6 100644 --- a/SignalServiceKit/src/Messages/MessageProcessor.swift +++ b/SignalServiceKit/src/Messages/MessageProcessor.swift @@ -193,7 +193,7 @@ public class MessageProcessor: NSObject { switch result { case .duplicate: Logger.warn("Duplicate envelope, envelopeSource: \(envelopeSource).") - completion(MessageProcessingError.duplicateMessage) + completion(MessageProcessingError.duplicatePendingEnvelope) case .enqueued: drainPendingEnvelopes() } @@ -628,7 +628,7 @@ private struct EncryptedEnvelope: PendingEnvelope, Dependencies { // Proceed with decryption. break case .duplicate: - return .failure(MessageProcessingError.duplicateMessage) + return .failure(MessageProcessingError.duplicateDecryption) } let result = Self.messageDecrypter.decryptEnvelope( @@ -803,5 +803,6 @@ public class PendingEnvelopes { // MARK: - public enum MessageProcessingError: Error { - case duplicateMessage + case duplicatePendingEnvelope + case duplicateDecryption } diff --git a/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.swift b/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.swift index 1f648ab84c..906e87c63c 100644 --- a/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.swift +++ b/SignalServiceKit/src/Network/WebSockets/OWSWebSocket.swift @@ -534,10 +534,12 @@ public class OWSWebSocket: NSObject { switch processingError { case nil: shouldAck = true - case MessageProcessingError.duplicateMessage?: + case MessageProcessingError.duplicatePendingEnvelope?: shouldAck = false + case MessageProcessingError.duplicateDecryption?: + shouldAck = true case let alertableError?: - shouldAck = false + shouldAck = true Logger.warn("Failed to process message: \(alertableError)") Self.databaseStorage.write { transaction in let errorMessage = ThreadlessErrorMessage.corruptedMessageInUnknownThread() diff --git a/SignalServiceKit/tests/Messages/MessageProcessingIntegrationTest.swift b/SignalServiceKit/tests/Messages/MessageProcessingIntegrationTest.swift index d9c7bd93ed..561a924030 100644 --- a/SignalServiceKit/tests/Messages/MessageProcessingIntegrationTest.swift +++ b/SignalServiceKit/tests/Messages/MessageProcessingIntegrationTest.swift @@ -105,8 +105,10 @@ class MessageProcessingIntegrationTest: SSKBaseTestSwift { serverDeliveryTimestamp: NSDate.ows_millisecondTimeStamp(), envelopeSource: .tests) { error in switch error { - case MessageProcessingError.duplicateMessage?: - XCTFail("duplicate") + case MessageProcessingError.duplicatePendingEnvelope?: + XCTFail("duplicatePendingEnvelope") + case MessageProcessingError.duplicateDecryption?: + XCTFail("duplicateDecryption") case .some(_): XCTFail("failure") case nil: @@ -177,8 +179,10 @@ class MessageProcessingIntegrationTest: SSKBaseTestSwift { serverDeliveryTimestamp: NSDate.ows_millisecondTimeStamp(), envelopeSource: .tests) { error in switch error { - case MessageProcessingError.duplicateMessage?: - XCTFail("duplicate") + case MessageProcessingError.duplicatePendingEnvelope?: + XCTFail("duplicatePendingEnvelope") + case MessageProcessingError.duplicateDecryption?: + XCTFail("duplicateDecryption") case .some(_): XCTFail("failure") case nil: