net/context based http handlers

[peter.k...@pressly.com]

Just wondering about the interest level and any suggested approaches for adding context.Context-based http handler support?

I submitted a patch that adds a context.Context handler interface to the existing net/context/ctxhttp (https://github.com/golang/net/tree/master/context/ctxhttp) package and looking for some feedback.

The approach:

* First steps in a context.Context http handler world - added to a core “support” subpackage like ctxhttp

* Control a request - timeout, cancel, store values

* Standardized middleware that require a request context - versus the constant invention of inoperable handlers. See the different signatures of the popular mux libraries: goji, martini, gin, httprouter, gorilla, negroni, and it goes on.

* Optional and compatible - not to compete or break the standard library

Patch: https://go-review.googlesource.com/#/c/15620/

Code diff:

* https://go-review.googlesource.com/#/c/15620/3/context/ctxhttp/ctxhttp.go

* https://go-review.googlesource.com/#/c/15620/3/context/ctxhttp/ctxhttp_test.go

[Brad Fitzpatrick]

Repeating from the code review, I think we'd be better off adding a field to http.Request:

   package http

   // Context is expected to be a golang.org/x/net.Context value.

   // If non-nil, it is expected to have compatible methods.

   type Context interface{}

   type Request {

        ...
       // Context is the optional golang.org/x/net.Context for this request.

       Context Context

   }

Then we can interface-sniff it to see if it has the Done method:

   Done() <-chan struct{}

And use that struct value for the http.Client's cancel channel if the Request.Cancel field is nil.

But *http.Request is not just for Clients, but also for Servers (as your proposal is about). A good proposal would involve both.

If some "middleware" is responsible for populate Request.Context on incoming server requests, then the http Server itself couldn't take advantage of it being set already and use the context.Done, or cancel the whole request when the underlying connection goes away or the HTTP/2 stream gets a RST_STREAM.

So we probably want the server to populate the Request.Cancel. But there's no standard for these things, so it probably needs to be a new hook on http.Server, like:

package http

type Server struct {

     ...

     // PopulateRequestContext returns a Context for the given Request.

     // At this stage, the Request.Body is nil. 

     // RequestContext may be nil or return nil.

     // Server will always set a Context, but if RequestContext returns a non-nil

     // value, that Context becomes its parent context.

     RequestContext func(*http.Request) Context

A proposal also needs some sort of vendoring, partial re-implementation, or registration story for the x/net/context package, for how net/http makes them to give out.

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[David Crawshaw]

If you're going to use Context in net/http, why not skip the proxy
type and bring the package in as net/context?

[Brad Fitzpatrick]

Yeah, that should be considered in any proposal too.

My -2 on the code review was simply that the answer isn't a "ServeHandleC" method, fragmenting the community and  hurting composability.

I don't have time to drive a proposal for this at the moment, though.

[peter.k...@pressly.com]

Hey Brad,

I definitely appreciate your feedback. I didn't consider how I would change net/http to support Context, because I didn't think that'd have any chance of consideration, but I can see how the design you've just whipped together can work and maintain compatibility. It's a better approach than an interim proxy type for sure.

David,

As in importing net/context from net/http? I didn't consider that because net/context in my opinion is a 2nd tier core package given it lives in golang.org/x. I heard murmurs of net/context going into stdlib potentially one day... so then, yea, that makes sense too.

It will be exciting when someone pulls this off and it all comes together. Great feedback.

[sam...@google.com]

I'd be happy to see Context move into the standard library.  If this is something that should happen for 1.6, please let me know.

[Dave Day]

+1, I'd also be keen to see context moving into the core library and to have good support in net/http – I've been thinking about it for a while. I'm willing to try to drive this proposal, unless there's someone else who would like it on. Is 1.6 a feasible milestone, or is this too big too late?

Brad, your proposal seems like a good starting point. You have a lot more state in your head about the various interactions and uses of the http package so it would be great to have you as involved as time permits.

Sameer, can you envisage many changes you'd make to context if you were to do it again? Are you aware of any other places in the stdlib we can retrofit it without damaging the APIs?

[sam...@google.com]

The only change I considered for context removing the Deadline method and replacing it with a context value added via a separate deadline package.  Doing this would remove the context package's dependence on time and so permit context to be imported anywhere in the standard library.  I discussed this with others when I first open sourced context, and they said it's fine as is.

I'd like to do the move to the standard library myself, if we decide to do it.

[peter.k...@pressly.com]

I think net/context is good as is, I'd keep the Deadline() returning a time -- the only package that couldn't use Context would be "time" itself. I'd love to see net/context in stdlib too.

[Brad Fitzpatrick]

Sameer, I think more realistic is we should try to do this for Go 1.7, which ships early July 2016, so code freeze early May 2016, so probably proposal + code starting in the next few months. It would probably be even better if this went in early in Go 1.6 dev (Feb 2016) to get the most number of people trying it out.

I'm willing to help with the proposal in a few weeks here whenever you want to get started.

[sam...@google.com]

Sounds good.  Was that last sentence supposed to say 1.7 dev?

[Brad Fitzpatrick]

Yeah, whoops. 1.7 dev starts in Feb.

[jimmy frasche]

Would this be going into the stdlib as net/context?

While many, perhaps the majority, of the use cases are for networking,
there's nothing inherently network specific about the package itself
and its inclusion in net/ somewhat obscures its generality.

I'd prefer it just be import "context".

[jgbe...@gmail.com]

I'd love to see a Context added to the http.Request.  It would simplify so much and allow both context aware middleware and existing non-context aware middleware to play nicely.

If I were to assume that a context.Context were to be included in the http.Request in the next year or so, what would be the best way to write a new API so that it could most easily handle these expected future changes?

[dcco...@gmail.com]

This means we cannot use it until almost a year from now. Is there a 'polyfill' option for this?

Context for http request was such an obvious feature to have to begin with, but then again the go team is metal minimalist. The lack of this has caused so much community confusion, wheel reinvention and bad practice for newbies such as creating things like martini.

[Rodrigo Kochenburger]

In the documentation for net/context, it says: "Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it."

I'm not sure what's the reasoning behind or if it's simple for consistent but just think it makes sense to bring it up before adding to the Request object. Also, I like the idea of a different handler because then you can expect the context to always be available and initialized instead of maybe being there.

The idea as a whole though makes sense to me, I've been using a custom handler type with net.Context for a while so I'd be happy to see that on the stdlib.

[sam...@google.com]

We have discussed adding a version of the HTTP handler with a Context to the ctxhttp package, but the risk there breaking the codebase into silos around which interfaces you're using.  But I agree it would be more explicit to have an explicit non-nil Context as a parameter instead of a possibly-nil Context in the Request.

[Dave Day]

The comment in Brad's original code snippet says that "Server will always set a Context, but if RequestContext returns a non-nil

value, that Context becomes its parent context." I think that's the right way to go: servers will always set a context (so handlers can assume one exists). However probably http.NewRequest should default to not setting the context – so you would need to add one for requests that you want to send, in unit tests, etc.

[John Beckett]

In my mind at least, a context would simply contain contextual information about a request, and so should always be there - even if at a minimum it's the context from the context.Background() function.  Given that way of thinking about it, I don't see much value in having an explicit non-nil context in a new handler type, as this would effectively create two sets of handler codebases, for little (if any) gain.  

Even in the worst case situation where the server software hasn't set a context, it's trivial to either: 1) Check for a non-nil context in the first handler to touch a request and simply instantiate a Background context; or 2) Write a context getter which instantiates an empty context if the context is nil, or returns the context if it's non-nil.

You received this message because you are subscribed to a topic in the Google Groups "golang-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-dev/cQs1z9LrJDU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-dev+...@googlegroups.com.

[coding...@gmail.com]

Apologies if this is an inappropriate place to reply...

For middleware, I've been working on/with https://github.com/codemodus/chain.  Please refer to https://github.com/codemodus/chain/tree/exp/ctx0 for more info.

[John Jeffery]

The suggestion to store a context inside the http.Request struct does a nice job of making the context available for the purpose of canceling the HTTP request, but it limits the usefulness of having a context.

My understanding is that the best practice for using net/context is that the context.Context should be passed as a function parameter. The godoc documentation at https://godoc.org/golang.org/x/net/context advises:


Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:

If the Handler interface has an extra parameter as per Peter's patch, then it is possible to add to the context in successive layers of middleware, something like:

func Authenticate(ctx context.Context, w http.ResponseWriter, r *http.Request) {
var err error
if ctx, err = auth.Authenticate(ctx, r); err != nil {
http.Error(w, "unauthorized", 401)
return
}

// new ctx has additional information about the caller that can
// be used by successive handlers by calling auth.Identity(ctx)

NextHandler(ctx, w, r)
}

If the context is stored in a struct, then the ability to add to it in successive layers is lost. I understand that the aesthetics of defining a new handler interface with an extra parameter are not appealing, but it is my understanding that this is the only way to use net/context to full benefit.

