feat(Async): Add exception-unwrapping Await#19785
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
b2b2b6a to
b710cc1
Compare
|
@T-Gro if you and/or others can give this a quick scan please, I'd like to confirm:
TL;DR the overall proposition
CC @TheAngryByrd @gusty @njlr who have provided useful feedback/review on stuff in this space in recent times |
There was a problem hiding this comment.
Pull request overview
Adds a new Async.Await API to FSharp.Core for Task/Task<'T> (and ValueTask/ValueTask<'T> on netstandard2.1) that unwraps “egregious” single-inner AggregateExceptions so exception matching works as expected, while aiming to preserve existing AwaitTask stacktrace behavior.
Changes:
- Implement
Async.Awaitinasync.fsby sharing the existingAwaitTaskcontinuation machinery and selectively unwrapping single-innerAggregateExceptions. - Expand unit tests to exercise both
AwaitTaskandAwaitbehavior (including AggregateException cases) and add ValueTask coverage under#if NETSTANDARD2_1. - Update FSharp.Core surface area baseline (partially) and add a release-notes entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs | Converts many tests to run against both AwaitTask and new Await; adds new behavior-focused tests for AggregateException unwrapping and ValueTask. |
| tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.debug.bsl | Adds Async.Await entries for netstandard2.1 Debug surface area baseline. |
| src/FSharp.Core/async.fsi | Updates AwaitTask XML docs and adds new Async.Await API docs (including ValueTask overloads under netstandard2.1). |
| src/FSharp.Core/async.fs | Implements Async.Await and refactors task-await internals to optionally unwrap single-inner AggregateExceptions. |
| docs/release-notes/.FSharp.Core/11.0.100.md | Adds release note entry for new Async.Await. |
7aec92d to
9ab202c
Compare
|
I'm thinking about naming. |
@majocha Yes, this was already part of the brief as per the comment from @dsyme. I'd personally need to research what it would entail, so if you or anyone wants to contribute an impl, feel free to hang a PR off this one. But bottom line I agree it would be good for the support for tasklike things to be done either as part of this PR or as a very fast follow so the world only needs to validate the overloading works out cleanly once. |
a887f40 to
885c40f
Compare
. @majocha Added a commit:
|
93badc9 to
50814f5
Compare
4b7deb3 to
7055921
Compare
| econt e | ||
| else | ||
| (awaiter :> ICriticalNotifyCompletion) | ||
| .UnsafeOnCompleted(fun () -> |
There was a problem hiding this comment.
UnsafeOnCompleted skips ExecutionContext capture/restore; the typed overloads flow it via ContinueWith.
type Wrapper<'T>(t: Task<'T>) = member _.GetAwaiter() = t.GetAwaiter()
let al = AsyncLocal<string>()
async { al.Value <- "trace-id"
let! _ = Async.Await(Wrapper someIncompleteTask)
return al.Value }- typed
Async.Await→"trace-id" - this SRTP overload →
null - lost:
AsyncLocal,Activity.Current, logging scopes
Verified at BCL level: UnsafeOnCompleted runs the continuation with the completing thread's context; OnCompleted captures at subscribe and restores. The awaiter is :> ICriticalNotifyCompletion : INotifyCompletion, so:
- (awaiter :> ICriticalNotifyCompletion)
- .UnsafeOnCompleted(fun () ->
+ (awaiter :> INotifyCompletion)
+ .OnCompleted(fun () ->There was a problem hiding this comment.
Applied (and regression test added)
Aside: given that ICriticalNotifyCompletion is now no longer being used, the type expectation could drop to INotifyCompletion (though that would be inconsistent with every other SRTP await helper we have)
|
|
||
| if (^Awaiter: (member get_IsCompleted: unit -> bool) awaiter) then | ||
| try | ||
| cont ((^Awaiter: (member GetResult: unit -> 'T) awaiter)) |
There was a problem hiding this comment.
Multi-inner faults surface differently depending on which Async.Await binds:
let tcs = TaskCompletionSource<int>()
tcs.SetException [ ArgumentException(); InvalidOperationException() ]
Async.Await tcs.Task // typed
Async.Await (Wrapper tcs.Task) // SRTP- typed →
AggregateException(UnwrapExnunwraps onlyCount=1) - SRTP →
ArgumentException(rawGetResult(), first inner)
Full alignment isn't possible for arbitrary awaiters, so just document it — append to the SRTP <remarks> in async.fsi:
/// with <c>IsCompleted</c> and <c>GetResult()</c> members. Exceptions thrown by <c>GetResult()</c> are
- /// propagated directly.
+ /// propagated directly. Unlike the <see cref="T:System.Threading.Tasks.Task"/> overloads, an
+ /// <see cref="T:System.AggregateException"/> carrying multiple inner exceptions is not preserved:
+ /// the first inner exception surfaces (standard <c>GetResult()</c> semantics).| /// re-raised as-is. For the legacy behavior of uniformly presenting the raw underlying | ||
| /// <see cref="T:System.AggregateException"/>, use <c>Async.AwaitTask</c>. | ||
| /// | ||
| /// If the task is canceled then <see cref="T:System.Threading.Tasks.TaskCanceledException"/> is raised. |
There was a problem hiding this comment.
The ambient-cancellation caveat that AwaitTask carries was dropped from the new Await remarks (here and at 847/877/905). Await is the recommended default, and it deliberately ignores the ambient CT (tested at AsyncType.fs:548), so users need this guidance:
/// If the task is canceled then <see cref="T:System.Threading.Tasks.TaskCanceledException"/> is raised.
+ /// Note the task may be governed by a different cancellation token to the overall async computation;
+ /// normally start it with the token from <c>let! ct = Async.CancellationToken</c>, and catch
+ /// <see cref="T:System.Threading.Tasks.TaskCanceledException"/> where the overall async is started.There was a problem hiding this comment.
Done with some rewording in e1d0708.
This text should also reference Async.StartTaskImmediate when that lands.
|
I noticed I was owing a few replies here, I apologize for taking a longer time.
Agreed, and worth stating explicitly beyond the approval: deferring #2127 and matching
Keep all four. The concrete overloads give stacktrace parity, Let me know if any of the other questions from earlier are still open on your side — happy to pick up anything I've skipped. |
Thanks @T-Gro for the great review work - I should get around to addressing them all in the next few days and will @ you on any follow-ups |
|
@T-Gro Thanks for the thorough review, and especially for catching the erroneous Let me know if any of my doc changes overreach One potential clone noted: #19785 (comment) Please mark any threads as resolved; I'll address any follow-ups you desire later and/or tomorrow |
Implements
Async.AwaitforTask,Task<'T>,ValueTaskandValueTask<'T>(aka a polished version of thecommunity
AwaitTaskCorrectimplementation). Includes a fallback SRTP based type augmentation that uses theGetAwaiterprotocol to support custom waits a la C#await.Key differentiation from
Async.AwaitTaskis thatAggregateExceptions are unwrapped such that atry ... with <ExceptionType> ->will type-match correctly.Key distinction from the canonical implementation (which derives from https://www.fssnip.net/7Rc/title/AsyncAwaitTaskCorrect) is that the implementation is intended to have 1:1 matching of all stacktrace preservation properties borne by
AwaitTask(and continue to track that over time).NOTE one key implementation decision is that this PR does NOT attempt to resolve #2127 so:
Awaitis invoked, normal cancellation semantics as perAwaitTaskapply:OperationCanceledExceptionTaskin question'sResultwill go unobservedAwait, it will (likeAwaitTask):Taskawaitwould] until such time as theTaskcompletes (either successfully, with a fault, or via cancellation)Checklist
AwaitTaskwill always yield anAggregateExceptionAwaitshould be the used in preference for new codeAwaitcan technically still propagate anAggregateException