-
Notifications
You must be signed in to change notification settings - Fork 18
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
Sync with canonical Async.AwaitTaskCorrect #49
Sync with canonical Async.AwaitTaskCorrect #49
Conversation
NOTE there are two behavior changes here:
Ran this past @eiriktsarpalis; he confirmed there is no useful variation in the impl herein ASIDE: Having a immediate release for this is not critical for me as I'm comfortable there is no consequential behavior change that I need - hopefully I'll get around to doing cleanup on |
4c52d74
to
1513b51
Compare
1513b51
to
1390edf
Compare
static member AwaitTaskCorrect(task : Task<'T>) : Async<'T> = | ||
Async.FromContinuations(fun (sc,ec,cc) -> | ||
task.ContinueWith(fun (t : Task<'T>) -> | ||
if task.IsFaulted then ec t.InnerException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I glanced at this in analyzing what became dotnet/fsharp#13165, I went down a rathole of what would happen if this threw and/or returned null
, and what that might lead to within Async.FromContinuations
. For this reason I feel it's best to have this inlined and straightforward (along with the other benefits of it being identical to the canonical version)
if e.InnerExceptions.Count = 1 then ec e.InnerExceptions[0] | ||
else ec e | ||
elif task.IsCanceled then | ||
ec (TaskCanceledException()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note intentional swapping to ec
instead of cc
, and a different exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay to merge this, agree it’s a good idea to update to the latest version. Tests pass, yolo.
In isolating dotnet/fsharp#13165, the fact that the
AwaitTaskCorrect
impl herein predates the current fsssnip edition, and that thetask.InnerException
is subtle led me down additional paths that would be nice to avoid, especially given that ultimately there should be convergence in the implementations via fsharp/fslang-suggestions#840.