From 06968cc05c09f625037dc24ef73683a42ef6ab7f Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 19 Apr 2019 10:03:28 -0400 Subject: [PATCH] Respond to CR. --- Scripts/sds_codegen/sds_generate.py | 2 +- .../src/Contacts/TSThread+SDS.swift | 2 +- .../Attachments/TSAttachment+SDS.swift | 2 +- .../Interactions/TSInteraction+SDS.swift | 2 +- .../OWSBatchMessageProcessor+SDS.swift | 2 +- .../src/Messages/OWSMessageReceiver+SDS.swift | 2 +- .../Messages/OWSReadReceiptManager+SDS.swift | 2 +- .../Stickers/DownloadStickerOperation.swift | 21 +++++++------ .../DownloadStickerPackOperation.swift | 21 +++++++------ .../Stickers/InstalledSticker+SDS.swift | 2 +- .../Stickers/InstalledStickerPack+SDS.swift | 2 +- .../Messages/Stickers/InstalledStickers.swift | 4 +-- SignalServiceKit/src/Util/JobQueue.swift | 9 ++++-- SignalServiceKit/src/Util/OWSOperation.h | 6 ++-- SignalServiceKit/src/Util/OWSOperation.m | 6 +++- .../tests/Storage/SDSKeyValueStoreTest.swift | 30 +++++++++++++++++++ 16 files changed, 77 insertions(+), 38 deletions(-) diff --git a/Scripts/sds_codegen/sds_generate.py b/Scripts/sds_codegen/sds_generate.py index 088d1f5fce..c8463115d5 100755 --- a/Scripts/sds_codegen/sds_generate.py +++ b/Scripts/sds_codegen/sds_generate.py @@ -879,7 +879,7 @@ extension %s { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> %s? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Contacts/TSThread+SDS.swift b/SignalServiceKit/src/Contacts/TSThread+SDS.swift index 7fa61eff25..4ef0bc4a9b 100644 --- a/SignalServiceKit/src/Contacts/TSThread+SDS.swift +++ b/SignalServiceKit/src/Contacts/TSThread+SDS.swift @@ -242,7 +242,7 @@ extension TSThread { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> TSThread? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachment+SDS.swift b/SignalServiceKit/src/Messages/Attachments/TSAttachment+SDS.swift index 3317835d34..fe13c88cdf 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachment+SDS.swift +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachment+SDS.swift @@ -315,7 +315,7 @@ extension TSAttachment { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> TSAttachment? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction+SDS.swift b/SignalServiceKit/src/Messages/Interactions/TSInteraction+SDS.swift index a391e9998b..25a94d61c4 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction+SDS.swift +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction+SDS.swift @@ -1063,7 +1063,7 @@ extension TSInteraction { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> TSInteraction? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/OWSBatchMessageProcessor+SDS.swift b/SignalServiceKit/src/Messages/OWSBatchMessageProcessor+SDS.swift index fb12cf9ec1..dfb6c48c51 100644 --- a/SignalServiceKit/src/Messages/OWSBatchMessageProcessor+SDS.swift +++ b/SignalServiceKit/src/Messages/OWSBatchMessageProcessor+SDS.swift @@ -158,7 +158,7 @@ extension OWSMessageContentJob { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> OWSMessageContentJob? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/OWSMessageReceiver+SDS.swift b/SignalServiceKit/src/Messages/OWSMessageReceiver+SDS.swift index 025dd55e02..0b8bf13445 100644 --- a/SignalServiceKit/src/Messages/OWSMessageReceiver+SDS.swift +++ b/SignalServiceKit/src/Messages/OWSMessageReceiver+SDS.swift @@ -150,7 +150,7 @@ extension OWSMessageDecryptJob { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> OWSMessageDecryptJob? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager+SDS.swift b/SignalServiceKit/src/Messages/OWSReadReceiptManager+SDS.swift index 720d17e413..7d43e23cbb 100644 --- a/SignalServiceKit/src/Messages/OWSReadReceiptManager+SDS.swift +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager+SDS.swift @@ -151,7 +151,7 @@ extension TSRecipientReadReceipt { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> TSRecipientReadReceipt? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/Stickers/DownloadStickerOperation.swift b/SignalServiceKit/src/Messages/Stickers/DownloadStickerOperation.swift index e962380ef1..de2f930e6a 100644 --- a/SignalServiceKit/src/Messages/Stickers/DownloadStickerOperation.swift +++ b/SignalServiceKit/src/Messages/Stickers/DownloadStickerOperation.swift @@ -37,14 +37,14 @@ class DownloadStickerOperation: OWSOperation { return OWSSignalService.sharedInstance().cdnSessionManager } - var firstAttempt = true - override public func run() { if InstalledStickers.isStickerInstalled(packId: packId, stickerId: stickerId) { Logger.verbose("Skipping redundant operation.") - return didFail(error: StickerError.redundantOperation) + var error = StickerError.redundantOperation + error.isRetryable = false + return reportError(error) } // https://cdn.signal.org/stickers//full/ @@ -69,24 +69,25 @@ class DownloadStickerOperation: OWSOperation { Logger.verbose("Decryption succeeded.") self.success(plaintext) - self.didSucceed() + self.reportSuccess() } catch let error as NSError { owsFailDebug("Decryption failed: \(error)") // Fail immediately; do not retry. - self.didFail(error: error) + error.isRetryable = false + return self.reportError(error) } - }) { [weak self] (_, error) in + }, + failure: { [weak self] (_, error) in guard let self = self else { return } Logger.error("Download failed: \(error)") - self.failureCount += 1 // TODO: We need to discriminate retry-able errors from // 404s, etc. We might want to abort on all 4xx and 5xx. self.reportError(error) - } + }) } override public func didFail(error: Error) { @@ -95,8 +96,6 @@ class DownloadStickerOperation: OWSOperation { failure(error) } - private var failureCount: UInt = 0 - override public func retryInterval() -> TimeInterval { // Arbitrary backoff factor... // With backOffFactor of 1.9 @@ -109,7 +108,7 @@ class DownloadStickerOperation: OWSOperation { let backoffFactor = 1.9 let maxBackoff = kHourInterval - let seconds = 0.1 * min(maxBackoff, pow(backoffFactor, Double(self.failureCount))) + let seconds = 0.1 * min(maxBackoff, pow(backoffFactor, Double(self.errorCount))) return seconds } } diff --git a/SignalServiceKit/src/Messages/Stickers/DownloadStickerPackOperation.swift b/SignalServiceKit/src/Messages/Stickers/DownloadStickerPackOperation.swift index d0c3cb49c3..4cd0def485 100644 --- a/SignalServiceKit/src/Messages/Stickers/DownloadStickerPackOperation.swift +++ b/SignalServiceKit/src/Messages/Stickers/DownloadStickerPackOperation.swift @@ -34,13 +34,13 @@ class DownloadStickerPackOperation: OWSOperation { return OWSSignalService.sharedInstance().cdnSessionManager } - var firstAttempt = true - override public func run() { if InstalledStickers.isStickerPackInstalled(packId: packId) { Logger.verbose("Skipping redundant operation.") - return didFail(error: StickerError.redundantOperation) + var error = StickerError.redundantOperation + error.isRetryable = false + return reportError(error) } // https://cdn.signal.org/stickers//manifest.proto @@ -65,24 +65,25 @@ class DownloadStickerPackOperation: OWSOperation { Logger.verbose("Decryption succeeded.") self.success(plaintext) - self.didSucceed() + self.reportSuccess() } catch let error as NSError { owsFailDebug("Decryption failed: \(error)") // Fail immediately; do not retry. - self.didFail(error: error) + error.isRetryable = false + return self.reportError(error) } - }) { [weak self] (_, error) in + }, + failure: { [weak self] (_, error) in guard let self = self else { return } Logger.error("Download failed: \(error)") - self.failureCount += 1 // TODO: We need to discriminate retry-able errors from // 404s, etc. We might want to abort on all 4xx and 5xx. self.reportError(error) - } + }) } override public func didFail(error: Error) { @@ -91,8 +92,6 @@ class DownloadStickerPackOperation: OWSOperation { failure(error) } - private var failureCount: UInt = 0 - override public func retryInterval() -> TimeInterval { // Arbitrary backoff factor... // With backOffFactor of 1.9 @@ -105,7 +104,7 @@ class DownloadStickerPackOperation: OWSOperation { let backoffFactor = 1.9 let maxBackoff = kHourInterval - let seconds = 0.1 * min(maxBackoff, pow(backoffFactor, Double(self.failureCount))) + let seconds = 0.1 * min(maxBackoff, pow(backoffFactor, Double(self.errorCount))) return seconds } } diff --git a/SignalServiceKit/src/Messages/Stickers/InstalledSticker+SDS.swift b/SignalServiceKit/src/Messages/Stickers/InstalledSticker+SDS.swift index e8dc2342b2..2b9c6cd7e2 100644 --- a/SignalServiceKit/src/Messages/Stickers/InstalledSticker+SDS.swift +++ b/SignalServiceKit/src/Messages/Stickers/InstalledSticker+SDS.swift @@ -154,7 +154,7 @@ extension InstalledSticker { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> InstalledSticker? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/Stickers/InstalledStickerPack+SDS.swift b/SignalServiceKit/src/Messages/Stickers/InstalledStickerPack+SDS.swift index 9ac9fb2926..fe515c0b1d 100644 --- a/SignalServiceKit/src/Messages/Stickers/InstalledStickerPack+SDS.swift +++ b/SignalServiceKit/src/Messages/Stickers/InstalledStickerPack+SDS.swift @@ -168,7 +168,7 @@ extension InstalledStickerPack { // Fetches a single model by "unique id". @objc - public class func anyFetch(withUniqueId uniqueId: String, + public class func anyFetch(uniqueId: String, transaction: SDSAnyReadTransaction) -> InstalledStickerPack? { assert(uniqueId.count > 0) diff --git a/SignalServiceKit/src/Messages/Stickers/InstalledStickers.swift b/SignalServiceKit/src/Messages/Stickers/InstalledStickers.swift index 5075af1cfb..6ffc6474b2 100644 --- a/SignalServiceKit/src/Messages/Stickers/InstalledStickers.swift +++ b/SignalServiceKit/src/Messages/Stickers/InstalledStickers.swift @@ -73,7 +73,7 @@ public class InstalledStickers: NSObject { let uniqueId = InstalledStickerPack.uniqueId(forPackId: packId) - return InstalledStickerPack.anyFetch(withUniqueId: uniqueId, transaction: transaction) + return InstalledStickerPack.anyFetch(uniqueId: uniqueId, transaction: transaction) } @objc @@ -217,7 +217,7 @@ public class InstalledStickers: NSObject { let uniqueId = InstalledSticker.uniqueId(forPackId: packId, stickerId: stickerId) - return InstalledSticker.anyFetch(withUniqueId: uniqueId, transaction: transaction) + return InstalledSticker.anyFetch(uniqueId: uniqueId, transaction: transaction) } @objc diff --git a/SignalServiceKit/src/Util/JobQueue.swift b/SignalServiceKit/src/Util/JobQueue.swift index f5ec89f8b4..2a2d568b41 100644 --- a/SignalServiceKit/src/Util/JobQueue.swift +++ b/SignalServiceKit/src/Util/JobQueue.swift @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // import Foundation @@ -22,7 +22,12 @@ import Foundation public extension Error { var isRetryable: Bool { - return (self as NSError).isRetryable + get { + return (self as NSError).isRetryable + } + set { + (self as NSError).isRetryable = newValue + } } } diff --git a/SignalServiceKit/src/Util/OWSOperation.h b/SignalServiceKit/src/Util/OWSOperation.h index ebeeaa53f4..436bef1ecd 100644 --- a/SignalServiceKit/src/Util/OWSOperation.h +++ b/SignalServiceKit/src/Util/OWSOperation.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // #import @@ -24,7 +24,9 @@ typedef NS_ENUM(NSInteger, OWSOperationState) { // any of the errors were fatal. Fatal errors trump retryable errors. @interface OWSOperation : NSOperation -@property (readonly, nullable) NSError *failingError; +@property (nonatomic, readonly, nullable) NSError *failingError; + +@property (nonatomic, readonly) NSUInteger errorCount; // Defaults to 0, set to greater than 0 in init if you'd like the operation to be retryable. @property NSUInteger remainingRetries; diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index 444d9c886e..2b30490103 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -15,13 +15,15 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; @interface OWSOperation () -@property (nullable) NSError *failingError; +@property (nonatomic, nullable) NSError *failingError; @property (atomic) OWSOperationState operationState; @property (nonatomic) OWSBackgroundTask *backgroundTask; // This property should only be accessed on the main queue. @property (nonatomic) NSTimer *_Nullable retryTimer; +@property (nonatomic) NSUInteger errorCount; + @end @implementation OWSOperation @@ -172,6 +174,8 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; error.isRetryable, (unsigned long)self.remainingRetries); + self.errorCount += 1; + [self didReportError:error]; if (error.isFatal) { diff --git a/SignalServiceKit/tests/Storage/SDSKeyValueStoreTest.swift b/SignalServiceKit/tests/Storage/SDSKeyValueStoreTest.swift index 37625d5558..fede33cafa 100644 --- a/SignalServiceKit/tests/Storage/SDSKeyValueStoreTest.swift +++ b/SignalServiceKit/tests/Storage/SDSKeyValueStoreTest.swift @@ -81,4 +81,34 @@ class SDSKeyValueStoreTest: SSKBaseTestSwift { XCTAssertNil(store.getString("stringA")) XCTAssertEqual("valueC", store.getString("stringB")) } + + func test_data() { + let store = SDSKeyValueStore(collection: "test") + + let bytesA = Randomness.generateRandomBytes(32) + let bytesB = Randomness.generateRandomBytes(32) + + XCTAssertNil(store.getData("dataA")) + XCTAssertNil(store.getData("dataB")) + + store.setData(bytesA, key: "dataA") + + XCTAssertEqual(bytesA, store.getData("dataA")) + XCTAssertNil(store.getData("dataB")) + + store.setData(bytesB, key: "dataA") + + XCTAssertEqual(bytesB, store.getData("dataA")) + XCTAssertNil(store.getData("dataB")) + + store.setData("valueC", key: "dataB") + + XCTAssertEqual(bytesB, store.getData("dataA")) + XCTAssertEqual("valueC", store.getData("dataB")) + + store.setData(nil, key: "dataA") + + XCTAssertNil(store.getData("dataA")) + XCTAssertEqual("valueC", store.getData("dataB")) + } }