In ObjectMethodExecutor, add FSharpAsync support #221

SteveSandersonMS · 2017-05-02T12:46:39Z

To cover MVC requirements, this extends ObjectMethodExecutor so it knows how to treat FSharpAsync<T> as async and coerce it to an awaitable so that the upstream code gets the correct async result value.

dnfclas · 2017-05-02T12:46:42Z

@SteveSandersonMS,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

SteveSandersonMS · 2017-05-02T12:54:35Z

shared/Microsoft.Extensions.ObjectMethodExecutor.Sources/ObjectMethodExecutorFSharpSupport.cs

+            {
+                var parameters = candidateMethodInfo.GetParameters();
+                if (parameters.Length == 3
+                    && TypesHaveSameIdentity(parameters[0].ParameterType, possibleFSharpAsyncGenericType)


Does anybody know why parameters[0].ParameterType == possibleFSharpAsyncGenericType would be returning false here, even when the two System.Type instances really seem to refer to FSharpAsync<>? Why are there two different System.Type instances at runtime for what seems to be the same type?

As you can see, I've worked around this by comparing their Assembly, Namespace, and Name properties, but ideally I'd like to understand why I can't just compare the Type instances.

Generic crazyness? What types are involved?

Both parameters[0].ParameterType and possibleFSharpAsyncGenericType are instances of System.Type, and both represent the same type, FSharpAsync<>. However they are different System.Type instances. On one of them, .FullName is populated, and on the other, .FullName is null.

It's not really a problem - the workaround of comparing Assembly/Namespace/Name works perfectly adequately - it's just not what I was expecting to happen.

@davidfowl Can you think of someone specific on CoreFX we could ask this question? We don't need to block the PR on resolving it but I'm really curious why it wouldn't work...

SteveSandersonMS · 2017-05-02T12:55:55Z

shared/Microsoft.Extensions.ObjectMethodExecutor.Sources/ObjectMethodExecutor.cs

@@ -255,14 +260,15 @@ private static MethodExecutor WrapVoidMethod(VoidMethodExecutor executor)

            // var getAwaiterFunc = (object awaitable) =>
            //     (object)((CustomAwaitableType)awaitable).GetAwaiter();
-            var customAwaiterParam = Expression.Parameter(typeof(object), "awaitable");
+            var customAwaitableParam = Expression.Parameter(typeof(object), "awaitable");


The variable rename here is just because the old name was wrong.

analogrelay

Haven't fully finished reviewing, but some small initial comments :)

analogrelay · 2017-05-02T16:54:08Z

shared/Microsoft.Extensions.ObjectMethodExecutor.Sources/ObjectMethodExecutorFSharpSupport.cs

+
+        private static bool IsFSharpAsyncOpenGenericType(Type possibleFSharpAsyncGenericType)
+        {
+            if (possibleFSharpAsyncGenericType?.FullName != "Microsoft.FSharp.Control.FSharpAsync`1")


Also I think we (and certainly I) prefer string.Equals with an explicit StringComparison.Ordinal for clarity.

I don't mind changing it to a const, but out of interest, what's the reason? Obviously there are benefits if the literal value was repeated and might one day be changed, but it isn't repeated here, and practically can't ever change value.

I tend to think that moving the string contents away from the usage site makes it harder to reason about what the code does, because you have to scroll up and down to follow it.

That's a reasonable point. And mostly our guidance is to do that in places where the literal will be used in multiple sites.

OK, I've changed to string.Equals in all the applicable places. I'll leave the nonrepeated constants inline for easier reading as per the discussion above.

analogrelay · 2017-05-02T16:54:17Z

shared/Microsoft.Extensions.ObjectMethodExecutor.Sources/ObjectMethodExecutorFSharpSupport.cs

+        {
+            var assembly = possibleFSharpAsyncGenericType.Assembly;
+            var fsharpOptionType = assembly.GetType("Microsoft.FSharp.Core.FSharpOption`1");
+            var fsharpAsyncType = assembly.GetType("Microsoft.FSharp.Control.FSharpAsync");


More Constants :)

As per discussion above, will leave these in place.

davidfowl · 2017-05-03T10:35:17Z

/cc @dsyme

davidfowl · 2017-05-03T10:37:25Z

shared/Microsoft.Extensions.ObjectMethodExecutor.Sources/ObjectMethodExecutor.cs

@@ -354,6 +366,51 @@ private static MethodExecutor WrapVoidMethod(VoidMethodExecutor executor)
            return lambda.Compile();
        }

+        private static bool IsAwaitableDirectlyOrViaCoercion(
+            Type type,
+            out Expression coerceToAwaitableExpression,


It might be time to use a structure 😄

Yeah, and that structure could also wrap up a lot of the Expression generation code as well.

davidfowl · 2017-05-03T10:41:52Z

test/Microsoft.Extensions.Internal.Test/ObjectMethodExecutorTest.cs

@@ -427,6 +511,17 @@ public ValueTask<string> ValueTaskOfReferenceType(string result)
            public void MethodWithMultipleParameters(int valueTypeParam, string referenceTypeParam)
            {
            }
+
+            public FSharpAsync<string> FSharpAsyncMethod(string parameter)


I would go further and test an F# project (depending on how hard that is of course)

Adding a test project would be great, it would help us see exactly what is being supported, and would also make it easier to consider corner cases for cancellation, exceptions and so on

I will add an F# test project to the MVC repo and put in a functional test there. That will be a separate PR (since it's a different repo).

Initially it will only cover this specific functionality (invoking actions that return FSharpAsync<T>) but in the future we can add whatever extra functionality and tests we want.

@SteveSandersonMS I'm happy to help with that. How do you connect the two repositories? Or are you depending on a MyGet nupkg of this repo?

@panesofglass yeah. The packages get uploaded to MyGet (https://dotnet.myget.org/f/aspnetcore-ci-dev) once they get built and we reference the feed in all our repos.

@panesofglass Thanks for the offer! As it happens I already implemented the basics of this yesterday - it's now in this pull request https://github.com/aspnet/Mvc/pull/6231/files in case you have any suggestions for making it more F#-idiomatic.

Thanks, @SteveSandersonMS! I added a comment to show an async member, though I doubt you needed that assistance. The F# looks good. Thanks again for pursuing this, and please let me know if I can help by adding test cases. I'm trying to get the Mvc project building locally so that I can do so.

davidfowl · 2017-05-03T10:45:00Z

cc @enricosada

dsyme · 2017-05-03T11:01:25Z

@davidfowl I don't know much about ObjectExecutor or the internals of ASP.NET, but I tweeted here inviting more F# community eyes on this, and perhaps to spark additional work in integrating F# support deeper into ASP.NET infrastructure.

It would certainly be really handy if F# async were supported directly in the same places where tasks are supported. You can convert from F# async to/from tasks but it is a little annoying and can occasionally be error prone. Supporting F# async directly is better

davidfowl · 2017-05-03T11:13:12Z

It would certainly be really handy if F# async were supported directly in the same places where tasks are supported. You can convert from F# async to/from tasks but it is a little annoying and can occasionally be error prone. Supporting F# async directly is better

The consuming infrastructure is written in C# and we just need to be able to get the return value of a method. If that method is async, we need to await it before to optionally get the result and continue execution of the pipeline. The ObjectMethodExecutor is piece of a code that exposes a generic "thunk" to a .NET method. Given a type info and a method info, it will return an object model that exposes Execute and ExecuteAsync. The async code path now supports arbitrary awaitable objects based on the C# heuristics. The F# async support is doing the same but converting the F# async into a Task (since it covers all of the cases). If there's a better way to natively wrap the FSharpAsync<T> into a customer awaiter that we consume, that's fine too 😄 (if there's a benefit).

davidfowl · 2017-05-03T11:23:41Z

@dsyme out of curiosity, why isn't FSharpAsync<T> natively awaitable from a C# POV? That would make it so you can just await it from C#? I'm sure I'm missing something...

dsyme · 2017-05-03T11:44:15Z

@dsyme out of curiosity, why isn't FSharpAsync natively awaitable from a C# POV? That would make it so you can just await it from C#? I'm sure I'm missing something...

@davidfowl That's a really good question.

The philosophy of F# async is that "an async is a generator of tasks" in the same way that "an IEnumerable is a generator of IEnumerator".

That is, we explicitly generate a task or execution for an async (using Async.RunSynchronously or Async.StartImmediate, Async.StartAsTask... etc.) in the same way that we explicitly generate an enumerator for an IEnumerable using GetEnumerator.

Composing using generators has many advantages for correct async programming, just like composing with IEnumerable has many advantages (no one programs directly with IEnumerator - it's dangerous because you're directly exposed to the mutable state - just like programming directly with Task is dangerous, because the task is changing status and you can use task.Result much too easily). You can generate multiple tasks from the same async, for example. But more crucially, you supply the cancellation token when you start the async (i.e. generate a task for the async). The cancellation token is implicitly propagated through the entire async execution. That means you almost never need to propagate cancellation tokens explicitly in F# async code, and it is much easier to get cancellation right.

In short: An F# async is a generator of tasks. Cancellation tokens are provided on task generation and propagated implicitly

If we made asyncs implicitly awaitable then it becomes too easy to implicitly start asyncs and impossible to supply the necessary cancellation token when they are started. In the code in this PR, you should be careful to supply the right cancellation token when starting the async.

This fundamental difference between F# async and C# tasks is not deeply appreciated (though there are some details in this excellent guide to how C# and F# async differ) and the advantages of auto-propagating cancellation tokens were somehow lost in the long process by which F# async was transferred to C# and now many other languages. To be honest, I think it's really important in practice - I see a lot of complexity (and, I think, mistakes) in code that passes cancellation tokens explicitly and the corresponding F# code generally looks really clean.

Having said all this, there may be some way we can make F# async's much nicer to use from C#, e.g. by providing an AsTask instance method that replaces StartAsTask - very happy to have suggestions and PRs :)

panesofglass · 2017-05-04T03:52:47Z

I don't think it's going to be possible to directly use F# Async given @dsyme's explanation above and my own experience adding support for F# Async into ASP.NET Web API several years ago. I think you'll have to stick to the to/from approach. Making F# Async easier to consume in C# should probably get some attention, though, as that would probably be very beneficial to this and other projects.

SteveSandersonMS · 2017-05-05T16:17:45Z

@anurse @davidfowl It looks like all the CR feedback is covered. Is there anything else, or can one of you approve this?

davidfowl

LGTM

dsyme · 2017-05-05T17:41:32Z

I haven't looked at the code - can I just confirm that the cancellation token is being passed in when starting the F# async as a task (which I presume is what's happening in this code at some point), as I mentioned above?

analogrelay · 2017-05-05T17:52:57Z

We don't currently support CancellationTokens in MVC controller actions at all (even for C# async methods), so right now CancellationToken.None is provided when starting the Task.

SteveSandersonMS · 2017-05-05T18:03:43Z

Thanks, reviewers!

dsyme · 2017-05-06T00:07:14Z

We don't currently support CancellationTokens in MVC controller actions at all (even for C# async methods), so right now CancellationToken.None is provided when starting the Task.
View details

@anurse Thanks. If there's an issue tracking any future work for cancellation tokens then please add a link to this issue. The reflection techniques used in this code would make it very difficult for anyone to ever spot that they might need to do this :)

dmitry-a-morozov · 2017-05-07T00:34:36Z

@anurse Can you use
https://msdn.microsoft.com/en-us/library/microsoft.owin.owinrequest.callcancelled(v=vs.113).aspx
to grab instance of CancellationToken and pass into Async.StartAsTask?

davidfowl · 2017-05-07T00:57:45Z

@dmitry-a-morozov

I'm not sure we should pass that through by default. It would mean any of your async operations would be automatically cancelled if the client disconnects. Something like that should be opt in IMO. Anyways this library would need to allow passing a cancellation token and then it becomes a leaky abstraction.

panesofglass · 2017-05-08T19:39:14Z

@davidfowl just curious, but when would you not want to automatically cancel when a client disconnects?

SteveSandersonMS · 2017-05-08T19:54:45Z

@panesofglass Example: logging. You wouldn't want to abort logging that something happened just because the client disconnected before your log action completed.

davidfowl · 2017-05-08T19:55:29Z

Fire and forget
A database transaction that didn't have anything to do with the client

dmitry-a-morozov · 2017-05-08T20:22:42Z

@davidfowl @SteveSandersonMS
For this kind of operations is better to start child computation with dedicated cancelation taken.
I would argue that one doesn't need async top level op for this at all. Just have sync method returning void and spawn child op on thread pool or something like that.
I believe it's useful to trigger top-level cancelation when client disconnected. Maybe combined with optional timeout.

var tokenSource = CancellationTokenSource.CreateLinkedTokenSource(
    this.Response.ClientDisconnectedToken, this.Request.TimedOutToken);
 
CancellationToken token = tokenSource.Token;

davidfowl · 2017-05-09T03:20:13Z

I don't agree and I think cancellation server side code based on the client should be intentional. That said, this library isn't ASP.NET Core specific and just needs to provide a way to flow some token so that frameworks can pass it to begin with.

As for what token we pass in each of the frameworks, that can be debated at some later time on the relevant repository.

@pranavkm

* Link to InMemoryFileProvider As per @pranavkm's comment on #221 - adding a link to my contrib project. Made it a table in case there are other community contributions. * corrected readme as per pr review * corrected readme

In ObjectMethodExecutor, add FSharpAsync support

26e11cf

dnfclas added the cla-already-signed label May 2, 2017

SteveSandersonMS requested review from analogrelay and rynowak May 2, 2017 12:47

SteveSandersonMS commented May 2, 2017

View reviewed changes

Tweak comment

c75f8fa

analogrelay reviewed May 2, 2017

View reviewed changes

davidfowl reviewed May 3, 2017

View reviewed changes

SteveSandersonMS added 2 commits May 4, 2017 10:17

Refactor: Split out AwaitableInfo and CoercedAwaitableInfo structs

4eed022

CR feedback: Change string comparisons

85ce3a4

analogrelay approved these changes May 5, 2017

View reviewed changes

davidfowl approved these changes May 5, 2017

View reviewed changes

SteveSandersonMS merged commit 57fdf3b into dev May 5, 2017

SteveSandersonMS deleted the stevesa/objectmethodexecutor-supportfsharpasync branch May 5, 2017 18:08

SteveSandersonMS mentioned this pull request May 8, 2017

Support F# async in actions aspnet/Mvc#5570

Closed

knocte mentioned this pull request May 9, 2017

Improve async knocte/2fsharp#1

Open

knocte mentioned this pull request Feb 18, 2019

Why only expose C#-style API from an F# lib if you can expose both? joemphilips/fsharp-bitbankcc#2

Open

ghost locked as resolved and limited conversation to collaborators May 30, 2023

In ObjectMethodExecutor, add FSharpAsync support #221

In ObjectMethodExecutor, add FSharpAsync support #221

Uh oh!

Conversation

SteveSandersonMS commented May 2, 2017

Uh oh!

dnfclas commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented May 3, 2017

Uh oh!

dsyme commented May 3, 2017

Uh oh!

davidfowl commented May 3, 2017

Uh oh!

davidfowl commented May 3, 2017

Uh oh!

dsyme commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panesofglass commented May 4, 2017

Uh oh!

SteveSandersonMS commented May 5, 2017

Uh oh!

davidfowl left a comment

Choose a reason for hiding this comment

Uh oh!

dsyme commented May 5, 2017

Uh oh!

analogrelay commented May 5, 2017

Uh oh!

SteveSandersonMS commented May 5, 2017

SteveSandersonMS May 2, 2017 •

edited

Loading

dsyme commented May 3, 2017 •

edited

Loading

dmitry-a-morozov commented May 7, 2017 •

edited

Loading