Task/Task<'T> Computation expression async bindings

[gerardtoconnor]

As discussed in issue #53, I have updated new continuation format to use task {} rather than async {}.

This version allows binding to both Task<'T> & plain Task without the need of type assertions thanks to TaskBuilder based on @rspeele implementation that uses Extention methods & task-like member constraints for seamless overloading. I have emailed Robert and he is happy for us to use in Giraffe based on his implementation (I have removed taskbuilder context-sensitive related code given asp.net core does no context switching and is purely context-insensitive).

This TaskBuilder approach should also allow the use of ValueTask<'T> in the future to remove scheduling of final pipeline completion Tasks of Some/None but for now, Task should be fine.

Due to change in execution path logic for subRoutes, I had to update handlerWithRootedPath to test returned value (Some/None), such that it would strip the current subpath out of the routekey if it had failed and needed to continue. The completed Task that provides this option result is passed completed into the pipeline rather than rescheduling a new task as the binder will pick up that it is already completed on next continuation and not waste scheduling. My next PR for new Router will provide a more direct performant alternative for sub-routing.

[dustinmoris]

Hi, I did run some load tests of the latest develop branch today and then compare it to this PR and it seems that the async branch is about 9% slower than the change to the task CE. This is a pretty good improvement!

I noticed that you made a few changes to the TaskBuilder.fs and I wanted to ask you if it was possible to keep the original as it is? The reason why I am asking is because in the future if the original author will make updates to the TaskBuilder.fs (e.g. bug fixing or other stuff) then I would want to keep it simple by just copy and replacing the file in Giraffe.

[gerardtoconnor]

