New ValueTask Computation Expression to replace Async

[gerardtoconnor]

As discussed, this CE replaces Async for performance and more fluidly fit into Asp.net code framework.

The CE is overloaded to take value ('T), 'Task<'T>, ValueTask<'T> and Async<'T>

Feel free to reject fsproj header changes, don't know why they are showing up here.

I have updated all tests too and ensure they pass

[dustinmoris]

Hey, there seems to be an issue when executing the tests. I re-started the build for the commit and I can see that it gets stuck on the test execution again:

It will be stuck at this point now until I run out of build minutes and after 60 minutes AppVeyor will mark the build as failed saying that I ran out of the maximum build time (1h).

The first thought that comes to my mind is that the test execution got deadlocked somewhere. We should investigate this and make sure that the build will succeed on AppVeyor.

[gerardtoconnor]

Fully agree, please kill build if you can? The tests ran and passed in 2 seconds on my machine (after having failed pre test updates), I was running with dotnet core 2.0 pr1 so I wonder if that is cause for discrepancy? I will re-review tests, sounds like 'Task' failing to complete in tests

[gerardtoconnor]

From Build Tests page, it appears these are the two tests causing the issue:
Giraffe.ModelBindingTests.bindModel during HTTP GET request with query string returns correct result
Giraffe.HttpHandlerTests.PUT "/post/2" returns 404 "Not found"

Will investigate these some more, I have rerun test on my machine and all complete fine so ¯_(ツ)_/¯

Test run for F:\Code\Giraffe\tests\Giraffe.Tests\bin\Debug\netcoreapp1.1\Giraffe.Tests.dll(.NETCoreApp,Version=v1.1)
Microsoft (R) Test Execution Command Line Tool Version 15.0.0.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
[xUnit.net 00:00:00.5216346]   Discovering: Giraffe.Tests
[xUnit.net 00:00:00.6542994]   Discovered:  Giraffe.Tests
[xUnit.net 00:00:00.7086648]   Starting:    Giraffe.Tests
[xUnit.net 00:00:01.3558138]   Finished:    Giraffe.Tests

Total tests: 74. Passed: 74. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 1.8489 Seconds

[dustinmoris]

Ok cool, I also need to spend a bit more time to review the new ValueTaskCE.fs and better understand what is going on there. I might reach out to you with a few questions. Meanwhile I also created this as a suggestion to the F# language. I think you're right that the constant conversion from task to async doesn't make a lot of sense. Please feel free to add your own comments there or help me push this suggestion by upvoting :).

[gerardtoconnor]

... hmmm, your a braver man than I, using task over async is heresy to a lot of FP diehards! It fits this case perfectly due to vast majority of work being done in Task by asp.net core anyway so just removing conversion overhead as well as taking advantage of recent TPL performance updates (many who claim Async faster were correct pre 2013 before asp.net team started to performance tweak for asp.net core).

I noticed you identified linux build test in other PR was due to beta XUnit, could this also perhaps be the reason my PR tests are failing?

If you have any questions about ValueTask or CE, feel free to fire them across.

[dustinmoris]

The other PR had a dependency upgrade from a stable xunit version to a beta version, but I don't see this change in this PR, so not sure if it is the same issue.

I don't care what some FP diehards believe, I want F# to be as good as possible and it feels like that it can do a lot better on the async vs task topic at the current stage of where we are in .NET and therefore I feel brave enough to suggest it :)

[gerardtoconnor]

Where would we be without tests ... My implementation with ValueTask is not working properly with CE TryWith ... all the overloading and type inference is getting messy to the point I am wondering should we just still with plain Task CE and sync bindings can be looked into as part of Discussion I can raise on continuation handlers (vs bind ones). I will fiddle around with it a bit more but I think that due to the fact that I cannot gain access to the underlying task, to attach a continueWith handler, before it's faulted means it may not be possible to handle correctly.

[dustinmoris]

Yes I think its a smart move to improve the API in small steps. Let's get the regular Task CE in place and then once that is confirmed to work well we can get ValueTasks integrated as well.

I had a look at your Task CE implementation and it is mostly clear to me, but I was wondering why we were using GetAwaiter instaed of ContinueWith. I think the SO thread mentioned initially that GetAwaiter would perform faster, but then later it has been acknowledged that there was no real evidence for it. From my readings today I think that GetAwaiter is not really intended to be used and ContinueWith is the easier (and probably preferred) option to implement it in our case.

FSharpX and Orleankka do the same. The other difference which I noticed is that in the TryWith we return an AggregateException in the case of failure while the task CE in FSharpX and Orleankka would return t.Exception.GetBaseException(), but maybe I misunderstood the code.

I think there was one more thing that I wanted to ask, but I don't remember it from the top of my head now. The rest of the code made sense to me :)

I'll wait for you to update the PR with the suggestion you made and then I'll see what's left where I have questions! Thanks for all your work on this by the way!

[gerardtoconnor]

Could have saved myself a bit of headache not bothering with ValueTask ... I butchered TryWith trying to get it to work with all the already extra overloads and its not working properly ... hence why not same as originals, they work, thats why one of the tests are not working, the error handler is not firing properly as TryWith is a mess. I will wind back all the changes to plain Task CE.

Task.Result = GetAwaiter().GetResult() like ContinueWith is almost same as awaiter().OnCompleted ... the awaiter methods are the compiler low level methods that generally should not be touched, I know the speed difference between ContinueWith and awaiter are essentially the same so I will use ContinueWith if you prefer.

No worries on the help ... this is the first open source Project I've really helped on (with exception of small Paket performance optimizations), the quality of the docs and code make it a pleasure!

[gerardtoconnor]

I dont think Travis CI is configured, hence why keeps failing anyway but on appveyor, one of the tests is failing due to an expected result which may not be correct.

let ``Test /error returns status code 500`` () =
    use server = new TestServer(createHost())
    use client = server.CreateClient()

    get client "/error"
    |> isStatus HttpStatusCode.InternalServerError
    |> readText
    |> shouldEqual "One or more errors occurred. (Something went wrong!)"

But in the route, the fail message is

let webApp = 
    choose [
        GET >=>
            choose [
                route  "/"           >=> text "index"
                route  "/ping"       >=> text "pong"
                route  "/error"      >=> (fun _ -> failwith "Something went wrong!")
                route  "/login"      >=> loginHandler
                ... ]
          ]

My exception handler is sending as "Something went wrong!", I cannot find where/how it is being wrapped in One or more errors occurred. (%A)

Was the error response just returning this when using Async and it was used as expected result to match up with test? I doesn't seem correct to me if we are including text we do not control (we can manually wrap such a response if we want but shouldn't be happening automatically unless I am missing something? If I update expectation to Something went wrong! it passes

[dustinmoris]

Hi Gerard, thanks for all the work. I just did a quick review and the only questions I have now is:

• why do we have to use task.AwaitTask in certain situations
• why do we need to run asPlainTask in the middleware?

I am at a wedding today but I think the rest looks fine and I will try to get some time tomorrow morning to merge the PR, otherwise latest on Monday morning!

[gerardtoconnor]

No rush on the merge, it's best we take time to get it right, reach the right compromise.

The answer to both questions is fundamentally same, .net plain Task is not same as Task<'T>.

Everything looked fine initially as I overloaded the CE to take both Task and Task<'T> but then due to compiler issue with type inference, it wasn't able to infer the types of some bindings (was able to bind most). having to put type annotations on bind values is not ideal so I added task.AwaitTask function to convert Task to Task<unit> , and removed the overload, so now was only Task<'T>.

The Invoke methods in the middle ware all use Task, not Task<'T>, what is returned by Task CE ... so I cast Task<'T> to Task (as Task<'T> inherits from Task (one reasons break inference)).

I have submitted alternative that has both overloads, and requires some type inference in places, and I also added an additional await CE, which is a CE for plain Task that is just used in middleware where we are both binding and returning Task.

It's a pain that Task is not compatible with Task<unit> so need to figure out if convenience of overload CE, with better performance but occasional type annotations being required is better then using no overload and having to convert Task, to Task<unit> to make it compatible with non-overloaded task CE.

[gerardtoconnor]

@dustinmoris should we scrap this, allow me to rebase and try again or are you able to resolve conflicts? there's probably quite a few with recent two PRs.

[dustinmoris]

I think I will be able to resolve this. I planned on doing it today actually...

…

On 23 Jun 2017 9:00 am, "Gerard" ***@***.***> wrote: @dustinmoris <https://github.com/dustinmoris> should we scrap this, allow me to rebase and try again or are you able to resolve conflicts? there's probably quite a few with recent two PRs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#59 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEXlat6e6DCrvEZTS7GQHFBlE-1_oo4Yks5sG3CMgaJpZM4N6n9s> .

[gerardtoconnor]

Cool, and is it clear the difference between overloading CE with plain 'Task' ?

Pros overload

• No need to additionally bind to additional task with additional awaitTask fn.
• Better performance no wasteful bind
• Simpler consistent bind

Cons overload

• due to method overload with Task/FSharpFunc, cannot always infer
• In some bind cases, let! binding requires type annotation e.g.:

task { let! (ctx:HttpContext) = ... }

• some/most task binding does work, unsure about why the 3/4 fringe cases in project happen.
  I think adding await CE for 'Task' delegate handler is no brainier

[dustinmoris]

I have resolved the conflicts and moved this PR into this new branch.

A few things I changed:

• I removed a few usages of task where the original method could be just rewritten without the task CE. For example, the readFileAsString in Common.fs didn't need to have a dependency on Giraffe.Tasks:

let readFileAsString (filePath : string) =
    use stream = new FileStream(filePath, FileMode.Open)
    use reader = new StreamReader(stream)
    reader.ReadToEndAsync()

I've made similar changes elsewhere applicable.

I did some experimentation, but couldn't get the type inference properly working either, which is really annoying and I tried to get rid of the second AwaitBuilder by overloading the this.Return from the task CE but that always resulted in the unit test which checks for error handling to fail.

Looks like we have two new CE, which is not very nice to be honest. This get's quite complicated when there is async, task and await now available. I think the aim should be to have one properly working task CE and not have to require a third await one :(

The other thing which is still something I would like to understand is the difference between this task CE implementation and the normal async workflow from F#. The error handling of tasks seems to be different and this is why you had to change the unit test from the sample app.

[gerardtoconnor]

With Computation Expressions, a hybrid form of Monad, the return/wrapped type should be the same so there is usually no way you could overload return type as you would then need to add additional overloads to all the binds, combines, for/while/try.... it becomes a disaster and although it's technically possible , practically it's not, not with generic types and method overloading inference issues.

The await is not needed at all, it's purely only used in middle ware, wasn't for general use anywhere else, if you don't want it, we can just imperatively return Task from each relevant exec branch.

I'll try re-merge my develop branch to your new branch, this may be too much of a breaking change for most who don't care about performance, tough call.

Error handling between two is similar, tasks return aggr exception (if isfaulted), the task {} currently just extracts the base exception, the Async tells you there was aggregate exceptions text wrapping the base exn... it may list multiple too ... I think users should probably get just base as they are not debugging the system!