From ab1c720f606cae8869e81d8c105259625b332f33 Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Wed, 17 May 2017 17:11:46 +0200 Subject: [PATCH 1/5] Make GroupOperation error aggregation thread-safe --- PSOperations/GroupOperation.swift | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/PSOperations/GroupOperation.swift b/PSOperations/GroupOperation.swift index bc38362..2b95c8d 100644 --- a/PSOperations/GroupOperation.swift +++ b/PSOperations/GroupOperation.swift @@ -26,6 +26,7 @@ open class GroupOperation: Operation { fileprivate let startingOperation = Foundation.BlockOperation(block: {}) fileprivate let finishingOperation = Foundation.BlockOperation(block: {}) + fileprivate let errorsLock = NSLock() fileprivate var aggregatedErrors = [NSError]() public convenience init(operations: Foundation.Operation...) { @@ -65,7 +66,9 @@ open class GroupOperation: Operation { of errors reported to observers and to the `finished(_:)` method. */ public final func aggregateError(_ error: NSError) { - aggregatedErrors.append(error) + errorsLock.withCriticalScope { + aggregatedErrors.append(error) + } } open func operationDidFinish(_ operation: Foundation.Operation, withErrors errors: [NSError]) { @@ -100,11 +103,18 @@ extension GroupOperation: OperationQueueDelegate { } final public func operationQueue(_ operationQueue: OperationQueue, operationDidFinish operation: Foundation.Operation, withErrors errors: [NSError]) { - aggregatedErrors.append(contentsOf: errors) + errorsLock.withCriticalScope { + aggregatedErrors.append(contentsOf: errors) + } if operation === finishingOperation { internalQueue.isSuspended = true - finish(aggregatedErrors) + + let errors = errorsLock.withCriticalScope { + return aggregatedErrors + } + + finish(errors) } else if operation !== startingOperation { operationDidFinish(operation, withErrors: errors) From 2a60a6ff03c96082014d696671ecd9e957b08bd3 Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Wed, 17 May 2017 17:12:21 +0200 Subject: [PATCH 2/5] Make check for finished in Operation thread-safe --- PSOperations/Operation.swift | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/PSOperations/Operation.swift b/PSOperations/Operation.swift index 7d6287c..3aa563a 100644 --- a/PSOperations/Operation.swift +++ b/PSOperations/Operation.swift @@ -380,10 +380,9 @@ open class Operation: Foundation.Operation { A private property to ensure we only notify the observers once that the operation has finished. */ - fileprivate var hasFinishedAlready = false + fileprivate var hasFinishedAlready: UInt8 = 0 public final func finish(_ errors: [NSError] = []) { - if !hasFinishedAlready { - hasFinishedAlready = true + if !OSAtomicTestAndSet(0, &hasFinishedAlready) { state = .finishing _internalErrors += errors From 9d9c628839d64bb029d1cd71775d50e7ad6d19fc Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Wed, 31 May 2017 13:21:12 +0200 Subject: [PATCH 3/5] Hold strong reference to delegate which can be deallocated during completion block execution --- PSOperations/OperationQueue.swift | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/PSOperations/OperationQueue.swift b/PSOperations/OperationQueue.swift index 6c9acbc..564984c 100644 --- a/PSOperations/OperationQueue.swift +++ b/PSOperations/OperationQueue.swift @@ -46,7 +46,9 @@ open class OperationQueue: Foundation.OperationQueue { finishHandler: { [weak self] finishedOperation, errors in if let q = self { - q.delegate?.operationQueue?(q, operationDidFinish: finishedOperation, withErrors: errors) + if let delegate = q.delegate { + delegate.operationQueue?(q, operationDidFinish: finishedOperation, withErrors: errors) + } //Remove deps to avoid cascading deallocation error //http://stackoverflow.com/questions/19693079/nsoperationqueue-bug-with-dependencies finishedOperation.dependencies.forEach { finishedOperation.removeDependency($0) } @@ -96,7 +98,9 @@ open class OperationQueue: Foundation.OperationQueue { */ operation.addCompletionBlock { [weak self, weak operation] in guard let queue = self, let operation = operation else { return } - queue.delegate?.operationQueue?(queue, operationDidFinish: operation, withErrors: []) + if let delegate = queue.delegate { + delegate.operationQueue?(queue, operationDidFinish: operation, withErrors: []) + } //Remove deps to avoid cascading deallocation error //http://stackoverflow.com/questions/19693079/nsoperationqueue-bug-with-dependencies operation.dependencies.forEach { operation.removeDependency($0) } From 6a83853a18b14bcd4e4b81481e8cea56211c178f Mon Sep 17 00:00:00 2001 From: Sergey Bykov Date: Fri, 9 Jun 2017 14:40:20 +0200 Subject: [PATCH 4/5] Generate isReady KVO even only once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem was that “isReady” property change KVO event was generated on every “state” property change, but in most of cases state-change doesn’t mean isReady-change. After every isReady KVO event, isReady property is read by NSOperations runtime. Since state-change (and as a result isReady KVO event) may happen and happens in multiple threads, isReady-getter is called from multiple threads too. The problem happens when isReady is called from multiple threads around the same time: the first call return False, but the second one returns True. Since there are two threads, first call can be actually finished after second one. In this case NSOperations runtime receivce isReady state value in the order like [false, false… false, true, false]. In this situation operations is actually ready, but it is not executed by queue (looks like because last isReady state was False). To fix the described problem idea is to stop bombing with KVO events. Actually during NSOperation life isReady is changed only once from false -> true. So ideally is to have only one KVO event when operations is really ready. --- PSOperations/Operation.swift | 78 ++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/PSOperations/Operation.swift b/PSOperations/Operation.swift index 3aa563a..262ca80 100644 --- a/PSOperations/Operation.swift +++ b/PSOperations/Operation.swift @@ -16,6 +16,8 @@ import Foundation */ open class Operation: Foundation.Operation { + private var observerContext = 4321 + /* The completionBlock property has unexpected behaviors such as executing twice and executing on unexpected threads. BlockObserver * executes in an expected manner. */ @@ -29,12 +31,7 @@ open class Operation: Foundation.Operation { } } - // use the KVO mechanism to indicate that changes to "state" affect other properties as well - class func keyPathsForValuesAffectingIsReady() -> Set { - return ["state" as NSObject, "cancelledState" as NSObject] - } - class func keyPathsForValuesAffectingIsExecuting() -> Set { return ["state" as NSObject] } @@ -47,6 +44,15 @@ open class Operation: Foundation.Operation { return ["cancelledState" as NSObject] } + public override init() { + super.init() + self.addObserver(self, forKeyPath: "isReady", context: &observerContext) + } + + deinit { + self.removeObserver(self, forKeyPath: "isReady", context: &observerContext) + } + // MARK: State Management fileprivate enum State: Int, Comparable { @@ -142,48 +148,49 @@ open class Operation: Foundation.Operation { assert(_state.canTransitionToState(newState, operationIsCancelled: isCancelled), "Performing invalid state transition.") _state = newState + updateReadiness() } didChangeValue(forKey: "state") } } - // Here is where we extend our definition of "readiness". - override open var isReady: Bool { - - var _ready = false - + open override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) { + if context == &observerContext { + updateReadiness() + } else { + super.observeValue(forKeyPath: keyPath, of: object, change: change, context: context) + } + } + + private func updateReadiness() { stateLock.withCriticalScope { - switch state { - - case .initialized: - // If the operation has been cancelled, "isReady" should return true - _ready = isCancelled - - case .pending: - // If the operation has been cancelled, "isReady" should return true - guard !isCancelled else { + let cancelled = isCancelled + + if state == .pending { + if cancelled { state = .ready - _ready = true - return - } - - // If super isReady, conditions can be evaluated - if super.isReady { + } else if super.isReady { evaluateConditions() - _ready = state == .ready } - - case .ready: - _ready = super.isReady || isCancelled - - default: - _ready = false } - + + // Generate KVO notitification only once when operation is really ready + let newReady = state >= .ready || (state == .initialized && cancelled) + if !_ready && newReady { + willChangeValue(forKey: "isReady") + _ready = true + didChangeValue(forKey: "isReady") + } + } + } + + private var _ready: Bool = false + + override open var isReady: Bool { + return stateLock.withCriticalScope { + return _ready && super.isReady } - - return _ready } open var userInitiated: Bool { @@ -214,6 +221,7 @@ open class Operation: Foundation.Operation { didSet { didChangeValue(forKey: "cancelledState") if _cancelled != oldValue && _cancelled == true { + updateReadiness() for observer in observers { observer.operationDidCancel(self) From ed9e249ca4431c44b7bef43aae6111c49dd4672e Mon Sep 17 00:00:00 2001 From: Lisa Konysheva Date: Fri, 25 Aug 2017 14:55:52 +0200 Subject: [PATCH 5/5] Added extension for GroupOperation to handle error case --- PSOperations/GroupOperation.swift | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/PSOperations/GroupOperation.swift b/PSOperations/GroupOperation.swift index 2b95c8d..72bf68c 100644 --- a/PSOperations/GroupOperation.swift +++ b/PSOperations/GroupOperation.swift @@ -121,3 +121,13 @@ extension GroupOperation: OperationQueueDelegate { } } } + +/* Internal extensions for UnitTesting */ + +extension GroupOperation { + func stubGroupOperation(withError error: NSError) { + internalQueue.cancelAllOperations() + cancelWithError(error) + } +} +