net/http: allow modifying Server's base context

[taralx]

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

[matttproud]

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

[happygiraffe]

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]

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

[taralx]

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]

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]

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

[bradfitz]

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]

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]

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]

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]

@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 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]

I'm fine with this.

[matttproud]

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

[bradfitz]

Accepted in proposal review.

[taralx]

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

[bradfitz]

@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.

[matttproud]

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

[navytux]

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.

[gopherbot]

Change https://golang.org/cl/181260 mentions this issue: net/http: support BaseContext & ConnContext for http2 Server

[gopherbot]

Change https://golang.org/cl/181259 mentions this issue: http2: support getting the Server connection's base context from net/http