(FWIW we have a little package, github.com/spkg/httpctx that defineds a new handler interface with both context.Context and error returns. To the best of my knowledge it is only used internally at the company where I work, but it has been extremely useful in my experience).

[Brad Fitzpatrick]

The rule about not putting a context in a struct isn't a hard rule. It's just general guidance to avoid people doing it wrong. If you know the lifetime of your values, putting a context in them is fine, and we know the lifetime of *http.Request for both clients and servers.

[Daved]

Another experiment: https://github.com/codemodus/chain/tree/exp/ctxreq0

A wrapped http.Request is provided with context.Context for middleware.

I found that the most useful aspect of placing the context in the request is that it is available throughout nested handlers and does not require passing around a pointer (see Context Scope for clarification along with the code here).  While I believe that the single added allocation in "BenchmarkNoCancel10" is due to the wrapping required to mock the discussed changes, if cancellation is managed, increased allocations seem inevitable for even cases where the request context goes unused.  Admittedly, this may be due to my own incorrect assumptions about the implementation of what is being proposed, but I thought it worth bringing up.

With 1.5.1 (benchmarks renamed here for clarity):

BenchmarkWCancel10   30000     52198 ns/op    3667 B/op      51 allocs/op

BenchmarkNoCancel10   30000     51653 ns/op    3491 B/op      48 allocs/op

BenchmarkOldChain10   30000     51598 ns/op    3459 B/op      47 allocs/op

[Olivier Poitrey]

We just posted our experience toward net/context at Dailymotion on our engineering blog:

  http://engineering.dailymotion.com/our-way-to-go/

We are definitely on the side of passing the context over instead of storing it in a request. I'm not even sure how this could work with the way net/context is implemented. It would means that you'd have to store the new context each time it's wrapped if you want to pass the newly stored data to the other layers. I think it would defeat the purpose of the chosen design.

For those who don't have time to read the blog post, here is the proposed approach: https://github.com/rs/xhandler. It's very close to Peter's proposal: trying to mimic core http.Handler design as most as possible.

[Daved]

Apologies if I'm confused, but I believe this may clarify usage where context is stored within the request:

https://github.com/codemodus/chain/blob/9b1c83a89fa08abb00acd29219271e4b4be30ccf/README.md#handler-wrapper-and-context-usage-set

[azei...@gmail.com]

The usage is clear, but if we compare net/context to Russian dolls, you would only store the inner doll and expose it to all the handlers stack, skipping all the intermediary dolls.

