Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Support F# async in actions #5570

Closed
enricosada opened this issue Nov 28, 2016 · 10 comments
Closed

Support F# async in actions #5570

enricosada opened this issue Nov 28, 2016 · 10 comments

Comments

@enricosada
Copy link

enricosada commented Nov 28, 2016

Support F# async like the Task based async in actions.

So an action like the following can be used OOTB.

member this.About () = async {
    let! msg = getDescription ()
    this.ViewData.["Message"] <- msg
    return this.View()
    }

I added an example aspnet project with both F# async and current workaround actions.
Example is based on dotnet new -l fsharp -t web template in preview4, but is the same for all sdk versions (preview2 etc)

The f# async Home/About doesnt work (async class is converted as string => {}), converting as Task works in Home/About2

The workaround is to call Async.StartAsTask before returning the FSharpAsync, so is converted as Task.

How to implement that.

Like xunit does, recognize the result is FSharpAsync and invoke Async.StartAsTask (ref xunit commit to add f# async support xunit/xunit@a2aa665 )

In previous aspnet was possibile to use an ActionResult.

Speaking with @davidfowl he directed me into ObjectMethodExecutor class.

It should not give runtime performance penality (f# or c#), because it's done at initialization step also for normal Task, so at runtime just use a different delegate configured in ObjectMethodExecutor (in GetCoerceMethodCallExpression?)

/cc @panesofglass

@enricosada
Copy link
Author

If the approach is correct and approved as idea, can be put as up-for-grab? if noone want to step in, i'll fix that later.

@rynowak
Copy link
Member

rynowak commented Nov 28, 2016

/cc @Eilon thoughts? Having looked at the xunit approach, I like that they avoid adding a hard dependency on any F# libraries.

@davidfowl
Copy link
Member

davidfowl commented Nov 29, 2016

I think we should do this.

PS: Whatever we do here we'll want similar logic in Common because SignalR will be the next thing to implement this.

@enricosada
Copy link
Author

enricosada commented Nov 29, 2016

If it's ok to hardcode it (clean job and reusable obv) for now as suggested, it will be possible to refactor out and move as external library (initialized in Startup or when creating the WebHostBuilder) when an extension point exists (and add that library only to f# aspnet template). But is a small use case for an extension point, i cannot think of others (non f# too) atm because serializers already understand f# types. We are just starting using aspnet with f#, so let's gather feedback first 😄

For example for aspnet core handlers, we dont need changes, we just use an extension method to overload IApplicationBuilder.Run method

let myHandler (context: HttpContext) = async {
    do! context.Response.WriteAsync("Hello World from F#!") |> Async.AwaitTask
}

app.Run(myHandler)

And extension will be inside an helper lib soon (need to publish that 😄 , but following code is ok too to embedd)

[<AutoOpen>]
module Async =
    // Async<unit> -> Task
    let inline StartAsPlainTask (work : Async<unit>) = work |> Async.StartAsTask :> Task

    //Extension method used to Run an Async<unit> as RequestDelegate
    type Microsoft.AspNetCore.Builder.IApplicationBuilder with
        member this.Run(handler : HttpContext -> Async<unit>) =
            this.Run(RequestDelegate(handler >> StartAsPlainTask))

I hope to resolve it once and for all for all libs, but no clue how to do it, so fix every lib when needed is the best current option.

@enricosada
Copy link
Author

@rynowak @Eilon @davidfowl sry for ping, just to not stop the discussion, because i'd like to add that if is approved as idea.

@nikonthethird
Copy link

nikonthethird commented Jan 16, 2017

Would that mean that async getter-only properties are supported as well? Or does it have to be a method?

type MyController () =
    inherit Controller ()

    // Would this be possible?
    member this.IndexProperty = async {
        return this.View ()
    }

    // Or does it have to be a method?
    member this.IndexMethod () = async {
        return this.View ()
    }

@rynowak
Copy link
Member

rynowak commented Jan 18, 2017

@nikonthethird - no plans to change what we consider an action as part of this work, only methods.

You could write something that adds support properties if you want to, but out of the box we'd only consider methods.

@Eilon Eilon added this to the 2.0.0 milestone Jan 30, 2017
@Eilon Eilon added the up-for-grabs Members of our awesome commnity can handle this issue label Jan 30, 2017
@Eilon
Copy link
Member

Eilon commented Jan 30, 2017

Hi @enricosada , sorry for the delay. We would certainly accept a PR for this if you have time to work for it.

@rynowak
Copy link
Member

rynowak commented Mar 29, 2017

See #6040 - while were here we should do valuetask as well

@SteveSandersonMS
Copy link
Member

Done since dotnet/extensions#221 now understands FSharpAsync<T>.

Have added a functional test to show it works end-to-end: #6240

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants