async: separate cancelation from exceptions #32
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a big one, I'm happy to walk through the module face-to-face sometime if you'd like.
Essentially,
cancelshould not be an exception for a couple of minor reasons:trywill discard a cancelation signal and the code may carry on as a leaked strandBut also one big reason: it should be possible to defer cancelation. This allows code to implement "critical sections" where they won't get canceled. You don't need this for exceptions because your critical section should be written carefully not to throw exceptions, but it's not possible to write uncancelable code (unless you forbid yielding, which isn't practical).
The existing code allowed this via individual callbacks (using
cancelable=False), but using that requires dealing with callbacks. Now you can wrap an arbitraryasyncexpression indefer-cancelationand know that the block won't be interrupted in the middle, cancelation will be delayed until after the block has completed.There's precedence for these cancelation semantics, the one I'm most familiar with is cats-effect, there's an interesting thread here going over the rationale for this model.
I've added fairly extensive tests so I'm fairly confident in the new semantics 🤞. And I've added comments to explain the reasoning of why things are structured in a certain way, thus the rather high line-count.
There's one outstanding issue I haven't been able to solve, which is that
asyncxshould not includest<global>. I mentioned this in discord, but I haven't been able to make a more minimal reproduction yet to demonstrate the issue.