So if for instance, some parent handler want to listen on their version of the context's Done() channel; Depending on when they start listening (or more precisely when they read the request's Context property), they won't get the same version of the context. They may thus not get notified if another handler down the line calls Cancel() on an outer doll. Tell me if I'm not clear.

There are some good reasons why net/context documentation discourages such practice.

[Daved]

I believe that you're being clear.  My lack of understanding is due to my lack of experience and/or ability to picture the issue in my mind's eye.  Using the experimental branch I provided, would you mind writing up an example of the poor behavior?

[azei...@gmail.com]

I think I've miss-read your example. You are actually not storing the context in the request, so your solution is fine. Sorry about my confusion.

I was referring to previous posts talking about actually storing the context on the http.Request object.

[azei...@gmail.com]

I must be very tired, didn't notice that your example was in a separate branch from your "chain" lib.

So here is an example demonstrating the issue backward: http://play.golang.org/p/J4p3XzDBFC (using your ctxreq0 branch).

Depending on when you read the r.Context, you may get notified about the timeout or not. As the context timeout is set in an inner handler, the outer handler should never see the timeout (/test2 is right here).

Does it clarify?

[Daved]

Thanks for that.

The control flow issue will be a factor for the done channel regardless of whether the context exists within the request struct, or is passed in as a handler arg.  That is, unless the std lib context provides a new/alternate mechanism.

As for mitigation, there is now a SetTimeout func in the library and SetContext was reintroduced to the experimental branch.  Please note that providing a timeout/deadline-based context to SetContext will create grief.  Cancellation management was also moved as it's behavior was quirky.

https://github.com/codemodus/chain/tree/b61c410e20eabce0bd89ccd08e68cd32a9e066bd

At this point, I wouldn't agree that there is a compelling reason to avoid placing context within the request.  The control flow of nested functions should be communicated to the community in general and is a caveat aside.

[jtwa...@linux-consulting.us]

In your example provided at http://play.golang.org/p/J4p3XzDBFC, isn't this at its core just a concurrency bug?  

I do agree that passing context as a arg is easier to reason about and might help avoid this mistake, but fundamentally there is no functional difference between passing as arg vs storing in request.  Passing as an arg provides the local copy implicitly, making it harder to make mistakes, and passing in request requires you to explicitly make a local copy if you need it.

I feel that we need a solution that does not break the stdlib http.Handler interface.

[peter.k...@pressly.com]

Hey guys,

I've put together a simple router that supports chaining, subrouters and inline middlewares - built on net/context - https://github.com/pressly/chi ..the core router is <600LOC.

The plan+hope is net/http will support some kind of request context based on context.Context one day. You'll notice that chi.Handler and http.Handler are very similar and the middleware signatures follow the same structure. One day chi.Handler will be deprecated and the router will live on just as it is without any dependencies beyond stdlib.

[Olivier Poitrey]

Yes this is a concurrency issue for the sake of he demo.

Storing the context opens this kind of issue while passing the context doesn’t. My understanding of the net/context’s design is that changes applied on the context lower in the call stack shouldn’t affect the view of the context in the upper stack.

Take the example of deadline again, if I inherit a context with an existing deadline, I may set a different (usually shorter) deadline on the context I derivate, in order to pass it to network client library. This change must not change the global deadline for the request, just affect the sub-network-roundtrip handled by this library.

You may answer by saying: don’t store it back to the request, but I expect it to be very confusing for people because it hides how the net/context is supposed to work.

[Olivier Poitrey]

Great API, but the fact your Use() method takes an interface{} bugs me. In addition to the lack of static type checking, but it means you can chain context-aware with non-context-aware handlers. With such a mix, your context passing chain will be broken at some point. I don’t see how you handle that?

[Joseph Watson]

I agree with your understanding of net/context.

Isn't the act of saving the derived context back to the request the same thing as passing the derived context to a chained handler via arg? (if the interface included context as an arg)  If you make a copy of the context in the handlers that need to use it, then they will not be affected by downstream re-assignment of the context.

I agree with you that having context in the interface would have been a better solution, but the go promise now stands in the way of that, and I personally want a solution that conforms the std lib.  If we are willing to step outside of std lib interface, we have lots of solutions on github, and they all seem to do something different.

[Olivier Poitrey]

The Peter’s chi framework proves that we could add http.HandlerC interface to the std library and keep both kinds of handlers without breaking the backward compat.

[Olivier Poitrey]

Inspired by chi, I added a chaining capability to xhandler without having to rely on interface{}:

  https://github.com/rs/xhandler#usage

The code is trivial (~40LOC): https://github.com/rs/xhandler/blob/master/chain.go

[Daved]

Isn't the act of saving the derived context back to the request the same thing as passing the derived context to a chained handler via arg? 

*http.Request, being a pointer, allows the context to be accessed throughout nested handlers more easily.  From the master branch of chain - "By not using more broadly scoped context access, a small trick is needed to move data to and from certain points in the request life cycle. For instance, if a final handler adds any data to the context, that data will not be accessible to any wrapper code residing after calls to ServeHTTP/ServeHTTPContext."

And demonstrated:

As arg - http://play.golang.org/p/5t8X10XJJP (not as desired)

As field - http://play.golang.org/p/Qo3g6WzlOZ (as desired)

Regarding shortening timeouts (which is also a control flow issue)...

As arg - http://play.golang.org/p/grkKG2tZ8d (not as desired)

As field - http://play.golang.org/p/OHiWpJ_x9e (as desired)

Sorry for the strange examples..

[Joseph Watson]

Excellent point.  Having the context stored in the request allows for values/timeouts to be passed up AND/OR down the handler chain at your discretion, which is very powerful.  I am starting to like this for more reasons then just the go promise.

[and...@gmail.com]

Please don't take offense, but IMO you present good examples for why context should absolutely not be a part of Request structure. The first pair of snippets is essentially an attempt to pass return value from further handler in middleware chain to previous through context. This is really not better than having an arbitrary mutable field in request struct which everybody modifies at leisure. It just opens the way for the nasty bugs where further handlers in chain replace parent context with some entirely incompatible copy with missing values, wrong Done channels etc.

In the second example, correct me if I'm wrong, you try to set timeout on child context, but without a way to pass its Done channel to parent handler. And to achieve this goal, you simply replace original parent context with derived one, a totally not thread safe practice. You can start waiting on Done channel of original parent context, then run a goroutine that substitutes it with new one with another deadline, the possibilities of failure are endless here. 

FWIW, I think that putting context into pointer parameter, where it can be freely replaced by badly designed downstream hadler, will lead to disaster. It simply undermines the whole idea, and I'm surprised that mr. Ajmani does not object to this proposal.

пятница, 30 октября 2015 г., 2:59:40 UTC+3 пользователь Daved написал:

[Daved]

No offense taken, thank you for being considerate.

I would appreciate it if you would provide examples of some race conditions which concern you.  The code samples I provided do not trigger the race detector, but that does not mean that problems do not exist.  Providing proof of poor behavior will help put a light on this issue.

[Andrew Rodionoff]

Here's what I have in mind: https://play.golang.org/p/A4aJmGPxKo  Note how if you run it few times it may panic or may run successfully. It's a worst kind of bug in my book. Mind you, It's a contrived example, and totally not implying that you will do anything resembling it. But consider that you have no control of what third-party inner middleware will do. And its developers are not concerned about what you do in your outer code, it's not their business.

I think that if you want to have a global-ish thread safe storage to modify at your leisure, then make just that, a RWLock-protected map or something. And store pointer to it in parent context so that child context users will pick it up and mutate it if needed. Otherwise you'll have to make an RWLock member of Request struct to make it safe.

ср, 11 нояб. 2015 г. в 1:34, Daved <coding...@gmail.com>:

