I'm creating an NSOperation that executes a closure with a delay. The operations are added into a queue, every time before a new operation is added I cancel all existing ones in the queue:
let myOp = SomeOperation { [weak self] in /* do something */ }
queue.cancelAllOperations()
queue.addOperation(myOp)
Operation Code 1
final class SomeOperation: Operation {
private let closure: () -> Void
init(closure: @escaping () -> Void) {
self.closure = closure
}
override func main() {
if isCancelled {
return
}
DispatchQueue.main.asyncAfter(deadline: .now() 0.3, execute: doSomething)
}
private func doSomething() {
guard isCancelled == false else {
return
}
closure()
}
}
While the above code works, the code below doesn't. In the DispatchQueue closure, self is nil:
Operation Code 2
final class SomeOperation: Operation {
private let closure: () -> Void
init(closure: @escaping () -> Void) {
self.closure = closure
}
override func main() {
if isCancelled {
return
}
DispatchQueue.main.asyncAfter(deadline: .now() 0.3) { [weak self] in
guard let self = self else { return }
guard isCancelled == false else { return }
self.closure()
}
}
}
So I'm trying to learn a bit deeper:
- In Code 2,
selfisnilbecause as soon asDispatchQueue.main.asyncAfter…is called, themainmethod finishes and the operation is thus released. - Code 1 works because
execute: doSomethingimplicitly captures/retains aself, so even afterasyncAfter,selfis still there.
So my questions are:
- In Apple's doc it says for concurrent operations I should rather be using
start,asynchronous,executing,finished, etc. In my case I just need to have a delay, not actually doing anything asynchronous, should I do it inmainonly or should I do it as an async operation by implementing those methods Apple suggested? - Is my thinking correct that in Code 1 there's a
selfretained implicitly, which doesn't sound correct and can create retain cycle?
Thanks!
CodePudding user response：
You asked:
- In Apple's doc it says for concurrent operations I should rather be using
start,asynchronous,executing,finished, etc. In my case I just need to have a delay, not actually doing anything asynchronous, should I do it in main only or should I do it as an async operation by implementing those methods Apple suggested?
First, you are doing something asynchronous. I.e., asyncAfter is asynchronous.
Second, the reason we wrap asynchronous operations as Apple described is that the whole purpose is that the operation should not finish until the asynchronous task it launched finished. It’s one of the key reasons we use operations rather than just GCD. Perhaps you don’t necessarily need that functionality at this point, but there’s no reason to have the operation complete before the wrapped asynchronous task finishes. And if you do defer the completion of the operation, it opens the door for all sorts of elegant dependencies between asynchronous tasks.
For example, let’s say you create three of these SomeOperation, each dependent upon the prior one. Or perhaps you add them to a queue whose maxConcurrentOperationCount is 1.
The intent in this scenario is that you don’t want operation 2 to start until the asynchronous process of operation 1 is done. If you don’t make this a concurrent operation, then operation 1, 2, and 3 will start immediately.
- Is my thinking correct that in Code 1 there's a self retained implicitly, which doesn't sound correct and can create retain cycle?
Regarding the strong reference cycle issues, let’s look at your first example. While it is prudent for the creator of the operation to use [weak self] capture list, it should not be required. Good design of the operation (or anything using asynchronously called closures) is to have it release the closure when it is no longer needed:
class SomeOperation2: Operation {
private var closure: (() -> Void)?
init(closure: @escaping () -> Void) {
self.closure = closure
}
override func main() {
if isCancelled {
return
}
DispatchQueue.main.asyncAfter(deadline: .now() 0.3, execute: doSomething)
}
override func cancel() {
closure = nil
super.cancel()
}
private func doSomething() {
guard !isCancelled else {
return
}
closure?()
closure = nil
}
}
It doesn’t mean that the caller shouldn’t use [weak self] capture list, only that the operation no longer requires it, and will resolve any strong reference cycles when it is done with the closure.
[Note, in the above, I omitted synchronization of the variable, to keep it simple. But you need to synchronize your access to it to ensure thread-safe design.]
But this design begs the question as to why would you would want to keep the asyncAfter scheduled, still firing even after you canceled the operation. It would be better to cancel it, by wrapping the closure in a DispatchWorkItem, which can be canceled, e.g.:
class SomeOperation: Operation {
private var item: DispatchWorkItem!
init(closure: @escaping () -> Void) {
super.init()
item = DispatchWorkItem { [weak self] in
closure()
self?.item = nil
}
}
override func main() {
if isCancelled { return }
DispatchQueue.main.asyncAfter(deadline: .now() 0.3, execute: item)
}
override func cancel() {
item?.cancel()
item = nil
super.cancel()
}
}
Having outlined the memory issues, we should note that this is all probably moot, as you probably should just make this a concurrent operation (with all that custom KVO) as you identified in the documentation. E.g.,
class SomeOperation: AsynchronousOperation {
private var item: DispatchWorkItem!
init(closure: @escaping () -> Void) {
super.init()
item = DispatchWorkItem { [weak self] in
closure()
self?.item = nil
self?.complete()
}
}
override func main() {
if isCancelled { return }
synchronized {
DispatchQueue.main.asyncAfter(deadline: .now() 3, execute: item)
}
}
override func cancel() {
super.cancel()
synchronized {
item?.cancel()
item = nil
}
}
}
The above uses an asynchronous operation base class that (a) performs the necessary KVO notifications; and (b) is thread-safe. Here is one random example of how that could be implemented:
/// Asynchronous Operation base class
///
/// This class performs all of the necessary KVN of `isFinished` and
/// `isExecuting` for a concurrent `NSOperation` subclass. So, to developer
/// a concurrent NSOperation subclass, you instead subclass this class which:
///
/// - must override `main()` with the tasks that initiate the asynchronous task;
///
/// - must call `complete()` function when the asynchronous task is done;
///
/// - optionally, periodically check `self.cancelled` status, performing any clean-up
/// necessary and then ensuring that `complete()` is called; or
/// override `cancel` method, calling `super.cancel()` and then cleaning-up
/// and ensuring `complete()` is called.
public class AsynchronousOperation: Operation {
private let lock = NSLock()
private var _executing: Bool = false
override private(set) public var isExecuting: Bool {
get {
synchronized { _executing }
}
set {
willChangeValue(forKey: #keyPath(isExecuting))
synchronized { _executing = newValue }
didChangeValue(forKey: #keyPath(isExecuting))
}
}
private var _finished: Bool = false
override private(set) public var isFinished: Bool {
get {
synchronized { _finished }
}
set {
willChangeValue(forKey: #keyPath(isFinished))
synchronized { _finished = newValue }
didChangeValue(forKey: #keyPath(isFinished))
}
}
override public var isAsynchronous: Bool { return true }
/// Complete the operation
///
/// This will result in the appropriate KVN of isFinished and isExecuting
public func complete() {
if isExecuting {
isExecuting = false
isFinished = true
}
}
public override func cancel() {
super.cancel()
complete()
}
override public func start() {
if isCancelled {
isFinished = true
return
}
isExecuting = true
main()
}
override public func main() {
fatalError("subclasses must override `main`")
}
public func synchronized<T>(block: () throws -> T) rethrows -> T {
try lock.synchronized { try block() }
}
}
extension NSLocking {
public func synchronized<T>(block: () throws -> T) rethrows -> T {
lock()
defer { unlock() }
return try block()
}
}