nbrooke
commented
3 years ago I think making it on by default for background tasks only does make some sense for fixing the most dangerous cases without causing any problems in existing code, but that is a little bit more complicated (we don't track that right now, though it wouldn't be super hard to).
This feature is also, I would argue, still useful for the non-background task case, so we probably still want the flag to switch between "background task only" and "everything". In particular, it's actually quite tricky to write any code that is conceptually "waiting" on something to finish (even if asynchronously, not actually blocked) without this because you have to have pretty specific knowledge of how to unwind things.
For example, consider the following pseudocode:
class SomeViewController {
func doOperation() {
showHud()
bluejay.listen(characteristicA) {
hideHud()
actOnResult($0)
}
}
}This operation isn't blocking in a threading sense, but it is UI blocking. Question is: how do we hide the hud if we disconnect while listening? With this change (and broadcasting errors enabled), that'll just happen with this code, because the listen will get called with an BluejayError.notConnected when the disconnect happens.
Without this, you need to implement it like this:
class SomeViewController: DisconnectHandler {
func viewDidLoad() {
bluejay.registerDisconnectHandler(self)
}
func doOperation() {
showHud()
bluejay.listen(characteristicA) {
hideHud()
actOnResult($0)
}
}
func didDisconnect(/*...*/) -> AutoReconnectMode {
hideHud()
}
}Which is more complicated and more error prone (what if you change the work done in the listen but not in the cleanup? What if the operation being done isn't something that has a trivial cleanup function to call, like it's got a closure passed in to the original function to call back or something? What if there are multiple UI blocking operations in the same class with different cleanup?).
I'm inclined to leave this as is for now and do that as a follow up, since I'd definitely want to use it in the "notify everything" mode for now, and I think we'd want to do some more careful testing even of the background mode before we turned it on by default, whereas this change as is keeps me going with eh current problem while being super low risk.
Did realize when double checking this that I HAD actually enabled it by default by not checking the flag, so have fixed that now.
sakuraehikaru
Fixes #239 (also helps with workarounds for #240):
Summary of Problem:
As detailed in #239, when you are in a background task, there are currently cases where you can end up unable to exit the task when you are in a blocking listen either due to a disconnection or (also) intentionally via calling cancelEverything, because none of the errors generated by that will propagate to the listeners, which we can be blocked on. This behaviour also causes problems with implementing your own timeouts to work around #240 (which is hard to fix within Bluejay due to the behaviour of the semaphore timeouts), because there is no way to implement a timeout at the application level if calling cancelEverything will not actually cancel everything.
Proposed Solution:
Add an option to allow errors sent in to cancelEverything to be propagated to all current listeners, this will allow background tasks that are blocked on listeners to be intentionally cancelled via calling cancelEverything, and also implicitly handles unintentional disconnects (which also end up calling cancelEverything).
Although I think this setting probably should be the default, it could be very disruptive to anything doing non-background-task listens that wasn't written with this in mind, becasue you could get lots of spurious errors on the listeners, so having it explicitly enabled seems like the best approach for now.