You should never design a function with a completion handler that sometimes invokes it synchronously and sometimes asynchronously. That can cause all sorts of problems, the simplest of which is a caller that expects it to be asynchronous and ends up performing work out-of-order because the handler was invoked synchronously.
The final form of the code in this article accidentally fixes this, by moving the work it does onto a background queue, but this just introduces a separate flaw: it performs the callback from its internal private queue. This isn't necessarily a problem, except in that it will block any other work from running on that queue unnecessarily (as it's a serial queue), but in more complicated code, having external code running on your private queue can indeed cause problems.
---
There's also another issue, which is that if the AccessTokenService gets deinited before the loader calls its callback, the callback is dropped on the floor. In general, you should not design a function that takes a completion callback with conditions where the completion callback is never fired. There's 3 reasonable behaviors that could be implemented here:
1. Have the callback keep the AccessTokenService alive. It's pretty common for networking operations to keep themselves alive while executing, so this is probably the approach I'd take.
2. Have the AccessTokenService fire all of its pending completion handlers with an error in deinit. The error should ideally represent the notion that the operation was cancelled.
3. Change the AccessTokenLoader completion handler to look more like the initial version, where it still calls the callback even if self is nil. With the pending completion handlers approach, this requires moving the pending completion handlers into a reference-type wrapper and strongly referencing that from the AccessTokenLoader callback. In this case, that's pretty much the same as approach #1, so you should just go ahead and use approach #1, but in a more complex class this approach might have some utility.