4 ms·
This code has a significant and common flaw: If the token is cached, it invokes the handler synchronously. You should never design a function with a completion
by eridius 8y ago
This code has a significant and common flaw: If the token is cached, it invokes the handler synchronously.
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.
- anothergoogler 8y agoYou have it backwards, it's the caller's bug if they assume order in an async process.
- SamReidHughes 8y agoNo no no no no. He's right, and this is a hard-learned lesson that many practitioners have discovered independently. Both the caller and callee face huge problems of reentrant function calls when you mix up the behavior. Generally the right choice is to make every callback invoked asynchronously, unless you need to do something else.
- solarexplorer 8y agoExactly. UITableView is a good example of how _not_ to do things.
- dep_b 8y agoYeah the mixup of MVC layers is terrible and anything beyond a hard reload data has many edge cases.
- alexashka 8y agoCan you explain what problems someone faces if a callback is executed immediately?
- Matthias247 8y agoOne usual issue on the callee side is that the code inside the callback might call again into the same component (here: AccessTokenLoader::retrieveToken). When this call is done, state inside the component is modified before the callback returns. When the callback returns the component is now no longer in the state that it was, which might not be expected. In C++, it could even happen that the object was deleted within the callback. Then accessing any local field after the callback returns leads to a segfault. One kind of remedy is to only call callbacks at the end of the the logic, and don't touch anything anymore after the callback returns. That works in some cases - however scheduling the callback might be easier to reason about. Then there are also issues on the caller side, if the caller doesn't expect that the callback is sometimes made synchronously and sometimes asynchronously. If it's assumed that the callback happens asynchronously and some state is modified in it, the code that runs after invoking the initial function might work on wrong assumptions (wasn't aware that state could have been modified).
- charleslmunger 8y agoAbsolutely this. See https://google.github.io/guava/releases/23.0/api/docs/src-html/com/google/common/util/concurrent/ListenableFuture.html#line.141 https://google.github.io/guava/releases/23.0/api/docs/src-ht... For a discussion on some of the dangers of invoking completion listeners synchronously.