--
You received this message because you are subscribed to a topic in the Google Groups "golang-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-dev/cQs1z9LrJDU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-dev+...@googlegroups.com.

[Daved]

The buggy code demonstrates a problem that exists apart from where the context instance is stored.  Alternately consider http://play.golang.org/p/ddvZYNKlZO which will produce the same result with the context passed as an arg.

It seems to me that you are arguing in favor of providing a more padded implementation of the context.Context interface in order to prevent injuries from potential abuses.  Please correct me if I'm misunderstanding the direction you are heading.  However, if I am understanding your perspective correctly: Be mindful that context.Context is an interface and you should be able to produce an implementation that is sufficiently abuse-proof.

[Olivier Poitrey]

With this implementation, your code will consistently panic as the inner activity will never affect the outer’s. That is the expected behavior.

[Andrew Rodionoff]

My original answer got somehow eaten by the engine, I apologize in advance if its duplicate will float up eventually. I'll re-iterate: your code snippet will raise panic with or without race condition, because the current implementation of x/net/context explicitly protects parent context from changes occured to its children. In this respect we have an already well-padded default context implementation, and guidelines in package documentation are serving to further this approach: tree-like information flow from parent contexts to children, not graph-like. It's robust and easy to reason about in multithreaded environment. 

What I'm arguing against is that we put context into request structure inherently mutable through pointer. It's tempting, but goes against the very idea of the contexts. I'll repeat: if somebody needs common storage for upstream and downstream handlers, they should do just that, put a lock around map[string]interface{} and be done with it. 

среда, 11 ноября 2015 г., 11:26:40 UTC+3 пользователь Daved написал:

[Rodrigo Moraes]

On Wednesday, November 11, 2015 at 9:55:47 AM UTC-2, Andrew Rodionoff wrote:

What I'm arguing against is that we put context into request structure inherently mutable through pointer. It's tempting, but goes against the very idea of the contexts. I'll repeat: if somebody needs common storage for upstream and downstream handlers, they should do just that, put a lock around map[string]interface{} and be done with it. 

I agree with these points. The dilemma is: set it as a Request field (which sounds wrong given its mutability), or define a new HandlerC interface (which sounds the correct way but fragments the community a little bit). In the more immediate scenario, the first option sounds attractive; in long road, the second option looks more solid.

-- rodrigo

[Andrew Rodionoff]

Well, I think that it's better to leave the whole idea alone than botch up the http library with improper context addition. Besides, it's so simple to incorporate contexts into http handler flow, that we perhaps don't really need change in existing net/http library at all. What would be really great instead is official adoption of net/context package into standard library. That's what I really hope and wait for.

пятница, 4 декабря 2015 г., 10:34:06 UTC+3 пользователь Rodrigo Moraes написал:

[Matt Silverlock]

> I agree with these points. The dilemma is: set it as a Request field (which sounds wrong given its mutability), or define a new HandlerC interface (which sounds the correct way but fragments the community a little bit). In the more immediate scenario, the first option sounds attractive; in long road, the second option looks more solid.

On the topic of the API and user experience, I'm in agreement with Rodrigo here, but it's definitely not clear-cut:

1. http.Request provides implicit, automatic support going forward, but will strictly require Go 1.7+ and is effectively backwards incompat (any library using r.Context can only interop with 1.7+). 

2. Adding a (e.g.) http.HandlerC with a ServeHTTPC(context.Context, http.ResponseWriter, *http.Request) method to net/http allows some degree of backwards compatibility—you could start supporting this now, pre-1.7, if the design was locked-in—without forcing package users to be on the latest version of Go. What's not clear about this approach is how you might support it in net/http: do you add http.HandleFuncC/HandleC(path string, http.HandlerC) methods to *http.ServeMux? You would also need to determine whether those methods instantiate a context.Background() for the handlers they call (or not).

Right now we have multiple competing implementations and interface names/method names—just look through this thread to see ServeHTTPContext, ServeHTTPC, etc.—which makes it hard for library authors and near-impossible for application authors to integrate context.Context using packages into their applications.

[grat...@gmail.com]

On the server side, I think there should also be context passed during the creation of a server, so it can be terminated "the context way".

In many cases an http request's context should not be child of the server's one; because some users would like to achieve graceful shutdown. Stop listening for new requests but still finish ongoing ones.

I also believe some special server would like to stop the ongoing requests with the server.

So I believe a mix of both approaches would be nice:

1/ Put the context of the server in the request. Handler can use it, or not, but should not set it. ( races !! )

2/ Use ServeHTTP just like before, but define a set of contextualised ServeHTTP, Handler and HandlerFunc ( unused by ListenAndServe and http.Handle ). Something like :

type ContextualisedHandler interface {

ServeHTTPC(context.Context, http.ResponseWriter, *http.Request)

}

Now a handler has the server's ctx, and might use it or not.

But the most interesting part would be to escalate a non contextualised request ton a contextualised one.

For example on appengine one would do :

func AppengineContext(next http.ContextualisedHandler) http.Handler {

    fn := func(w http.ResponseWriter, r *http.Request) {

        next.ServeHTTPC(appengine.NewContext(r), w, r) // now every subhandler must be an HandlerC 

    }

    return http.HandlerFunc(fn)

}

Or if I wanted my request to stop right away with server:

func StopWithServerContext(next http.ContextualisedHandler) http.Handler {

    fn := func(w http.ResponseWriter, r *http.Request) {

        next.ServeHTTPC(r.Context, w, r)

    }

    return http.HandlerFunc(fn)

}

Now defining contextualised handler signatures would be a nice way to not split the community whilst not forcing any usage.

PS: Not so fond of the name 'ContextualisedHandler'.

[s...@ward.io]

After considering all of the approaches here, I am starting to suspect that maybe a "standard middleware interface" is actually a harmful idea, and should not be encouraged with changes to the standard library.  While I agree that we should prevent the "constant invention of inoperable handlers," it seems to me that the real reason we have this problem is that we have wrongly assumed that handlers should be the mechanism of code reuse.

Let's say some middleware expects a context value to exist (even optionally), like a request ID.  We already have a way of expressing that: arguments for functions and receiver values.  But the community wants a uniform interface to chain different things together, so now we find ourselves hiding context values into generic bags.  Well when you pull things out of the bag again you are actually accepting statically typed arguments through a backdoor.  That means you're going to have type assertions and things that are virtually the same as type assertions, such as the "extractors" which pull statically typed values out of contexts (e.g. thing, ok := things.FromContext(ctx)). So this middleware approach is systematically leading people to circumvent the type system in the hopes of achieving widespread code reuse.

Whenever we write handlers with "context" bags, we can no longer glance at the arguments of the function to learn what it needs.  Instead we use runtime type checks in the implementation, which creates hidden dependency graphs.  That's not what developers should expect from a statically typed language.  So I think this is proving to be a counterproductive approach.

If the main goal of contextualized middleware is to produce highly reusable HTTP components such as routers, session trackers, logging, etc., then given the above, how can we accomplish that goal?  It seems to me that discouraging the use of handler function types as a mechanism for attaching unknown functionality together would actually *improve* the modularity and reusability of our packages.  Whenever a Go web application needs to use some functionality across several of its handlers, it can do so by dropping the functionality into its own statically typed pipeline wherever it is appropriate to do so.  If a package is trying to tell developers how to structure their HTTP pipelines, then it is actually *harder to reuse* than a package which only exports a bunch of completely unique methods and types.  This approach is more verbose, but it is also safer, easier to read, and allows the most reusability of packages.

tl;dr instead of working around the problem with contexts, make it easier to write statically typed pipelines.

On Thursday, October 15, 2015 at 11:32:43 AM UTC-7, peter.k...@pressly.com wrote:

