Swift: Maintaining atomicity in a block-based execution using weak self

Question

I often see code that uses weak self like below:

api.call() { [weak self] (result, error) in
  if (error == nil) {
    setGlobalState()
    self?.doSomething()
  } else {
    setSomeErrorState()
    self?.doSomethingElse()
  }
}

But it seems to me that if self is nil, now the states are inconsistent because setGlobalState() would have executed but self?.doSomething() did not.

It would seem like the sensible thing to do is as follows:

api.call() { [weak self] (result, error) in
  guard let self = self else { return }

  if (error == nil) {
    setGlobalState()
    self.doSomething()
  } else {
    setSomeErrorState()
    self.doSomethingElse()
  }
}

Is my concern about the lack of atomicity in the first case legit? Should the latter case be the best practice when it comes to a block that uses weak self?

Answer 1

There’s no simple answer to this question, as the correct answer depends upon what doSomething and doSomethingElse are doing, something which is unclear here.

That having been said, you say:

I often see code that uses weak self like below:

api.call() { [weak self] (result, error) in
  if error == nil {
    setGlobalState()
    self?.doSomething()
  } else {
    setSomeErrorState()
    self?.doSomethingElse()
  }
}

But it seems to me that if self is nil, now the states are inconsistent because setGlobalState() would have executed but self?.doSomething() did not.

Technically, it is true that this could be a problem, but is a pretty unusual situation. But if this is a problem, that is likely code smell for a deeper problem.

Typically when you see something like this, doSomething or doSomethingElse are updating UI or something unique to whatever self is, in which the above pattern is actually preferable. The code snippet above ensures that setGlobalState or setSomeErrorState takes place, but if self is, for example, the view controller, it ensures that we don’t artificially retain it simply to update a UI that is no longer present.

But if it is the case that doSomething and doSomethingElse are not just updating a UI, but rather are “setting some singleton states” as you suggest in your subsequent comment, then your are right, that the above is not going to achieve what you want.

So, you go on to say:

It would seem like the sensible thing to do is as follows:

api.call() { [weak self] (result, error) in
  guard let self = self else { return }

  if error == nil {
    setGlobalState()
    self.doSomething()
  } else {
    setSomeErrorState()
    self.doSomethingElse()
  }
}

The challenge here is that if self is deallocated before the API completion handler is called, then it’s not going to do any of this. You’ve performed your API call, but neither setGlobalState/doSomething nor setSomeErrorState/doSomethingElse will be called. So if you’re really setting global states and singleton properties, you’re right that they’ll be internally consistent, but they’re now potentially out of sync with your web service.

You use this second pattern only if (a) it’s essential that, for example, if setGlobalState is called, then doSomething must also be called; but that (b) if self is deallocated, it’s OK that neither of these methods are called.

There is a third alternative that you might want to consider, namely omit [weak self] altogether:

api.call() { result, error in
  if error == nil {
    setGlobalState()
    self.doSomething()
  } else {
    setSomeErrorState()
    self.doSomethingElse()
  }
}

If it’s essential that both setGlobalState and doSomething (or both setSomeErrorState and doSomethingElse) must be called upon completion of the API call, then we simply wouldn’t use [weak self] at all. (Note, this only works if api was properly implemented, designed to not hang on to the closure when it was complete, but all well-designed asynchronous API do that, anyway.)

It should be noted, though, that if there is really some hidden global dependency between setGlobalState and doSomething (or between setSomeErrorState and doSomethingElse), that suggests a deeper problem in the design. Separate objects should be loosely coupled. It’s questionable to be using either globals or stateful singletons at all, much less encumbering the application developer with the responsibility of keeping both in sync themselves.

The bottom line, the choice of these three api completion alternatives is completely dependent upon what self is, what doSomething does, and what doSomethingElse does. I would not categorically dismiss the first alternative as it is often the correct solution.