-
Notifications
You must be signed in to change notification settings - Fork 869
feat(Async+Task+ValueTask): consistent helper modules #19844
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
base: main
Are you sure you want to change the base?
Changes from all commits
a21b6c9
d2e4b46
bc83170
40245a2
6708b8f
bb57111
60f399e
cfec168
ac1ad45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -716,3 +716,167 @@ module LowPlusPriority = | |||||
| this.Bind(computation, fun (result2: ^TResult2) -> this.Return struct (result1, result2)) | ||||||
| ) | ||||||
| ) | ||||||
|
|
||||||
| namespace Microsoft.FSharp.Control | ||||||
|
|
||||||
| open System.Threading.Tasks | ||||||
| open Microsoft.FSharp.Core | ||||||
| open TaskBuilder | ||||||
| open Microsoft.FSharp.Control.TaskBuilderExtensions | ||||||
| open Microsoft.FSharp.Control.TaskBuilderExtensions.LowPriority | ||||||
| open Microsoft.FSharp.Control.TaskBuilderExtensions.HighPriority | ||||||
|
|
||||||
| [<RequireQualifiedAccess>] | ||||||
| [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>] | ||||||
| module Task = | ||||||
|
|
||||||
| [<CompiledName("Result")>] | ||||||
| let inline result (value: 'T) : Task<'T> = | ||||||
| Task.FromResult value | ||||||
|
|
||||||
| [<CompiledName("Empty")>] | ||||||
| let empty: Task<unit> = result () | ||||||
|
|
||||||
| [<CompiledName("Map")>] | ||||||
| let inline map ([<InlineIfLambda>] mapping: 'T -> 'U) (task: Task<'T>) : Task<'U> = | ||||||
| // if task.IsCompleted then // includes Canceled or Faulted states | ||||||
| // try result (task.GetAwaiter().GetResult() |> mapping) // Result would surface AggregateException | ||||||
| // with e -> Task.FromException<'U>(e) | ||||||
| // else | ||||||
| if task.Status = TaskStatus.RanToCompletion then | ||||||
| result (mapping task.Result) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
let boom (_: int) : int = failwith "boom"
tcs.Task |> Task.map boom // pending
Task.result 21 |> Task.map boom // completed
Proposed fix: if task.Status = TaskStatus.RanToCompletion then
try result (mapping task.Result)
with e -> Task.FromException<'U> e
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @T-Gro I get the point/concern on the catch variants - easy to test etc and will fix.
But can you clarify the exact desire re cancellation handling please? This code will use the if task.Status = TaskStatus.RanToCompletion then
result (mapping task.Result)
else
task {
let! v = task
return mapping v
}Recall Changing if task.IsCompleted then // includes Canceled or Faulted states
try result (task.GetAwaiter().GetResult() |> mapping) // Result would surface AggregateException
with e -> Task.FromException<'U>(e)
else
TaskBuilder.task {
let! v = task
return mapping v
}Current (passing with above impls) test semantics: [<Fact>]
let ``Task.map flows Cancellation (sync)`` () =
use cts = new CancellationTokenSource()
cts.Cancel()
let t = Task.FromCanceled<int>(cts.Token) |> Task.map (fun x -> x * 2)
task {
let! e = Assert.ThrowsAsync<TaskCanceledException>(fun () -> t)
Assert.Equal(cts.Token, e.CancellationToken)
}
[<Fact>]
let ``Task.map flows Cancellation (async)`` () =
let tcs = TaskCompletionSource<int>()
let t = tcs.Task |> Task.map (fun x -> x * 2)
use cts = new CancellationTokenSource()
tcs.SetCanceled cts.Token
task {
let! e = Assert.ThrowsAsync<TaskCanceledException>(fun () -> t)
Assert.Equal(cts.Token, e.CancellationToken)
}
[<Fact>]
let ``Task.map propagates exception (sync)`` () =
let t = Task.FromException<int>(Exception "boom") |> Task.map (fun x -> x * 2)
task {
let! e = Assert.ThrowsAnyAsync<exn>(fun () -> t)
Assert.Equal("boom", e.Message)
}
[<Fact>]
let ``Task.map propagates exception (async)`` () =
let tcs = TaskCompletionSource<int>()
let t = tcs.Task |> Task.map (fun x -> x * 2)
tcs.SetException(exn "boom")
task {
let! e = Assert.ThrowsAnyAsync<exn>(fun () -> t)
Assert.Equal("boom", e.Message)
}How would one specify/validate the precise semantics you seek? Is it about being able to set a breakpoint? Anything worth borrowing from prior art?: https://github.com/fsprojects/FSharpPlus/blob/master/src/FSharpPlus/Extensions/Task.fs#L15-L28
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @TheAngryByrd @gusty If either of you have time to throw a set of eyes over the impl and the test suite to see if there are any gaps that FsToolkit and/or FSharpPlus cover which should be considered? https://github.com/bartelink/fsharp/blob/atvt/src/FSharp.Core/tasks.fs#L729-L804 The aim is for a balance of:
Bottom line it would be good to rule out footguns like egregious
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @T-Gro Task test suite reviewed, expanded and polished NOTE |
||||||
| else | ||||||
| TaskBuilder.task { | ||||||
| let! v = task | ||||||
| return mapping v | ||||||
| } | ||||||
|
|
||||||
| [<CompiledName("Bind")>] | ||||||
| let inline bind ([<InlineIfLambda>] binder: 'T -> Task<'U>) (task: Task<'T>) : Task<'U> = | ||||||
| if task.Status = TaskStatus.RanToCompletion then | ||||||
| binder task.Result | ||||||
| else | ||||||
| TaskBuilder.task { | ||||||
| let! v = task | ||||||
| return! binder v | ||||||
| } | ||||||
|
|
||||||
| [<CompiledName("Ignore")>] | ||||||
| [<RequiresExplicitTypeArguments>] | ||||||
| let inline ignore<'T> (task: Task<'T>) : Task<unit> = | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The implementation in F#+ allows to convert a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm problem is that people want RequiresExplicitTypeArguments [and hence generic] here (Best discussed in fslang-sugggestion) |
||||||
| if task.Status = TaskStatus.RanToCompletion then | ||||||
| empty | ||||||
| else | ||||||
| map ignore task | ||||||
|
|
||||||
| [<CompiledName("CatchWith")>] | ||||||
| let inline catchWith ([<InlineIfLambda>] handler: exn -> 'T) (task: Task<'T>) : Task<'T> = | ||||||
|
bartelink marked this conversation as resolved.
|
||||||
| if task.Status = TaskStatus.RanToCompletion then | ||||||
| task | ||||||
| else | ||||||
| TaskBuilder.task { | ||||||
| try | ||||||
| return! task | ||||||
| with | ||||||
| | :? System.OperationCanceledException as e -> return! raise e | ||||||
| | e -> return handler e | ||||||
| } | ||||||
|
|
||||||
| [<CompiledName("Catch")>] | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Task.FromCanceled<int>(CancellationToken true) |> Task.catch
Proposed fix: TaskBuilder.task {
try
let! v = task
return Ok v
with
| :? OperationCanceledException as e -> return raise e // stays Canceled
| e -> return Error e
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (corrected; pushed for @T-Gro I guess map, bind, catch, catchWith should each have xmldoc covering the pinned behavior? i.e. I'm thinking that it should allude to the fact that |
||||||
| let catch (task: Task<'T>) : Task<Result<'T, exn>> = | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we kind of agreed that this function shouldn't be included for the reasons mentioned in the discussion. I'm not pretending to restart the discussions, but just looking at the votes. There are 3 "really don't want" against 1 "really want", plus 2 "I'm ok with it", there are more against that in favor.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| if task.Status = TaskStatus.RanToCompletion then | ||||||
| result (Ok task.Result) | ||||||
| else | ||||||
| TaskBuilder.task { | ||||||
| try | ||||||
| let! v = task | ||||||
| return Ok v | ||||||
| with | ||||||
| | :? System.OperationCanceledException as e -> return! raise e | ||||||
| | e -> return Error e | ||||||
| } | ||||||
|
bartelink marked this conversation as resolved.
|
||||||
|
|
||||||
| #if NETSTANDARD2_1 | ||||||
| [<CompiledName("OfValueTask")>] | ||||||
| let inline ofValueTask (valueTask: ValueTask<'T>) : Task<'T> = | ||||||
| valueTask.AsTask() | ||||||
| #endif | ||||||
|
|
||||||
| #if NETSTANDARD2_1 | ||||||
| [<RequireQualifiedAccess>] | ||||||
| [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>] | ||||||
| module ValueTask = | ||||||
|
|
||||||
| [<CompiledName("Result")>] | ||||||
| let inline result (value: 'T) : ValueTask<'T> = | ||||||
| ValueTask<'T>(value) | ||||||
|
|
||||||
| [<CompiledName("Map")>] | ||||||
| let inline map ([<InlineIfLambda>] mapping: 'T -> 'U) (task: ValueTask<'T>) : ValueTask<'U> = | ||||||
| if task.IsCompletedSuccessfully then | ||||||
| ValueTask<'U>(mapping task.Result) | ||||||
| else | ||||||
| let t: Task<'U> = | ||||||
| TaskBuilder.task { | ||||||
| let! v = task | ||||||
| return mapping v | ||||||
| } | ||||||
|
|
||||||
| ValueTask<'U>(t) | ||||||
|
|
||||||
| [<CompiledName("Bind")>] | ||||||
| let inline bind ([<InlineIfLambda>] binder: 'T -> ValueTask<'U>) (task: ValueTask<'T>) : ValueTask<'U> = | ||||||
| if task.IsCompletedSuccessfully then | ||||||
| binder task.Result | ||||||
| else | ||||||
| let t: Task<'U> = | ||||||
| TaskBuilder.task { | ||||||
| let! v = task | ||||||
| return! binder v | ||||||
| } | ||||||
|
|
||||||
| ValueTask<'U>(t) | ||||||
|
|
||||||
| [<CompiledName("Ignore")>] | ||||||
| [<RequiresExplicitTypeArguments>] | ||||||
| let inline ignore<'T> (task: ValueTask<'T>) : ValueTask<unit> = | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as for the Task version |
||||||
| map ignore task | ||||||
|
|
||||||
| [<CompiledName("CatchWith")>] | ||||||
| let inline catchWith ([<InlineIfLambda>] handler: exn -> 'T) (task: ValueTask<'T>) : ValueTask<'T> = | ||||||
| if task.IsCompletedSuccessfully then | ||||||
| task | ||||||
| else | ||||||
| let t: Task<'T> = | ||||||
| TaskBuilder.task { | ||||||
| try | ||||||
| return! task | ||||||
| with e -> | ||||||
| return handler e | ||||||
| } | ||||||
|
|
||||||
| ValueTask<'T>(t) | ||||||
|
|
||||||
| [<CompiledName("Catch")>] | ||||||
| let catch (task: ValueTask<'T>) : ValueTask<Result<'T, exn>> = | ||||||
| if task.IsCompletedSuccessfully then | ||||||
| ValueTask<Result<'T, exn>>(Ok task.Result) | ||||||
| else | ||||||
| let t: Task<Result<'T, exn>> = | ||||||
| TaskBuilder.task { | ||||||
| try | ||||||
| let! v = task | ||||||
| return Ok v | ||||||
| with e -> | ||||||
| return Error e | ||||||
| } | ||||||
|
|
||||||
| ValueTask<Result<'T, exn>>(t) | ||||||
|
|
||||||
| [<CompiledName("Empty")>] | ||||||
| let empty: ValueTask<unit> = result () | ||||||
|
|
||||||
| [<CompiledName("OfTask")>] | ||||||
| let inline ofTask (task: Task<'T>) : ValueTask<'T> = | ||||||
| ValueTask<'T>(task) | ||||||
| #endif | ||||||
Uh oh!
There was an error while loading. Please reload this page.