Just wondering about the interest level and any suggested approaches for adding context.Context-based http handler support?

I submitted a patch that adds a context.Context handler interface to the existing net/context/ctxhttp (https://github.com/golang/net/tree/master/context/ctxhttp) package and looking for some feedback.

The approach:

* First steps in a context.Context http handler world - added to a core “support” subpackage like ctxhttp

* Control a request - timeout, cancel, store values

* Standardized middleware that require a request context - versus the constant invention of inoperable handlers. See the different signatures of the popular mux libraries: goji, martini, gin, httprouter, gorilla, negroni, and it goes on.

* Optional and compatible - not to compete or break the standard library

Patch: https://go-review.googlesource.com/#/c/15620/

Code diff:

* https://go-review.googlesource.com/#/c/15620/3/context/ctxhttp/ctxhttp.go

* https://go-review.googlesource.com/#/c/15620/3/context/ctxhttp/ctxhttp_test.go

[Manlio Perillo]

Il giorno venerdì 11 marzo 2016 17:30:22 UTC+1, s...@ward.io ha scritto:

After considering all of the approaches here, I am starting to suspect that maybe a "standard middleware interface" is actually a harmful idea, and should not be encouraged with changes to the standard library.  While I agree that we should prevent the "constant invention of inoperable handlers," it seems to me that the real reason we have this problem is that we have wrongly assumed that handlers should be the mechanism of code reuse.

The author of WSGI wrote what *can * happen when you start to offer a standard middleware interface:

http://dirtsimple.org/2007/02/wsgi-middleware-considered-harmful.html

> [...]

Manlio

[kevin.g...@sendgrid.com]

Adding context.Context as a param in a new handler type is not something we need the stdlib for (it's a handful of lines of code, as you've demonstrated). Many have explored this approach and many have avoided it due to the verbosity. If we're going to bake Context into net/http, the most reasonable argument I've seen so far is to add it as a new field to *Request.

[peter.k...@gmail.com]

Hey Sam,

You bring up some great points, and it's something I considered early on when thinking about service design in Go too. That is, checking a bag of values from a chainable context has two issues as I see: 1. namespace conflicts and explicitness of arguments, just like the article Manlio pointed out and 2. missing some static type checking of the values in the bag, even with type assertion

But, the concerns of a mixed bag of values in a request context and static type checking go beyond just http services and apply to all uses of context.Context for passing arguments. IMO, it's certainly no silver bullet (nothing is), but seeking the balance of idiomatic code, productivity, module reuse, composability, and signaling -- well at least for my team and I, it's been working as a robust, testable, maintainable and moving code base among a team of six.

I have some ideas for both 1 & 2 concerns, but it's really just another option, doesn't mean all http services should be written this way -- for example, gokit has a similar but different approach and can be argued to make even more robust services with more flexible transports, but, depends if you need a knife or a sword.

for problem #1 (mixed value bag): one can try a per-service struct type that implements context.Context, and setting the request.Context (from latest proposal) to that object, which offers a predictable structure and more safety.

for problem #2 (static type checking the bag): static analysis for context.Context values :)

[kevin.g...@sendgrid.com]

The reason net.Context value keys are interface{} is precisely to allow for namespacing.

type key int

ctx = context.WithValue(ctx, key(0), someval)

Since there's no way to iterate over the stored values (you have to access values by a keys you already know about), other packages cannot access any value you store with an unexported key type. It's perfect namespacing.

On Sunday, March 13, 2016 at 10:33:37 AM UTC-6, peter.k...@gmail.com wrote:

That is, checking a bag of values from a chainable context has two issues as I see: 1. namespace conflicts...

[Denis Karataev]

Hi, I have just 1 small question: When I see context in debugger (delve) I cannot see inner contexts, it shows just address in memory for inner context. Could we somehow make it convenient? Or maybe there is already some way but I just don't know about it?

Thanks.