I had not gotten around to doing performance testing based on this alternative Taskbuilder but submitted with the view that we could tweak performance once the api was in the format we want (can bind both Task & Task<'T> without the need for type assertions).

+9% performance boost is actually a bit disappointing and a lot less than previous tests, some of this is due to the next continuations removing unneeded async bindings, but I would still have expected double digit improvements in performance but the TaskBuilder needs tweaking as prior implementation was initially giving 16% performance boost over async-bind, and +26% over async-bind when continuations included (task-cont).

To answer your question along with details above, The TaskBuilder.fs implementation defaults to Task being context-sensitive, and in a namespace unrelated to Giraffe which might be confusing to devs. ASP.NET Core does not do context switching on tasks as each request (HttpContext) is in its own scope without any shared state so we are purely context-insensitive (The Services are shared but that is handled separately by framework). Therefore if we use TaskBuilder.fs unchanged we need to have the devs import twice the code that they need, along with potential confusion as to which namespace to open, (ensuring it's the FSharp.Control , context-insensitive one).

The namespace/code bulk issue is only a small/incremental reason, the real reason is down to performance. Following on from above, performance is slower than I would expect (might not be full +16% due to permutations of cont/bind async/task) and this is likely due to duplication of task state-machine with DU in this implementation. The TaskBuilder can be done, like before without an internal (flow) state-machine (Task has its own internal one already), and as the state machine is using DUs, this is not good for performance as we are constantly allocating, casting & GCing high frequency of DU where no DU is really needed (DU are great for Domain Modeling (lowFreq) but bad for performance (highFreq) as in addition to items mentioned above, the compiler cannot do as many optimisations vs more explicit imperative implementation).

So long story short, I hope to rework the internals of the TaskBuilder for performance purposes, removing internal DU state-machine to more explicit imperative implementation that has minimal GC pressure & more compiler optimizations so this current version is only a stopgap. I have submitted as is given we can have stable api now (allowing for both Task & Task<'T> bindings) so there will be no breaking changes once performance improvements baked in.

[dustinmoris]

Ok, what you say makes sense 👍

[dustinmoris]

Quick question, why did you re-position this in the test?

[gerardtoconnor]

Apologies, meant to switch back again, I ran into a testing issue where routes after a subroute handler were failing so I switched around the order to test the consistency of the issue. The route order can be put back as was as I fixed the problem.

The issue related to the subroute not stripping back the subpath on a failed match attempt now that the execution path had changed slightly and removed the need to do try/finally in handlerWithRootedPath function as it can determine success/failure in its own scope now.

Please disregard or change back as does not need to be changed.

[dustinmoris]

Hey I tried to get my head around why the execution path has changed. It worked with the continuation when it was still async. Is it because the task is a hot task now like in C#?

[gerardtoconnor]

Sorry, should have been more clear, the try/finally works a little differently in TaskBuilder.fs then did before (i didn't change it) and while I was going to fix up & correct, I didn't see why we were using try/finally to "Hack" the functionality (so that we could basically run a finalizer when task dropped out of scope etc), and with there being no need to use try/finally, I fixed it to use proper logic flow, stripping back stem on failed subroute match.

TaskBuilder.fs runs finally after there is a full Return value, not a bound task, not too difficult to tweak to how async does it.

I will fix try/finally in taskbuilder as I re-write/rebuild it but I really not sure why subroute was built the way it was using try/finally, that construct is for code with likely exceptions that need resources cleaned up (disposables etc), bizarre how it is being used in handlerWithRoutedPath.

If replicating prior try/finally function flow is a priority, I can tweak before we release?

NB: I think we can take it that basically, all Tasks are Hot Tasks as the framework is driving/starting them, and nearly all Task Async method calls are hot too ... when using async, it was all Hot Async as we were scheduling async based on the hot tasks (double scheduling), nothing was really being created in a cold state to be run later.

[dustinmoris]

ah that makes all sense. TBH I don't even know if you should fix try/finally to behave like it does with async. This is one of those things where we need to decide if we want the task ce to behave as closely as possible to C# tasks or to F# Async. Thanks for getting back to all my questions!

[gerardtoconnor]

No Problem (thanks for doing review!).

Well we can think of a few use cases and see what makes sense. There is merit in both, TaskBuilder.fs is running finally once all the chained tasks are completed (or failed), while async will (and has) run finally when the immediate bound async work has finished.

If I had chained binding work of IO on a disposable resource, I would rather the finally (Dispose) run when all work was finished, not reopen & dispose on each leg of async IO work.

Back to my original point, I changed handlerWithRoutedPath as I could not understand why try/finally was being used at all and think try/finally should be used sparingly.

[dustinmoris]

Ok, one more question, I am not sure why on line 73 you had to run ctx.Items.Item RouteKey <- savedSubPath.Substring(0,savedSubPath.Length - path.Length). Shouldn't assigning ctx.Items.Item RouteKey <- savedSubPath just be fine, as savedSubPath should still hold the same value from above?

I tried the following change and all tests still pass:

let private handlerWithRootedPath (path : string) (handler : HttpHandler) : HttpHandler =
    fun (next : HttpFunc) (ctx : HttpContext) ->
        task {
            let savedSubPath = getSavedSubPath ctx
            ctx.Items.Item RouteKey <- ((savedSubPath |> Option.defaultValue "") + path)
            let! result = handler next ctx
            match result with
            | Some _ -> ()
            | None ->
                match savedSubPath with
                | Some savedSubPath -> ctx.Items.Item   RouteKey <- savedSubPath
                | None              -> ctx.Items.Remove RouteKey |> ignore
            return result
        }

I think I am missing a critical test here where one of the two implementations should fail...

[gerardtoconnor]

Apologies, my bad, I am getting logic mixed up with my router that maintains the state in the dictionary, whereas here we keep a copy of string to re-insert after.

Before, we always reset the savedSubPath, whereas above, we only reset savedSubPath if route failed so as to allow further matching.

I updated to revert back and bizarrely am now failing on a completely unrelated test
```GET "/foo/blah blah/bar" returns "blah blah"`` (HttpHandlerTests.fs Line 453)
Looking over the test, I'm not sure why it is expecting `blah%20blah` as the result when given `blah blah` as it is usually the browser (& server) that substitute the space for `%20` and would not expect NSubstitiute dummy to replicate!? is there middleware or internal function that should be doing this substitution on path before/while parsing?

[dustinmoris]

Hi Gerard, thanks for this awesome work. I have finished all my review and did a bit of tidy up in the README and removed the task {} from a few places where I could just return the Task directly. Only thing I had to fix was the handlerWithRootedPath function. As discussed above, I found that ctx.Items.Item RouteKey <- savedSubPath.Substring(0,savedSubPath.Length - path.Length) was cutting off too much of the savedSubPath. It took me a while to realise what test I had to write to test for it and I added those tests as well now. Otherwise it looks really good and this PR plus the continuation are really good improvements. My perf tests also came out really well, I know you sounded a bit disappointed, but I only tested a few cases, so I suppose depending on the load test there could have even been bigger gains!

[gerardtoconnor]

Performance will improve the more and deeper the routes are but I will be working on tweaking performance once router merged (DU removal from task CE etc), and there are significant performance boosts from new router, parsing in particular. I will look to submit router PR this weekend when I have little time (you probably need a break from reviewing all these massive PRs sorry!).