diff --git a/PSOperations/GroupOperation.swift b/PSOperations/GroupOperation.swift index bc38362..72bf68c 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,14 +103,31 @@ 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) } } } + +/* Internal extensions for UnitTesting */ + +extension GroupOperation { + func stubGroupOperation(withError error: NSError) { + internalQueue.cancelAllOperations() + cancelWithError(error) + } +} + diff --git a/PSOperations/Operation.swift b/PSOperations/Operation.swift index 7d6287c..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) @@ -380,10 +388,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 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) }