FSharp.Core: Add Async.RunImmediate(Async<'T>, CancellationToken) #14532

vzarytovskii · 2023-01-02T16:34:25Z

Do we need more tests around cancellation/exception handling?

Whitespace cleanup was intentional

T-Gro · 2023-01-02T17:27:10Z

You can also (or I can do it in a follow-up PR) replace:

~\tests\FSharp.Test.Utilities\Utilities.fs func RunImmediate
~\src\Compiler\Utilities\illib.fs func RunImmediate
~\src\Compiler\Facilities\BuildGraph.fs NodeCode.RunImmediate to use it (right now it used Async.StartImmediateAsTask(work, cancellationToken = ct).Result, which I believe attempted to do similar thing by starting on the same thread, but made exception handling worse)

EDIT:
~\vsintegration\src\FSharp.LanguageService\LanguageServiceConstants.fs also has it's version
~\tests\service\Common.fs also has it's version
and first branch of ~\vsintegration\src\FSharp.Editor\Common\Extensions.fs RunImmediateExceptOnUI as well.

… TCS

vzarytovskii · 2023-01-02T17:41:32Z

You can also (or I can do it in a follow-up PR) replace:

~\tests\FSharp.Test.Utilities\Utilities.fs func RunImmediate ~\src\Compiler\Utilities\illib.fs func RunImmediate ~\src\Compiler\Facilities\BuildGraph.fs NodeCode.RunImmediate to use it (right now it used Async.StartImmediateAsTask(work, cancellationToken = ct).Result, which I believe attempted to do similar thing by starting on the same thread, but made exception handling worse)

EDIT: ~\vsintegration\src\FSharp.LanguageService\LanguageServiceConstants.fs also has it's version ~\tests\service\Common.fs also has it's version and first branch of ~\vsintegration\src\FSharp.Editor\Common\Extensions.fs RunImmediateExceptOnUI as well.

We can't reuse it everywhere just yet, since FSHARPCORE_USE_PACKAGE exists (it's the case when FSharp.Compiler.Service solution is being built).
Probably, the safest will be to replace it when we ship new FSharp.Core to nuget.

vzarytovskii · 2023-01-02T17:43:34Z

src/FSharp.Core/async.fs

+    static member RunImmediate(computation: Async<'T>, ?cancellationToken: CancellationToken) =
+        let cancellationToken = defaultArg cancellationToken Async.DefaultCancellationToken
+        AsyncPrimitives.RunImmediate cancellationToken computation
+


Using ResultCell version here instead of TCS

T-Gro

Would be good to see tests showing the exception stack traces between RunImmediate and RumSynchronously.

vzarytovskii · 2023-01-03T12:27:10Z

Would be good to see tests showing the exception stack traces between RunImmediate and RumSynchronously.

Not sure I understand exactly what do you mean? Just to compare frames? Let me see if we already have such tests for existing run methods. There were no changes in how things are scheduled/executed.

KevinRansom · 2023-01-03T18:33:14Z

src/FSharp.Core/async.fs

@@ -1501,6 +1501,10 @@ type Async =

            computation.Invoke newCtxt)

+    static member RunImmediate(computation: Async<'T>, ?cancellationToken: CancellationToken) =


Probably should be preview right?

fsharp/src/FSharp.Core/prim-types.fsi

Line 2571 in bc88b51

[<Experimental("Experimental library feature, requires '--langversion:preview'")>]

psfinaki · 2023-01-04T13:25:41Z

src/FSharp.Core/async.fsi

@@ -47,6 +47,42 @@ namespace Microsoft.FSharp.Control
    [<CompiledName("FSharpAsync")>]
    type Async =

+        /// <summary>Runs the asynchronous computation and await its result.</summary>


Suggested change

/// <summary>Runs the asynchronous computation and await its result.</summary>

/// <summary>Runs the asynchronous computation and awaits its result.</summary>

psfinaki · 2023-01-04T13:36:18Z

tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs

+        let actual = $"{t}, after task: {d}"
+
+        if not (actual = $"Before: {d}, in async: {d}, after async: {d}, after task: {d}") then
+            failwith actual


Took me a while to understand what's going on here, I'd just give vars clear names ("threadIdBefore", "threadIdAfter"...) and assert them all against Thread.CurrentThread.ManagedThreadId.

psfinaki · 2023-01-04T13:38:34Z

I'd also suggest either here or in a followup to go through the codebase and make use of the new method.

FSharp.Core: Add Async.Runimmediate(Async<'T>, CancellationToken)

3bb73d2

vzarytovskii requested a review from a team as a code owner January 2, 2023 16:34

Fantomas

67e98b3

Reuse existing runimmediate via ResultCell<AsyncResult<_>> instead of…

36d1469

… TCS

vzarytovskii commented Jan 2, 2023

View reviewed changes

vzarytovskii requested a review from dsyme January 2, 2023 17:43

T-Gro requested changes Jan 3, 2023

View reviewed changes

KevinRansom changed the title ~~FSharp.Core: Add Async.Runimmediate(Async<'T>, CancellationToken)~~ FSharp.Core: Add Async.RunImmediate(Async<'T>, CancellationToken) Jan 3, 2023

KevinRansom requested changes Jan 3, 2023

View reviewed changes

psfinaki reviewed Jan 4, 2023

View reviewed changes

psfinaki approved these changes Jan 4, 2023

View reviewed changes

vzarytovskii marked this pull request as draft February 24, 2023 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FSharp.Core: Add Async.RunImmediate(Async<'T>, CancellationToken) #14532

FSharp.Core: Add Async.RunImmediate(Async<'T>, CancellationToken) #14532

vzarytovskii commented Jan 2, 2023 •

edited

Loading

T-Gro commented Jan 2, 2023 •

edited

Loading

vzarytovskii commented Jan 2, 2023 •

edited

Loading

vzarytovskii Jan 2, 2023

T-Gro left a comment

vzarytovskii commented Jan 3, 2023

KevinRansom Jan 3, 2023

psfinaki Jan 4, 2023

psfinaki Jan 4, 2023

psfinaki commented Jan 4, 2023

		@@ -1501,6 +1501,10 @@ type Async =

		computation.Invoke newCtxt)

		static member RunImmediate(computation: Async<'T>, ?cancellationToken: CancellationToken) =

	/// <summary>Runs the asynchronous computation and await its result.</summary>
	/// <summary>Runs the asynchronous computation and awaits its result.</summary>

FSharp.Core: Add Async.RunImmediate(Async<'T>, CancellationToken) #14532

Are you sure you want to change the base?

FSharp.Core: Add Async.RunImmediate(Async<'T>, CancellationToken) #14532

Conversation

vzarytovskii commented Jan 2, 2023 • edited Loading

T-Gro commented Jan 2, 2023 • edited Loading

vzarytovskii commented Jan 2, 2023 • edited Loading

vzarytovskii Jan 2, 2023

Choose a reason for hiding this comment

T-Gro left a comment

Choose a reason for hiding this comment

vzarytovskii commented Jan 3, 2023

KevinRansom Jan 3, 2023

Choose a reason for hiding this comment

psfinaki Jan 4, 2023

Choose a reason for hiding this comment

psfinaki Jan 4, 2023

Choose a reason for hiding this comment

psfinaki commented Jan 4, 2023

vzarytovskii commented Jan 2, 2023 •

edited

Loading

T-Gro commented Jan 2, 2023 •

edited

Loading

vzarytovskii commented Jan 2, 2023 •

edited

Loading