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

net/http: allow modifying Server's base context #30694

Closed
taralx opened this issue Mar 8, 2019 · 19 comments

Comments

Projects
None yet
7 participants
@taralx
Copy link

commented Mar 8, 2019

This is #16220 revived, as that one is locked due to age.

Contexts are everywhere now, and can carry significant information, like authentication data. It is therefore useful to us to be able to set the base context to something other than context.Background().

(#20956 has a similar request, although that one is context modification per-connection instead of per-listener.)

While it is possible to do this by wrapping the handler and merging the contexts, this is error-prone and requires an additional goroutine to properly merge the Done channels.

The main question is one of API. There are two options that I can think of:

  1. Add a field BaseContext to the http.Server struct.
    Advantages: Keeps the API the same, easy to retrofit, composable with existing systems that take/manipulate http.Server.
    Disadvantages: We discourage storing contexts in structs because it makes it easier to misuse them (e.g. using a stale context).
  2. Add a Context parameter to the Serve call.
    Advantages: Standard way to provide base context; difficult to misuse.
    Disadvantages: API proliferation -- we need ServeContext for each variant of Serve.

cc: @bradfitz
cc: #18997

@gopherbot gopherbot added this to the Proposal milestone Mar 8, 2019

@gopherbot gopherbot added the Proposal label Mar 8, 2019

@matttproud

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Please consult internal issue id no. 34620629 for additional information and stakeholders.

@happygiraffe

This comment has been minimized.

Copy link

commented Mar 9, 2019

Rather than storing a context in http.Server, would storing a context creation function work better? Then the default could be a reference to context.Background.

@taralx

This comment has been minimized.

Copy link
Author

commented Mar 9, 2019

@happygiraffe It's possible, sure, but is a more complex API. What is your use case?

@taralx

This comment has been minimized.

Copy link
Author

commented Mar 15, 2019

I take that back, it's not much more complex. But since we would be calling the function basically at the start of net/http.Server.Serve I'm not sure that it's a win for expressiveness.

This proposal doesn't change the timing of the base context, so it's still one base context per Serve() call. If we were to re-engineer the code to construct the context later, we could have the connection information available, but that's about as late as we can go. Does anyone think that it's worth doing?

(I kind of wonder why http.Server doesn't have a ServeConn interface? Probably nobody has asked for it.)

@matttproud

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

For our use case, we expressly need to be the first to attach data to the request-scoped contexts —thanks to massive legacy use — as we cannot reliably guarantee to be the first callee by handlers enrolled with the http package (think handler chaining). We could be tolerant to accept additional deadline information if something attempts to set a shorter value later.

Between the two proposals, my expectation is a function value to create the request's context could be the most expressive and flexible solution for us:

  • Default (or maximum) deadline for handlers, which is critical for load shedding and other production
    fitness questions.
  • Don't want to preclude having handlers derive from a common distributed trace root ID.
package http

type Server struct {
  // detail elided

  // RequestContext is invoked by the server for each request it receives.  If nil, it defaults to
  // context.Background.
  //
  // The value is passed to each *Request the registered handlers receive.
  RequestContext func() context.Context
}

This also bodes well for supporting ephemeral HTTP servers (think: tests and other time-bound environments) that are not background-/root-scoped.

Another consideration: in the RPC space, we automatically cancel context upon handler completion. This is useful to force users to understand lifetime and remedy resource leaks. Perhaps this becomes part of the contract somehow.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 15, 2019

Change https://golang.org/cl/167681 mentions this issue: net/http: add Server.ConnContext hook to modify context for new connections

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

How about https://golang.org/cl/167681?

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 4e9ea34491..6ab9ef9906 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2542,6 +2542,10 @@ type Server struct {
        // If nil, logging is done via the log package's standard logger.
        ErrorLog *log.Logger
 
+       // ConnContext optionally specifies a function that modifies
+       // the context used for a new connection.
+       ConnContext func(context.Context, net.Conn) context.Context
+
        disableKeepAlives int32     // accessed atomically.
        inShutdown        int32     // accessed atomically (non-zero means we're in Shutdown)
        nextProtoOnce     sync.Once // guards setupHTTP2_* init
@@ -2876,6 +2880,12 @@ func (srv *Server) Serve(l net.Listener) error {
                        }
                        return e
                }
+               if cc := srv.ConnContext; cc != nil {
+                       ctx = cc(ctx, rw)
+                       if ctx == nil {
+                               panic("ConnContext returned nil")
+                       }
+               }
                tempDelay = 0
                c := srv.newConn(rw)
                c.setState(c.rwc, StateNew) // before Serve can return
@matttproud

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Just trying to suss a few details and potential uses for what you propose:

What is the motivation for the input parameters in the signature of func(context.Context, net.Conn)? Are you anticipating supporting client/session scoping with the net.Conn? What context value would the initial parameter contain? Does this still support scoping contexts to individual requests, or are are they just scoped to individual connections?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

The input context is a context.Background() (the baseCtx) + the context.WithValue(baseCtx, ServerContextKey, srv).

So you can get at the Server if you need to from the provided ctx, without closing over the Server. And if we add more variables to the context later (like the associated listener), we can get at those too.

And then the net.Conn is the just-Accepted connection, so you can add context based on properties of who just connected to you based on their address or whatnot. Or you can recognize the concrete type of the net.Conn if you had a fancy net.Listener upstream.

Not sure what you mean by client/session scoping.

And yes, this is per-connection, not per-request. There are already per-requests contexts that would be derived from this one (that are currently derived from the baseCtx). This per-connection context would go between the two.

@matttproud

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Thanks for clarifying; I had completely forgotten about that the user could acquire the server by way of the context. I think we could live with a layer cake similar to what you have proposed. It might be well to have the hierarchy documented somewhere for posterity if this is accepted and implemented.

By client/session scoping, I had imprecisely meant "connection scoping". I was uncertain how connection pooling factored into this if at all (e.g., sticky connection as a session through which multiple requests are made).

@taralx

This comment has been minimized.

Copy link
Author

commented Mar 15, 2019

@bradfitz Can't we already get this by wrapping the Handler? I filed this proposal because I'm trying to avoid merging the http context with an existing context.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

This is before the Handler. And you couldn't get the net.Conn to influence the context before.

The "existing context" that the above proposed hook gets is pretty minimal: it only has the ServerContextKey variable populated, which is a documented feature, so we can't risk letting users supply a hook that might not populate it.

So if you need to set the very base context (because you already have one), it'd need to be earlier I guess.

We could do both, even, and then be done with this. e.g. see patchset 2 of https://go-review.googlesource.com/c/go/+/167681

@taralx

This comment has been minimized.

Copy link
Author

commented Mar 16, 2019

I'm fine with this.

@matttproud

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

I think this will work for us (or at least affords the most reasonable tradeoffs). Thank you for the attention.

@ianlancetaylor ianlancetaylor changed the title Proposal: Allow modifying net/http.Server's base context proposal: allow modifying net/http.Server's base context Mar 20, 2019

@ianlancetaylor ianlancetaylor changed the title proposal: allow modifying net/http.Server's base context proposal: net/http: allow modifying Server's base context Mar 20, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Accepted in proposal review.

@bradfitz bradfitz self-assigned this Mar 20, 2019

@bradfitz bradfitz changed the title proposal: net/http: allow modifying Server's base context net/http: allow modifying Server's base context Mar 20, 2019

@bradfitz bradfitz modified the milestones: Proposal, Go1.13 Mar 20, 2019

@taralx

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

If I add tests to Brad's change, is that all that's needed here?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@taralx, sorry, I can do it tomorrow. I've been a combination of distracted by other things, sick, and traveling. But I'm back tonight & working like normal tomorrow.

@gopherbot gopherbot closed this in 2c802e9 Apr 15, 2019

@matttproud

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Thank you for taking care of this, Brad and JP!

@navytux

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

While it is possible to do this by wrapping the handler and merging the contexts, this is error-prone and requires an additional goroutine to properly merge the Done channels.

First of all I appologize for feedback when this issue is closed. Next I suggest to reconsider about context merging. I'm doing exactly this in my services - please see https://godoc.org/lab.nexedi.com/kirr/go123/xcontext#hdr-Merging_contexts for details.

It is true that xcontext.Merge currently requires extra goroutine. This comes from the fact that stdlib context package 1) does not provide way to parent-child link other context implementations tightly with what is in stdlib already, and 2) does not provide context.Merge. It thus could be fixed by implementing either "2" (preferably) or "1".

Attaching xcontext.Merge documentation for the reference.

Thanks beforehand,
Kirill

/cc @Sajmani


Merging contexts

Merge could be handy in situations where spawned job needs to be canceled whenever any of 2 contexts becomes done. This frequently arises with service methods that accept context as argument, and the service itself, on another control line, could be instructed to become non-operational. For example:

func (srv *Service) DoSomething(ctx context.Context) (err error) {
	defer xerr.Contextf(&err, "%s: do something", srv)

	// srv.serveCtx is context that becomes canceled when srv is
	// instructed to stop providing service.
	origCtx := ctx
	ctx, cancel := xcontext.Merge(ctx, srv.serveCtx)
	defer cancel()

	err = srv.doJob(ctx)
	if err != nil {
		if ctx.Err() != nil && origCtx.Err() == nil {
			// error due to service shutdown
			err = ErrServiceDown
		}
		return err
	}

	...
}

func Merge

func Merge(parent1, parent2 context.Context) (context.Context, context.CancelFunc)

Merge merges 2 contexts into 1.

The result context:

  • is done when parent1 or parent2 is done, or cancel called, whichever happens first,
  • has deadline = min(parent1.Deadline, parent2.Deadline),
  • has associated values merged from parent1 and parent2, with parent1 taking precedence.

Canceling this context releases resources associated with it, so code should call cancel as soon as the operations running in this Context complete.

navytux added a commit to navytux/go123 that referenced this issue May 19, 2019

xcontext: Add note about how Merge could be done efficiently
Pygolang does it. However my comment on Go issue tracker about this
topic got no feedback at all:

golang/go#30694 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.