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

Add Async.AwaitTask overloads which helps with CancellationToken passing, and a new warning #1284

Open
5 tasks done
jwosty opened this issue Jul 5, 2023 · 4 comments
Open
5 tasks done

Comments

@jwosty
Copy link
Contributor

jwosty commented Jul 5, 2023

I propose we add two Async.AwaitTask overloads:

  • Async.AwaitTask(makeTask: CancellationToken -> Task<'a>) -> Async<'a>
  • Async.AwaitTask(makeTask: CancellationToken -> Task) -> Async<unit>

I also propose that an accompanying compiler warning be added. In situations where the task method could have been provided a CancellationToken (either via an optional parameter or a separate overload), the compiler should emit a warning.

The warning could say something like: "This task-returning method can accept a CancellationToken (through an overload or optional parameter), but is not provided one. Please either use Async.AwaitTask(makeTask: CancellationToken -> Task<'a>), or capture and provide a CancellationToken manually."

Surely there are better wordings for such a warning; improvements are welcome.

For example, the following, which forgets to pass a CancellationToken, would now emit a warning:

let doWorkBad () = async {
    do! Async.AwaitTask (File.WriteAllTextAsync ("file.txt", "Hello, world!"))
}

To make the warning go away, you could use the new overload:

let doWorkGood () = async {
    do! Async.AwaitTask (fun ct -> File.WriteAllTextAsync ("file.txt", "Hello, world!", ct))
}

Or capture and pass it yourself:

let doWork () = async {
    let! ct = Async.CancellationToken
    do! Async.AwaitTask (File.WriteAllTextAsync ("file.txt", "Hello, world!", ct))
}

The existing way of approaching this problem in F# is ...

For the new method, you can write your own extensions today:

type Async =
    static member AwaitTask (makeTask: CancellationToken -> Task<'a>) = async {
        let! ct = Async.CancellationToken
        let! result = Async.AwaitTask (makeTask ct)
        return result
    }
    static member AwaitTask (makeTask: CancellationToken -> Task) = async {
        let! ct = Async.CancellationToken
        let! result = Async.AwaitTask (makeTask ct)
        return result
    }

Pros and Cons

The advantages of making this adjustment to F# are:

  • Makes it harder to forget to pass a CancellationToken when bridging from Async to Task code, which is a common source of bugs
  • Makes it easier to do the actual passing of the CancellationToken (reduces common boilerplate)

The disadvantages of making this adjustment to F# are:

  • More library and compiler complexity

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@vzarytovskii
Copy link

I personally don't think that emitting warning from compiler is a good idea, it makes it aware of a very specific type as well as twisting the promise of optional parameters (i.e. Optional parameters are optional, except CancellationToken).

This, however, should be a great analyzer for when we will be working on analyzers sdk.

@jwosty
Copy link
Contributor Author

jwosty commented Jul 5, 2023

I personally don't think that emitting warning from compiler is a good idea, it makes it aware of a very specific type as well as twisting the promise of optional parameters (i.e. Optional parameters are optional, except CancellationToken).

This, however, should be a great analyzer for when we will be working on analyzers sdk.

What is your opinion of just the additional overloads? Those could be added without the warning.

@bartelink
Copy link

TL;DR the Async.AwaitTask semantics are problematic, and adding more operations that implicitly couple to those semantics is therefore something that should not proceed until that's resolved.


I arrived at the same thing, with the name Async.call: https://github.com/jet/propulsion/blob/master/src/Propulsion/Internal.fs#L102-L107 https://github.com/jet/equinox/blob/master/src/Equinox.Core/Infrastructure.fs#L54-L58

I don't love the name call, but on balance, I feel it's bad news to overload the AwaitTask name, especially given its problematic semantics. For me, the best way to resolve the forces is to first get corrected AwaitTask operators (as described in fsprojects/FSharp.Control.TaskSeq#141) into a library before doing this work. Because the semantics differ from the AwaitTask ones, it will likely end up with a new name. Then, when that's done, the 'run a Task, passing the CT' operation can be implemented in terms of that.

Also, as noted in fsprojects/FSharp.AWS.DynamoDB#65, anything that can assist with making correct propagation of CTs the unobtrusive non-issue it is in F# app code bases would be very welcome (Even considering making a CT argument mandatory for the Task layer in FSharp.AWS.DynamoDB in order to force the issue). However I'd say that in practice, the actual calls of Task-returning methods are many levels deep into libraries (probably in an area that is using the task builder) - and hence forwarding the CTs through the layers can be a messy change that's rarely as neat as the presented 'to make the warning go away' example. Based on that, I'd also agree that any solution should be a language agnostic thing that does not belong in any compiler

@Frassle
Copy link

Frassle commented Jul 9, 2023

There's a long standing issue in the fsharp repo about this problem: dotnet/fsharp#2127

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

No branches or pull requests

4 participants