New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continuations over binding Httphandlers (Breaking Change) #69

Closed
gerardtoconnor opened this Issue Jul 13, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@gerardtoconnor
Member

gerardtoconnor commented Jul 13, 2017

As hinted to before, I would propose we change the HttpHandler format while it is still in alpha/beta, while we still can, to use a continuation format rather than a binding format.

To explain why (and not drown this thread, I have blogged small piece here:
Carry On! … Continuation over binding pipelines for functional web

The overall return format would remain the same, returning Task<HttpContext option> but HttpHandlers would now include a continuation parameter next such that this is called rather than Some when an evaluation wants to move to the next handler in the pipeline.

Examples:

type HttpHandlerResult = Task<HttpContext option> // same as previous 

type HttpCont = HttpContext -> HttpHandlerResult  // continuation same as previous HttpHandler 

type HttpHandler = HttpCont -> HttpContext -> HttpHandlerResult // (HttpCont -> HttpCont) : Handler now has two input parameters 

let compose (handler : HttpHandler) (handler2 : HttpHandler) : HttpHandler =
    fun (next:HttpCont) (ctx : HttpContext) ->
        // 1. last next will ultimately be a (fun ctx -> Some ctx) but allows to compose / short circuit with continuations
        // 2. We can apply next function at load so all handlers have thier next function refs and need only apply ctx on run 
        let child  = handler2 next // suffix child handler(s) take next (Some ctx) success function
        let next = handler child   // prefix parent handler takes child handler as next continuation function 
        match ctx.Response.HasStarted with
        | true  -> task { return Some ctx }    // short circuit case
        | false -> next ctx
            
let rec choose (handlers : HttpHandler list) : HttpHandler =    
    fun next ctx ->
        task {
            match handlers with
            | []              -> return None
            | handler :: tail ->
                let! result = handler next ctx
                match result with
                | Some c -> return Some c    // bind wrapping required for choose handler (branching case)
                | None   -> return! choose tail next ctx
        }

let httpVerb (verb : string) : HttpHandler =
    fun next ctx ->
        if ctx.Request.Method.Equals verb
        then next ctx           // no longer wrapping async ('Some' replaced with 'next' fn)
        else task { return None }
        
let route (path : string) : HttpHandler =
    fun next ctx ->
        if (getPath ctx).Equals path
        then next ctx           // no longer wrapping async ('Some' replaced with 'next' fn)
        else Task.FromResult None
@JonCanning

This comment has been minimized.

Show comment
Hide comment
@JonCanning

JonCanning Jul 13, 2017

Contributor

If I understand this correctly, an HttpHandler would behave like middleware?

IApplicationBuilder Use(Func<RequestDelegate, RequestDelegate> middleware)

Contributor

JonCanning commented Jul 13, 2017

If I understand this correctly, an HttpHandler would behave like middleware?

IApplicationBuilder Use(Func<RequestDelegate, RequestDelegate> middleware)

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 13, 2017

Member

@JonCanning that's a nice way of thinking of it, micro middleware that is functional and extends the existing pipeline. I know it's preferable not to introduce an extra parameter but given it comes with many benefits (as outlined in my post), I'm hoping people may warm to it!?

Member

gerardtoconnor commented Jul 13, 2017

@JonCanning that's a nice way of thinking of it, micro middleware that is functional and extends the existing pipeline. I know it's preferable not to introduce an extra parameter but given it comes with many benefits (as outlined in my post), I'm hoping people may warm to it!?

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 13, 2017

Member

Actually, the second example in writing middleware in MS Docs is nearly identical to what I'm doing/proposing, cache a next delegate/function, call the function with a context. The only difference really is that we use an option to allow branching/choosing of multiple paths, If it wasn't for this branching requirement, you would just do a functional wrapper/compose for the requestDelegate and not even the change format, but as we know this is restrictive and takes away a lot of the benefit of being able to 'try' paths with fallbacks (like choose handler)

using Microsoft.AspNetCore.Http;
using System.Globalization;
using System.Threading.Tasks;

namespace Culture
{
    public class RequestCultureMiddleware
    {
        private readonly RequestDelegate _next;

        public RequestCultureMiddleware(RequestDelegate next)
        {
            _next = next;
        }

        public Task Invoke(HttpContext context)
        {
            var cultureQuery = context.Request.Query["culture"];
            if (!string.IsNullOrWhiteSpace(cultureQuery))
            {
                var culture = new CultureInfo(cultureQuery);

                CultureInfo.CurrentCulture = culture;
                CultureInfo.CurrentUICulture = culture;

            }

            // Call the next delegate/middleware in the pipeline
            return this._next(context);
        }
    }
}
Member

gerardtoconnor commented Jul 13, 2017

Actually, the second example in writing middleware in MS Docs is nearly identical to what I'm doing/proposing, cache a next delegate/function, call the function with a context. The only difference really is that we use an option to allow branching/choosing of multiple paths, If it wasn't for this branching requirement, you would just do a functional wrapper/compose for the requestDelegate and not even the change format, but as we know this is restrictive and takes away a lot of the benefit of being able to 'try' paths with fallbacks (like choose handler)

using Microsoft.AspNetCore.Http;
using System.Globalization;
using System.Threading.Tasks;

namespace Culture
{
    public class RequestCultureMiddleware
    {
        private readonly RequestDelegate _next;

        public RequestCultureMiddleware(RequestDelegate next)
        {
            _next = next;
        }

        public Task Invoke(HttpContext context)
        {
            var cultureQuery = context.Request.Query["culture"];
            if (!string.IsNullOrWhiteSpace(cultureQuery))
            {
                var culture = new CultureInfo(cultureQuery);

                CultureInfo.CurrentCulture = culture;
                CultureInfo.CurrentUICulture = culture;

            }

            // Call the next delegate/middleware in the pipeline
            return this._next(context);
        }
    }
}
@JonCanning

This comment has been minimized.

Show comment
Hide comment
@JonCanning

JonCanning Jul 13, 2017

Contributor

I had to write a middleware the other day since I wanted to execute the next delegate and do something afterwards, so I approve.

Contributor

JonCanning commented Jul 13, 2017

I had to write a middleware the other day since I wanted to execute the next delegate and do something afterwards, so I approve.

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 13, 2017

Member

There's that benefit too! I'm fully open to alternative implementations provided we can maintain the TCO & pass-through of tasks from child handlers to avoid wrapping sync in async.

My version with 3 parameters fun next fail ctx -> ... removes the need for an option, allows fail to jump right to finish/failover, all TCO, min stack frames, but is a bigger, more breaking change so getting the balance right is important.

Member

gerardtoconnor commented Jul 13, 2017

There's that benefit too! I'm fully open to alternative implementations provided we can maintain the TCO & pass-through of tasks from child handlers to avoid wrapping sync in async.

My version with 3 parameters fun next fail ctx -> ... removes the need for an option, allows fail to jump right to finish/failover, all TCO, min stack frames, but is a bigger, more breaking change so getting the balance right is important.

@dustinmoris

This comment has been minimized.

Show comment
Hide comment
@dustinmoris

dustinmoris Jul 22, 2017

Member

If I understood it all correctly, even though a breaking change, the difference is not huge in how someone would build and compose a web application, but with some great benefits overall. I think I am in favour of this change.

Member

dustinmoris commented Jul 22, 2017

If I understood it all correctly, even though a breaking change, the difference is not huge in how someone would build and compose a web application, but with some great benefits overall. I think I am in favour of this change.

@dustinmoris dustinmoris added this to the Beta milestone Jul 22, 2017

@dustinmoris

This comment has been minimized.

Show comment
Hide comment
@dustinmoris
Member

dustinmoris commented Jul 22, 2017

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 22, 2017

Member

Is it easiest if I incorporate the task CE, next continuation together with the new router into my develop fork, few people can test and make sure comfortable, then merge? Do one big breaking change only?

Member

gerardtoconnor commented Jul 22, 2017

Is it easiest if I incorporate the task CE, next continuation together with the new router into my develop fork, few people can test and make sure comfortable, then merge? Do one big breaking change only?

@dustinmoris

This comment has been minimized.

Show comment
Hide comment
@dustinmoris

dustinmoris Jul 27, 2017

Member

Hi @gerardtoconnor I think we should keep the task CE and the continuation changes as two separate PRs please. I think the continuation is a much less risky change than the task CE, which will require a lot less testing as well.

Member

dustinmoris commented Jul 27, 2017

Hi @gerardtoconnor I think we should keep the task CE and the continuation changes as two separate PRs please. I think the continuation is a much less risky change than the task CE, which will require a lot less testing as well.

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 27, 2017

Member

Ok, So would you prefer I do continuation or task CE first?

On the task CE, I have found a way to allow overloading of both Task & Task<'T> without type assertions, using extension methods, should make the changeover more seamless.

I will leave the tree router in a 3rd, last PR then.

Member

gerardtoconnor commented Jul 27, 2017

Ok, So would you prefer I do continuation or task CE first?

On the task CE, I have found a way to allow overloading of both Task & Task<'T> without type assertions, using extension methods, should make the changeover more seamless.

I will leave the tree router in a 3rd, last PR then.

@dustinmoris

This comment has been minimized.

Show comment
Hide comment
@dustinmoris

dustinmoris Jul 27, 2017

Member

I think the continuation could be a quicker gain, but what do you think?

Member

dustinmoris commented Jul 27, 2017

I think the continuation could be a quicker gain, but what do you think?

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 27, 2017

Member

Well I think both are important for performance so both need to be done but I guess the continuation is little simpler, less breaking.

One of the reasons I wanted to do both together was to do just one big breaking change as doing two in close succession may be taxing on users, If we merge continuation with the Task CE to follow closely after, would be pointless for users to try upgrade till both are in ... I'm just thinking from users perspective?

I have both continuation & task CE updated in src of my gtoc-develop branch if you want to review, need to update test & sample proj still

Member

gerardtoconnor commented Jul 27, 2017

Well I think both are important for performance so both need to be done but I guess the continuation is little simpler, less breaking.

One of the reasons I wanted to do both together was to do just one big breaking change as doing two in close succession may be taxing on users, If we merge continuation with the Task CE to follow closely after, would be pointless for users to try upgrade till both are in ... I'm just thinking from users perspective?

I have both continuation & task CE updated in src of my gtoc-develop branch if you want to review, need to update test & sample proj still

@dustinmoris

This comment has been minimized.

Show comment
Hide comment
@dustinmoris

dustinmoris Jul 27, 2017

Member

I agree with you, however the Task CE is a more complex change IMHO and I would want to test its perf impact more thoroughly before merging, while the continuation change I think of as an elegant architectural change with little risk.

Member

dustinmoris commented Jul 27, 2017

I agree with you, however the Task CE is a more complex change IMHO and I would want to test its perf impact more thoroughly before merging, while the continuation change I think of as an elegant architectural change with little risk.

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 27, 2017

Member

Cool, I'll rebase off current async branch and do contination PR in next day or so.

Member

gerardtoconnor commented Jul 27, 2017

Cool, I'll rebase off current async branch and do contination PR in next day or so.

@gerardtoconnor

This comment has been minimized.

Show comment
Hide comment
@gerardtoconnor

gerardtoconnor Jul 29, 2017

Member

PR added #71

Member

gerardtoconnor commented Jul 29, 2017

PR added #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment