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

Proposal: Allow to change net/http.Server base context #16220

Closed
gregory-m opened this issue Jun 29, 2016 · 18 comments
Closed

Proposal: Allow to change net/http.Server base context #16220

gregory-m opened this issue Jun 29, 2016 · 18 comments

Comments

@gregory-m
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.7beta2 darwin/amd64

  2. What did you do?
    I'am trying to setup my session store and pass it to handlers via http.Request.Context()

  3. What did you expect to see?
    Some way to do this.
    Like:

    ctx := context.WithValue(context.Background(), seesionStoreKey, SessionStore{})
    
    s := &http.Server{
      BaseContex: ctx
    }
    s.ListenAndServe()
    
    #And use it inside handler:
    store, ok := r.Context().Value(seesionStoreKey).(SessionStore)
    
  4. What did you see instead?

I can't change base http.Request.Context()

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/24660 mentions this issue.

@kardianos
Copy link
Contributor

I'm against this change. If you need to setup a context handler you can always have the default mux pass the request a global handler, setup the context, then pass it on to the actual mux. This also let's you specify a cancel context as well.

How the mentioned CL and example are used, it would just setup the context to be a bag of values. And the only values you could add to it would be static values; there would be no way to inject a "current" value.

@gregory-m
Copy link
Contributor Author

Hello thank you for the response.

The problem with global handler starts if you need to initialize something only once like session store or db (I know its debatable if you want to put your db into context). Before context was moved to stdlib many people including myself was using net/context to eliminate global variables and package.Init() hell.

If we use global handler we still need global variables for all things you need to initialize once.

@kardianos
Copy link
Contributor

kardianos commented Jun 30, 2016

@gregory-m Please consider the following:

// Use a closure:
mux := NewCoolMux()
db := NewPool()

http.ListenAndServe(":8080", http.HandleFunc("/", func(w, r) {
    ctx := context.Background()
    ctx = mydb.NewContext(ctx, db)
    mux.ServeHTTP(w, r.WithContext(ctx))
})

You can also use a type that implements the ServeHTTP method and has a db pool as a field. Then you can setup the context from that as well.

@gregory-m
Copy link
Contributor Author

@kardianos clever and little bit hacky.

I don't know what core team think, but I'am good with close this issue, and see what will happen after 1.7 release and context adoption.

@pciet
Copy link
Contributor

pciet commented Jul 25, 2016

I think with Go 1 an ok application solution is a separate package with the HTTP handling, a private global context variable, and just directly call the global variable methods in your handlers.

@quentinmit
Copy link
Contributor

If we were designing net/http from scratch, I would say that ListenAndServe should take a Context argument. But we can't add that now without breaking the Go1 compatibility guarantee, and I don't think it's worth adding a second ListenAndServeWithContext. :/

So a vague +1 to being able to set the context in the Server struct, even if that's not the best place for it.

@quentinmit quentinmit added this to the Proposal milestone Jul 29, 2016
@bradfitz bradfitz self-assigned this Sep 12, 2016
@bradfitz bradfitz modified the milestones: Go1.8, Proposal Sep 12, 2016
@bradfitz
Copy link
Contributor

@kardianos makes a good point that there are ways to do this as-is. I think I'll close this issue instead for now. I think somebody should wrap up @kardianos's example into a small func or repo somewhere. (I'd take it into https://go4.org/ too) We can revisit this in another release or so, but I imagine we don't add more API for this.

@quentinmit
Copy link
Contributor

@bradfitz With @kardianos's solution, you lose the context information associated with the request by net/http. Such as the cancellation support. How would you replicate this in the handler?

@kardianos
Copy link
Contributor

Replace context.Background() with r.Context().

@quentinmit
Copy link
Contributor

If you do that then you lose propagation from whatever background context the user wants to use.

@kardianos
Copy link
Contributor

Either add the values to it or spin up a routine to wait for either ctx to finish in a select.

@quentinmit
Copy link
Contributor

It's not always possible to add the values to the context. The necessary key may or may not be accessible to the user.

/cc @bradfitz PTAL at my comments here.

@bradfitz
Copy link
Contributor

You can merge the two contexts with https://golang.org/pkg/context/#WithCancel and then cancel it after the inner ServerHTTP is done.

@quentinmit
Copy link
Contributor

Sorry if I'm being dense here. How do you merge two contexts using WithCancel?

@kardianos
Copy link
Contributor

@quentinmit on my desktop now, I'll try to expand.
I'll try to give a few examples, but first, I'd like to better understand your particular use case. In what situation do you have a static context you would like to append to each individual request, but do not have the values that go into the context?

Example 1: append to context using a function

// If you do have the values available:
func defaultContext(ctx context.Context) context.Context {
   ctx = v1.WithContext(ctx)
   /// etc
   return ctx
}
http.ListenAndServe(":8080", http.HandleFunc("/", func(w, r) {
    ctx := defaultContext(r.Context())
    mux.ServeHTTP(w, r.WithContext(ctx))
})

Example 2: assume your bag of values context is opaque. Unsure when this would actually happen in practice.

var bgCtx = MyBackgroundContext

http.ListenAndServe(":8080", http.HandleFunc("/", func(w, r) {
    ctx, cancel := context.WithCancel(bgCtx)
    defer cancel()
    mux.ServeHTTP(w, r.WithContext(ctx))
})

Example 3: assume you have values from an external context AND it has a cancel function. This wouldn't be possible with setting a background context.

// Incoming connection is forwarded from another service with a deadline and specific header values.
// transform the request into a context value.
func deriveRequestContext(r http.Request) context.Context {...}

http.ListenAndServe(":8080", http.HandleFunc("/", func(w, r) {
    rctx := deriveRequestContext(r)
    rctx, cancel := context.WithCancel(rctx)
    defer cancel()
    mux.ServeHTTP(w, r.WithContext(rctx))
})

You could also probably do something fancier by starting a goroutine and waiting on two ctx.Done() in a select, but the above should work. The second example, where you have a bag of opaque values but no deadline seems odd. Could you explain why you have this situation? Examples 1 and 3 seem far more likely.

@bradfitz
Copy link
Contributor

Yeah, sorry, you can't merge two contexts' values without implementing a custom Context type.

But between that and spinning up a goroutine to wait for one's cancelation or deadline as @kardianos mentioned, I think there are sufficient ways to do this otherwise. It's at least not a priority for me for Go 1.8. I'd like to avoid adding API where possible, especially if it might encourage context misuse, and it's still too early to tell.

And the OP's request is easy enough with #16220 (comment)

@tombergan
Copy link
Contributor

FYI, this proposal is going to end up happening via #18997.

@golang golang locked and limited conversation to collaborators Jul 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants