Skip to content
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

Add support for Google context #90

Closed
pjebs opened this issue Jun 7, 2015 · 22 comments
Closed

Add support for Google context #90

pjebs opened this issue Jun 7, 2015 · 22 comments
Assignees

Comments

@pjebs
Copy link
Contributor

pjebs commented Jun 7, 2015

Are you thinking of updating negroni (I.e. v2) to utilize Google context.

That seems to be idiomatic now. Everyone seems to using it. Or should I attempt to create a fork?

http://godoc.org/golang.org/x/net/context

https://blog.golang.org/context

@ericwq
Copy link

ericwq commented Jun 8, 2015

it seems there is already exist one. gorilla context.

@pjebs
Copy link
Contributor Author

pjebs commented Jun 9, 2015

that's a global context. Doesn't scale as well.

@jszwedko
Copy link
Contributor

Are you talking about passing a context through the middlewares? I like that negroni currently works with http.HandlerFunc so I wouldn't want to see the signature modified -- how were you thinking that it would be implemented @pjebs?

@robert-uhl
Copy link

Well, negroni works with http.HandlerFunc & http.Handler by wrapping them

How about this?

type Handler interface {
    ServeHTTP(ctx context.Context, rw http.ResponseWriter, r *http.Request, next http.HandlerFunc)
}

Just as now, Wrap calls an http.Handler and then calls next itself, it could receive (and ignore) ctx, call http.Handler and the call next itself.

Then:

func (n *Negroni) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
    ctx := context.Background()
    n.middleware.ServeHTTP(ctx, NewResponseWriter(rw), r)
}

And so forth.

@robert-uhl
Copy link

(as an aside, I agree with @pjebs: Google contexts seem to be the most idiomatic way to do this now. They work, they work under load, they work while avoiding global state, and they work well.

@pjebs
Copy link
Contributor Author

pjebs commented Dec 2, 2015

@mikegleasonjr
Copy link

I forked negroni and httprouter to pass down a Google Context, check my repo. All up to date and tests passing. I haven't updated the readme though.

@mikegleasonjr
Copy link

Just to follow up on my previous comment, I created a standard for myself for HTTP handlers to include a Google Context. Then I forked negroni and httprouter to follow my "standard". If anyone is interested on how they work together I can post some code. In the meantime here's my proposed new standard (to stop this nonsense of absolutely trying to be net/http Handler compliant when it's trivial to write wrappers to switch from one type of Handler to another) Anyway here it is: https://github.com/mikegleasonjr/go/blob/master/net/http/ctx/ctx.go

@pbgc
Copy link

pbgc commented Feb 2, 2016

Don't see the need to fork negroni to use Google context.
I use it all the time with just a little glue code: https://bitbucket.org/pbgc/nctx/overview

@robert-uhl
Copy link

I'm currently doing the same with my own glue code, but it'd be nice if negroni supported this natively.

@meatballhat meatballhat changed the title Google context Add support for Google context May 4, 2016
@meatballhat meatballhat self-assigned this May 4, 2016
@pjebs
Copy link
Contributor Author

pjebs commented Jun 3, 2016

It looks like Google Context will be officially part of standard package starting Go1.7: https://tip.golang.org/doc/go1.7#context

@jszwedko
Copy link
Contributor

Indeed it is, but appears that it will be directly accessible on the Request struct: https://tip.golang.org/pkg/net/http/#Request.Context so I don't think there will need to be any changes to negroni's API to take advantage of it.

@pjebs
Copy link
Contributor Author

pjebs commented Jun 15, 2016

I wonder if gorilla mux will load url routing values into the context rather than using their own Vars function to extract the route variables.

@rabeesh
Copy link

rabeesh commented Aug 7, 2016

One solution for it, wrap around ResponseWriter.

type ContextResponseWriter struct {
    http.ResponseWriter
    Context context.Context
}


// conext value
type Context struct {}

func (Context)  ServeHTTP(w http.ResponseWriter, r *http.Request, n http.HandlerFunc) {
    ctx := context.WithValue(context.Background(), "domain", "universal.webshop")
    ctx = context.WithValue(ctx, "version", "v1")
    ctxrw := &ContextResponseWriter{w, ctx}
    n(ctxrw, r)
}

This is not good solution, and read it value.

ctx := w.(*app.ContextResponseWriter).Context
version, _ := ctx.Value("version").(string)

fmt.Println("version:", version)

@yanfali
Copy link

yanfali commented Aug 27, 2016

I ripped out gorilla/context recently and now use Context to pass around parameters taken from the router library I use (httprouter). It's very easy in middleware/adapters to stuff values in the request context and then pull them back out again in a handler or middleware.

@catc
Copy link

catc commented Oct 3, 2016

@yanfali do you happen to have a gist of this? i'm trying to accomplish the same thing

@yanfali
Copy link

yanfali commented Oct 3, 2016

@catc this only works in go1.7, so if you are stuck on an earlier version of go, this won't help.
net/http now has a WithContext method.

Basically your middleware uses the following pattern:

func ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
    ctxt := context.Background()

   // ...middleware does stuff here...

    ctxt = context.WithValue(ctxt, "key", "some new value I want downstream")

    if next != nil {
        next(w, r.WithContext(ctxt))
    }
}

Downstream of the middleware, like in a handler or other middleware, you can use .Context() to pull values out.

@yanfali
Copy link

yanfali commented Oct 3, 2016

@catc specifically you can use this pattern to stuff the Param value from httprouter into request context and then extract parameters from the router downstream in handlers or other middleware.

@catc
Copy link

catc commented Oct 3, 2016

@yanfali I am using 1.7, so that worked perfectly. Thanks!

@heyimalex
Copy link

heyimalex commented Nov 18, 2016

@yanfali I think that when creating the new context you're supposed to extend from the one already on the request. Because the parent of your replacement context is context.Background, it won't actually be closed when the top-level ServeHTTP returns (which is supposed to happen).

func ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
-    ctx := context.Background()
+    ctx := r.Context()

    // ...middleware does stuff here...

    ctx = context.WithValue(ctx, "key", "some new value I want downstream")

    if next != nil {
        next(w, r.WithContext(ctx))
    }
}

@yanfali
Copy link

yanfali commented Nov 18, 2016

Agreed. Sorry if the example was misleading

On Thu, Nov 17, 2016, 16:52 Alex Guerra notifications@github.com wrote:

@yanfali https://github.com/yanfali I think that when creating the new
context you're supposed to extend from the one already on the request.
Because the parent of your replacement context is context.Background, it
won't actually be closed when the top-level ServeHTTP returns (which is
supposed to happen).

func ServeHTTP(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {- ctx := context.Background()+ ctx := r.Context()

// ...middleware does stuff here...

ctx = context.WithValue(ctx, "key", "some new value I want downstream")

if next != nil {
    next(w, r.WithContext(ctx))
}

}


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#90 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACK5U04ByahJcDulS27wU2Ppxnff-BI3ks5q_PbpgaJpZM4E60Fp
.

@yanfali
Copy link

yanfali commented Jan 3, 2017

I think @jszwedko you could probably close this issue as it's not really an issue as of go1.7

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

No branches or pull requests