3

There is a protocol with the following declaration:

typealias SuggestionSourceCallback = ([Suggestion]) -> ()

protocol SuggestionSource {
    func suggest(_ query: SuggestionQuery, callback: @escaping SuggestionSourceCallback)
}

Two classes implement this protocol. First class obtains suggestions asynchronously (via GCD)

final class FisrtClass: SuggestionSource {
    private let queue = DispatchQueue(label: "my.app.queue", attributes: [])
    private var lastQuery: SuggestionQuery?
    // ...
    func suggest(_ query: SuggestionQuery, callback: @escaping SuggestionSourceCallback) { 
        self.queue.async { [weak self] in

            // capturing strong self
            guard let strongSelf = self else {
                return
            }

            // referencing self here, for example
            guard self.lastQuery == query else {
                return
            }

            // suggestions is a local variable
            var suggestions: [Suggestion] = []

            // ... 

            DispatchQueue.main.async {
                callback(suggestions)
            }
        }
    }
}

...while second class does it synchronously

final class SecondClass: SuggestionSource {
    // ...
    func suggest(_ query: SuggestionQuery, callback: @escaping SuggestionSourceCallback) {
        // ...
        callback(self.suggestions[query])
    }
}

My questions are:

  1. should I capture strongSelf in FirstClass's implementation?
  2. should I capture strongSelf in SecondsClass's implementation?

UPDATE

Additional question. Suppose SecondClass has its suggestions as a static let, what pattern in this case would be?

final class SecondClass: SuggestionSource {
    static let suggestions: [String: [SuggestionQuery]] = {
         // ...
    }()

    // ...
    func suggest(_ query: SuggestionQuery, callback: @escaping SuggestionSourceCallback) {
        // ...
        callback(self.suggestions[query])
    }
}
Yevhen Dubinin
  • 4,657
  • 3
  • 34
  • 57
  • Where does `suggestions` come from in `FirstClass`? It's unclear whether it's an instance property, or creating from within the closure (if the latter, then why the need to capture `self` in the first place?). Also the code in your 'update' doesn't compile. – Hamish Nov 28 '16 at 16:52
  • @Hamish I edited the code snippet, please take a look – Yevhen Dubinin Dec 06 '16 at 10:46
  • Okay, well the answer for whether you should capture `self` weakly in your `FirstClass` is that *it depends*. It depends on whether you want the given instance of `FirstClass` to be retained by the closure, and wait until it has been executed by the queue before being deallocated, or whether you want the instance to not wait for the closure to execute. With such an abstract example, there's no way to tell what the best behaviour should be. Of course, the answer to the other two questions, as others have already said, is that because they're synchronous, you don't need weak capturing at all. – Hamish Dec 06 '16 at 18:26

2 Answers2

1

In SecondClass, there is no need to create a strongSelf variable. Where would you put it? The point is that self is guaranteed not to be nil anyway because you are running within the scope of one of its methods.

The same is true of your additional question, but for a different reason. suggestions is now static, so prefixing with self is a matter of syntax, (I am presuming you meant to also prefix the suggest method with static).

However, in FirstClass, there is a subtle difference between capturing strongSelf and not capturing it.

Because you are using [weak self], self could be nil when you enter that block so you need to check against that anyway. One way is to repeatedly use optional chaining, i.e.:

self?.doSomething()
self?.doSomethingElse()

This is saying:

If I have a reference to self, do something. If I still have a reference to self, do something else.

By adding a strongSelf variable:

guard let strongSelf = self else {
  return
}
strongSelf.doSomething()
strongSelf.doSomethingElse()

...you are saying:

do something and do something else if you have a reference to self, otherwise do nothing.

So, you guarantee that if the first thing happens, so does the second. The approach you take is going to depend on your application.

ganzogo
  • 2,516
  • 24
  • 36
-2

Scenario 1 is a good candidate for [unowned self].

In this case if the queue exists, so does self, therefore it is safe to reference self without retaining it.

Note: You should only use unowned when you can be sure that the block's lifecycle is directly tied to the captured variable. In other cases unowned can cause interrmittent crashes (which are really hard to debug).

Also unowned is more performant than weak so should be preferred where it is safe to use either source.

For scenario 2, self is not captured by any block that I can determine so you shouldn't need to worry at all about it.

For the update, you still don't capture self, the closure that defines the suggestions dictionary should be executed as soon as it is called.

Tristan Burnside
  • 2,546
  • 1
  • 16
  • 23
  • 2
    I disagree about this being a good case for `[unowned self]`. That is how you trigger a zombie, and odd side-effects or crashes. If you end up invoking the block after the object that created it was deallocated, the `unowned` reference to self won't be nil, and will now point to a block of memory that may be empty or may now point to a different object. This is the textbook case for using `weak`. – Duncan C Nov 28 '16 at 13:33
  • I disagree in this case, the only reference to the queue is in this class, so if there is a block there is a queue to hold a reference to it and if there is a queue that is executing the block then there is also self. – Tristan Burnside Nov 29 '16 at 12:11
  • Hmm, I guess you're right **IN THIS CASE** that if there is a block/closure then it must be running on the queue, hence the FirstClass instance must exist. As a general-purpose answer, though, I think it's important that people understand the danger of using unowned references. You need to really understand when it is and is not ok to use an unowned reference. – Duncan C Nov 29 '16 at 14:22
  • I added a warning about use of unowned. I think that saying you shouldn't even try to learn when it is safe to use and always use weak is bad advice because it will just promote more people not bothering to learn about it. – Tristan Burnside Nov 30 '16 at 06:54
  • "*In this case if the queue exists, so does self*" – this is not correct. The queue will be retained so long as it has tasks to execute. If the instance is deallocated before the given closure that captures that instance as `unowned` executes, you'll get a crash. – Hamish Dec 06 '16 at 18:32