Yes, this looks fine re KVO and thread-safety.
A few stylistic observations:
I would personally move the willChangeValue and didChangeValue into the setters. It feels a little cluttered to have them sprinkled about the implementation. This pattern in your code snippet harkens back to the original Objective-C implementation, but Swift provides us opportunities to tidy up this code.
I know you got your keynames right, but I always use #keyPath(isExecuting) and #keyPath(isFinished) to have the compiler enforce the string values.
I would make both isExecuting and isFinished read-only properties (with private(set)) as no external code has any business changing these values.
Personally, I would pull the main implementation out of ConcurrentOperation and put it into its own subclass. Once you have this ConcurrentOperation defined, you will find yourself using it everywhere you do not want to repeat all this boilerplate code in every concurrent operation subclass.
Thus:
class ConcurrentOperation: Operation {
private let stateQueue = DispatchQueue(label: "operation", attributes: .concurrent)
private var _isExecuting = false
private var _isFinished = false
override private(set) var isExecuting: Bool {
get {
stateQueue.sync { _isExecuting }
}
set {
willChangeValue(forKey: #keyPath(isExecuting))
stateQueue.async(flags: .barrier) {
self._isExecuting = newValue
}
didChangeValue(forKey: #keyPath(isExecuting))
}
}
override private(set) var isFinished: Bool {
get {
stateQueue.sync { _isFinished }
}
set {
willChangeValue(forKey: #keyPath(isFinished))
stateQueue.async(flags: .barrier) {
self._isFinished = newValue
}
didChangeValue(forKey: #keyPath(isFinished))
}
}
override var isAsynchronous: Bool {
return true
}
override func start() {
if isCancelled {
finish()
return
}
isExecuting = true
main()
}
override func main() {
fatalError("This must be overridden")
}
func finish() {
if isExecuting {
isExecuting = false
}
if !isFinished {
isFinished = true
}
}
}
And
class TwoSecondOperation: ConcurrentOperation {
override func main() {
DispatchQueue.global().asyncAfter(deadline: .now() + 2) {
if self.isCancelled {
print("cancelled")
} else {
print("done")
}
self.finish()
}
}
}
Going a step further, we generally strive for cancelable Operation subclasses, where possible. E.g., in this example, rather than using a closure with asyncAfter, we should instead used a DispatchWorkItem. Then we could make this operation truly cancelable (rather than having to wait two seconds before the cancelation is recognized).
For example, perhaps something like the following:
class TwoSecondOperationImproved: ConcurrentOperation {
private var item: DispatchWorkItem?
override init() {
super.init()
item = DispatchWorkItem {
if self.isCancelled {
print("cancelled")
} else {
print("done")
}
self.finish()
}
}
override func main() {
if let item = item {
DispatchQueue.global().asyncAfter(deadline: .now() + 2, execute: item)
}
}
override func cancel() {
super.cancel()
item?.cancel()
finish()
}
override func finish() {
item = nil
super.finish()
}
}
In short, if the asynchronous task being wrapped in ConcurrentOperation offers cancelation, make sure to override cancel, too.
If you prefer, you can also use locks to synchronize the properties of ConcurrentOperation:
class ConcurrentOperation: Operation {
private let lock = NSLock()
private var _isExecuting = false
private var _isFinished = false
override private(set) var isExecuting: Bool {
get {
synchronized { _isExecuting }
}
set {
willChangeValue(forKey: #keyPath(isExecuting))
synchronized { _isExecuting = newValue }
didChangeValue(forKey: #keyPath(isExecuting))
}
}
override private(set) var isFinished: Bool {
get {
synchronized { _isFinished }
}
set {
willChangeValue(forKey: #keyPath(isFinished))
synchronized { _isFinished = newValue }
didChangeValue(forKey: #keyPath(isFinished))
}
}
override var isAsynchronous: Bool {
return true
}
override func start() {
if isCancelled {
finish()
return
}
isExecuting = true
main()
}
override func main() {
fatalError("This must be overridden")
}
func finish() {
if isExecuting {
isExecuting = false
}
if !isFinished {
isFinished = true
}
}
private func synchronized<T>(block: () throws -> T) rethrows -> T {
try lock.synchronized { try block() }
}
}
extension NSLocking {
func synchronized<T>(block: () throws -> T) rethrows -> T {
lock()
defer { unlock() }
return try block()
}
}
