feat(Async): RunSynchronouslyImmediate#19804
Conversation
❗ Release notes required
|
181ce94 to
41f5f13
Compare
d6a0470 to
69e41a2
Compare
There was a problem hiding this comment.
Pull request overview
Adds Async.RunSynchronouslyImmediate to FSharp.Core to allow running an Async<'T> synchronously while always executing the initial step on the calling thread (aimed at improved diagnostics/stack traces in FSI/tests), with accompanying API surface updates, documentation, unit tests, and release notes.
Changes:
- Add public API
Async.RunSynchronouslyImmediateand wire it through Async primitives. - Add unit tests characterizing basic behavior and key differences vs
Async.RunSynchronously. - Update FSharp.Core surface area baselines and release notes; extend XML docs for
RunSynchronously/RunSynchronouslyImmediate.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs | Adds unit tests for RunSynchronouslyImmediate and contrasts with RunSynchronously. |
| tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.release.bsl | Records new public surface area entry for RunSynchronouslyImmediate. |
| tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard21.debug.bsl | Records new public surface area entry for RunSynchronouslyImmediate. |
| tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard20.release.bsl | Records new public surface area entry for RunSynchronouslyImmediate. |
| tests/FSharp.Core.UnitTests/FSharp.Core.SurfaceArea.netstandard20.debug.bsl | Records new public surface area entry for RunSynchronouslyImmediate. |
| src/FSharp.Core/async.fsi | Adds XML docs for the new API and revises RunSynchronously docs to reference it. |
| src/FSharp.Core/async.fs | Implements the new API by exposing an “immediate” synchronous runner and refactoring RunSynchronously internals. |
| docs/release-notes/.FSharp.Core/11.0.100.md | Adds release note entry for Async.RunSynchronouslyImmediate. |
Comments suppressed due to low confidence (1)
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs:527
- This test uses a raw Thread but doesn’t capture exceptions from the thread body. Any unexpected exception inside the thread (including from Async.RunSynchronously) will be unhandled and may crash the test process rather than reporting a normal xUnit failure. Capture exceptions in the thread and rethrow/assert after Join.
let t = Thread(fun () ->
callerThreadId <- Thread.CurrentThread.ManagedThreadId
async { runSyncThreadId <- Thread.CurrentThread.ManagedThreadId }
|> Async.RunSynchronously
async { immThreadId <- Thread.CurrentThread.ManagedThreadId }
|> Async.RunSynchronouslyImmediate)
| /// <summary><p>Runs the asynchronous computation, starting and blocking on the | ||
| /// calling thread regardless of <see cref="P:System.Threading.SynchronizationContext.Current"/> being non- | ||
| /// <c>null</c> or <see cref="P:System.Threading.Thread.IsThreadPoolThread"/> being <c>false</c>.</p> | ||
| /// <p>Any exception raised by the computation is propagated to the caller, with a stack trace bearing | ||
| /// caller frames for exceptions raised before the first asynchronous suspension.</p> | ||
| /// <p>Warning: may cause deadlock if called on a UI thread.</p> | ||
| /// </summary> | ||
| /// | ||
| /// The timeout parameter is given in milliseconds. A value of -1 is equivalent to | ||
| /// <see cref="F:System.Threading.Timeout.Infinite"/>. | ||
| /// <remarks> | ||
| /// <p>Warning: this method hard-blocks the calling thread for the duration of the computation, | ||
| /// including threads that have a non-<c>null</c> | ||
| /// <see cref="P:System.Threading.SynchronizationContext.Current"/> such as UI threads. Calling it | ||
| /// from a UI thread will make the UI unresponsive and risks deadlock if any continuation in the | ||
| /// computation needs to be dispatched back to that context. | ||
| /// </p> |
There was a problem hiding this comment.
Summary and remarks feel very much like.
Maybe summary = launches on the calling thread and what it is good for
remarks - the exact conditions and risks
?
There was a problem hiding this comment.
@T-Gro see #19804 (comment) - I'll rewrite the xmldoc based on what we decide. I agree that there should be less/no duplication, and summary should mention only risk (deadlock), especially if only key reason to use new fn is to get a direct callstack in a debugger.
There was a problem hiding this comment.
Heavily revised now - trying to stick to facts and being terse as the value prop for one vs the other doesn't feel slam dunk to me. Open to concrete suggestions!
(NOTE screenshot of rendered version in OP)
| /// </code> | ||
| /// Prints "A", "B" immediately, then "C", "D" in 1 second. result is set to 17. | ||
| /// Prints <c>"A"</c>, <c>"B"</c> immediately (on the calling thread), | ||
| /// then (from a continuation thread), after about one second, <c>"C"</c>, |
There was a problem hiding this comment.
Isn't the example commentary ((from a continuation thread)) swapped between the two versions?
There was a problem hiding this comment.
I'll double check after the rewrite when my comment in main thread is resolved - lots of cut and paste going on. Assuming the key diff is deemed to be less noise in the stack trace, the examples will emphasize that the first block prior to suspension runs on calling thread vs a threadpool thread
There was a problem hiding this comment.
switched to conveying via EOL comments. Wordings can definitely be improved and/or tightened so open to any suggestions
(NOTE screenshot of rendered version in OP)
|
@T-Gro I've implemented as per OP of fsharp/fslang-suggestions#1042 Firstly, in a debugger context a first chance exception breakpoint has more direct causation vs having to disentangle an adjacent thread waiting, which tooling may or may not be able to convey unaided. However, while the exception stack trace has less noise, it's not significantly better AFAICT:
It seems the TL;DR value prop is:
But its not a slam dunk as:
This is making me think I should dial back the xmldoc from where I have it trying to set a "use the new RunSynchronouslyImmediate in FSI from now on" vibe (as I've tried to do with AwaitTask vs Await where it is a slam dunk) |
| |> unfake | ||
|
|
||
| [<DebuggerHidden>] | ||
| let RunSynchronouslyImmediate2 computation (cancellationToken: CancellationToken) = |
There was a problem hiding this comment.
@T-Gro If I put this body (which gives a shorter callstack than the original RunSynchronouslyImmediate impl at L1141) into RunSynchronouslyImmediate, the tests also pass but I guess there is more risk of regressions due to changing a very hot path.
Let me know if you'd prefer for RunSynchronouslyImmediate2 and RunSynchronouslyImmediate to live on side by side as the code is now, or whether you're ok with me collapsing the two into this impl
There was a problem hiding this comment.
(applied now - can revert if you think it's a step too far)
75fed30 to
f9e0383
Compare
| ) | ||
|
|
||
| [<DebuggerHidden>] | ||
| let StartWithContinuations cancellationToken (computation: Async<'T>) cont econt ccont = |
There was a problem hiding this comment.
moved up as new RunSynchronouslyImmediate needs it (putting it there as the diffs look even more confusing if I put it as far down the file as possible)
| let RunSynchronouslyImmediate<'T> computation (cancellationToken: CancellationToken) = | ||
| let tcs = TaskCompletionSource<'T>() | ||
|
|
||
| computation.Invoke ctxt) | ||
| |> unfake | ||
|
|
||
| let res = resultCell.TryWaitForResultSynchronously().Value | ||
| res.Commit() | ||
| StartWithContinuations | ||
| cancellationToken | ||
| computation | ||
| tcs.SetResult | ||
| (fun edi -> tcs.SetException edi.SourceException) | ||
| (fun _ -> tcs.SetCanceled()) | ||
| // Synchronously block waiting for the result (i.e. even if continuations run on another thread, this caller will busy-wait) | ||
| tcs.Task.GetAwaiter().GetResult() // GetResult() unpacks the AggregateException that .Result would present | ||
|
|
||
| [<DebuggerHidden>] | ||
| let RunSynchronously cancellationToken (computation: Async<'T>) timeout = | ||
| // Reuse the current ThreadPool thread if possible. | ||
| let RunSynchronouslyBackgroundThreadPool (computation: Async<'T>) cancellationToken timeout = | ||
| // Run inline only where it's guaranteed to be safe | ||
| match SynchronizationContext.Current, Thread.CurrentThread.IsThreadPoolThread, timeout with | ||
| | null, true, None -> RunImmediate cancellationToken computation | ||
| | _ -> QueueAsyncAndWaitForResultSynchronously cancellationToken computation timeout | ||
| | null, true, None -> RunSynchronouslyImmediate computation cancellationToken // best stacktrace in case of exception |
There was a problem hiding this comment.
@T-Gro this is the key implication of settling on a uniform simplified impl of RunSynchronouslyImmediate shared between it and [the no-thread-hop path of] RunSynchronously
IMO If we roll it back, we get messier stack traces and uglier code so I'm suggesting we take this inconsistency hit.
| /// Run the asynchronous workflow and wait for its result. | ||
| [<DebuggerHidden>] | ||
| let QueueAsyncAndWaitForResultSynchronously (token: CancellationToken) computation timeout = | ||
| let QueueAsyncAndWaitForResultSynchronously computation (token: CancellationToken) timeout = |
There was a problem hiding this comment.
can revert this, but trying to get arg order consistent across most involved functions
| trampolineHolder | ||
| (fun res -> resultCell.RegisterResult(AsyncResult.Ok res, reuseThread = true)) | ||
| (fun edi -> resultCell.RegisterResult(AsyncResult.Error edi, reuseThread = true)) | ||
| (fun exn -> resultCell.RegisterResult(AsyncResult.Canceled exn, reuseThread = true)) |
There was a problem hiding this comment.
old behavior resulting in OperationCanceledException
| computation | ||
| |> unfake | ||
|
|
||
| [<DebuggerHidden>] |
There was a problem hiding this comment.
moved up as required by RunSynchronously
note the whole PR could be made much shorter by having the Async layer call Async.FromContinuations
However, as mentioned in other places, I believe its for the best that RunSynchronouslyImmediate and the immediate path of RunSychronously should have strong clear ties in the lower layer as this PR does
| Assert.NotEqual(callerThreadId, runSyncThreadId) | ||
| Assert.Equal(callerThreadId, immThreadId) | ||
|
|
||
| [<Fact>] |
There was a problem hiding this comment.
I'm not particularly attached to this test, and it can be argued that it's a red herring/aside/distraction


Implements
RunSynchronouslyImmediateper fsharp/fslang-suggestions#1042See also fsharp/fslang-suggestions#1467
Heavily revises the xmldoc for
RunSynchronouslyin order to convey the tradeoffs involved in selecting between the new and the old.Updated

RunSynchronously:NEW

RunSynchronouslyImmediate:NOTE xmldocs are intended to convey a nuanced message as explained in detail in #19804 (comment):
AsyncResult.CommitandQueueAsyncAndWaitForResultSynchronouslyChecklist
Async.RunSynchronously