Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AsyncEx.AwaitTask cancellation #46

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Conversation

TheAngryByrd
Copy link
Owner

Proposed Changes

Fixes #24 (comment) per @bartelink

Types of changes

What types of changes does your code introduce to IcedTasks?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • Build and tests pass locally
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@TheAngryByrd TheAngryByrd force-pushed the fix-asyncex-cancellation branch from 9b66bf4 to a399ec8 Compare April 22, 2024 23:31
@TheAngryByrd TheAngryByrd enabled auto-merge (squash) April 22, 2024 23:32
@TheAngryByrd TheAngryByrd disabled auto-merge April 22, 2024 23:33
@TheAngryByrd TheAngryByrd merged commit e720a0e into master Apr 22, 2024
8 of 9 checks passed
@TheAngryByrd TheAngryByrd deleted the fix-asyncex-cancellation branch April 22, 2024 23:41
TheAngryByrd added a commit that referenced this pull request Apr 22, 2024
## [0.11.5] - 2024-04-22

### Fixed
- [Fix AsyncEx.AwaitTask cancellation](#46) - Credits @TheAngryByrd
@@ -811,7 +811,68 @@ module PolyfillTest =
let! result = outer
Expect.equal result () "Should return the data"
}

let withCancellation (ct: CancellationToken) (a: Async<'a>) : Async<'a> =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something seems iffy here....

This sort of thing can be done much more cleanly decomposing into 3 bits:-

https://github.com/search?q=repo%3Ajet%2Fpropulsion%20runWithCancellation&type=code
https://github.com/jet/propulsion/blob/4f061bfa5efe63a8fa8270c4495498754ebcb196/src/Propulsion/Internal.fs#L128-L130
https://github.com/jet/propulsion/blob/4f061bfa5efe63a8fa8270c4495498754ebcb196/tests/Propulsion.Kafka.Integration/ConsumersIntegration.fs#L29-L31

  1. an operation to kick off a child computation or computations with a controlling CT va the Linked CTS
    let inline runWithCancellation (ct: CancellationToken) ([<InlineIfLambda>]f: CancellationToken -> Task) = task {
        use cts = System.Threading.CancellationTokenSource.CreateLinkedTokenSource(ct) // https://stackoverflow.com/questions/6960520/when-to-dispose-cancellationtokensource
        try do! f cts.Token
        finally cts.Cancel() }
  1. An operation to call a child from an Async, awaiting completion correctly:
module Async =

    let ofUnitTask (t: Task) = Async.AwaitTaskCorrect t
    let ofTask (t: Task<'t>) = Async.AwaitTaskCorrect t

    let inline call (start: CancellationToken -> Task<'T>): Async<'T> = async {
        let! ct = Async.CancellationToken
        return! start ct |> ofTask }
    let inline callUnit (start: CancellationToken -> Task): Async<unit> = async {
        let! ct = Async.CancellationToken
        return! start ct |> ofUnitTask }
  1. Some CancelAfter mechanism to cancel the CT
    type CancellationTokenSource with
        member cts.StopAfter(delay: TimeSpan) =
            Task.Delay(delay).ContinueWith(fun (_: Task) -> cts.Cancel()) |> ignore

I don't actually get what this overal block is trying to accomplish because there's too much going on (and I'm not in your head properly)....

Given these primitives, I think you can express this more cleanly

The main thing for me is that the Async.AwaitTaskCorrect impls already correctly manage all aspects of honoring the incoming CT:

  • yielding an exception on cancellation e.g. of a sleep
  • unwrapping AggregateException

The key thing this means is that any leg of an Async.Parallel throwing (or being cancelled) will

  • emit an exception
  • trigger cancellation of the siblings

I am not sure if you have tests or coverage for that but that's my litmus test (and I only see you doing negative tests about there not being weird mappings when you do normal things)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the cleanup in general of course - my main concern is just that I trust AwaitTaskCorrect as Just Working so I want to lean on that to the max degree possible

If you can build your wrapping and dependencies on it, then we just need to get those two into Core and your stuff, TaskSeq and Propulsion just depend only on those to wrap Task as Async with correct semantics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants