PoC onCancelRequested#4633
Open
reardonj wants to merge 26 commits into
Open
Conversation
Basic implementation of onCancelRequested for IO. This version uses existing IO combinators instead of inlining the entire implementation in IOFiber. It also runs the `ack` asynchronously as that was easier to implement. Also adds a `Fiber.joinOrCancel` implementation based on this new method.
A fiber can be canceled before it starts. Use a Deferred to make sure we have registered the onCancelRequested callback before cancelling the fiber
You cannot acknowledge cancelation after finalization has started. Which makes perfect sense in hindsight. This can happen if you register an `onCancelRequested` handler inside a `onCancel` handler, which is utterly uncancelable, which seems silly, but happened after I changed `Async.both` to use `joinOrCancel` This comes up in `Resource.both`, which uses `F.both` to run finalizers, which the `Async` implementation of uses the `join.onCancel(cancel)` construct. Changing `both` to use `joinOrCancel` means the implementation observed cancelation inside `onCancel`, at which point we are obviously never canceling anything. `both` is fundamentally lossy, but `joinOrCancel` will at least eliminate edge cases where the user might be able to observe that both sides obviously completed, but it still got canceled. The fix is a simple addition to the `acknowledgeCancelation` checks.
`par` is the prefix used elsewhere in CE to indicate when actions can happen concurrently, and `async` has a specific meaning: the async FFI.
Same race condition as cancelation after suspending to await the callback, if suspended then needs to ack we need to try resuming as well.
The change in raceOutcome behavior causes this to actually return a result when the fiber is canceled now, and the flatMap is evaluated to the next IO before cancelation actually takes effect, triggering a crash from the sys.error side effect. Wrapping it so it is properly suspended then ignored on cancelation.
If the fiber is canceled at this point, the ack isn't getting popped anyways.
This failed on one build for no particular reason, which I suspect is a CI issue and not an issue with the changes to race
- Don't spawn an extra fiber to cancel - poll when the first side of race cancels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Basic implementation of onCancelRequested for IO.
This version uses existing IO combinators instead of inlining the entire implementation in IOFiber.It also runs theackasynchronously asthat was easier to implement.the alternative would end up involving something that looks suspiciously like a stack of fiber stacks, since anonCancelRequestedhandler could have anotheronCancelRequestedinside it. Just starting a new fiber for each feels much safer.Also adds a
Fiber.joinOrCancelimplementation based on this new method.Fixes #4620
TODOs
async*combinators, or should the current combinators move to the new design? I broke a test trying to migratefromFutureCancelableas the test scenario never actually killed the fiber. I'm sure there are other bad usages ofasync*in real code, so I am hesitant to just update the existing combinators. I don't think we need something likeasyncFullwith bothonCancelRequestedandonCancelcallbacks, since one way or another the FFI should be done after the callback completes.