add a ConnContext to the quic.Config, remove it from the http3.Server

[marten-seemann]

As pointed out by @mattrobenolt in #4230 (comment), ConnContext should only be called once after the QUIC connection has been accepted.

Currently blocked on the http3 refactor for #3522.

[marten-seemann]

I'm actually not sure how we'd implement this. Currently, we use the stream context:

quic-go/http3/server.go

Lines 546 to 555 in da410a7

	ctx := str.Context()
	ctx = context.WithValue(ctx, ServerContextKey, s)
	ctx = context.WithValue(ctx, http.LocalAddrContextKey, conn.LocalAddr())
	ctx = context.WithValue(ctx, RemoteAddrContextKey, conn.RemoteAddr())
	if s.ConnContext != nil {
	ctx = s.ConnContext(ctx, conn)
	if ctx == nil {
	panic("http3: ConnContext returned nil")
	}
	}

This seems to be the right thing to do, because the request is inherently bound to the stream.

If we want to implement this change, we'd need a context that derives from both the stream and the connection, which is not possible.

Any ideas @mattrobenolt?

[mattrobenolt]

If we want to implement this change, we'd need a context that derives from both the stream and the connection, which is not possible.

Yeah, I'd suspect that's correct. You'd want one originating at the Connection itself, each Stream derives from that.

Not speaking about logistic for the implementation, but conceptually that's my expectation.

Connection -> Stream -> Request

Context is "forked" at each of those. Each Stream inherits from the Connection, and each Request inherits from the Stream. ConnContext is called when the Connection is established, and each Stream, thus each Request, inherits from that top level Connection.

Am I right in that each Stream is 1 HTTP3 request? Or can one Stream handle multiple HTTP3 requests?

[marten-seemann]

The problem here is that we're dealing with the HTTP/3 layer, but streams are opened on the QUIC layer.

Maybe that's the solution: We should add a ConnContext callback to the quic.Connection, not the http3.Server! This would allow us to get rid of the ConnectionTracingKey then. This would be really nice, it never felt like the right abstraction.

Unfortunately, this is a much bigger change, and we might want to implement #3862 first (but maybe not strictly necessary?).

[mattrobenolt]

I would agree, it would naturally be a better hook on the quic layer.

I'm curious how this will translate over to x/net/quic, but I have little skin in the game except for being a user. :)

[marten-seemann]

Unfortunately, this is a much bigger change, and we might want to implement #3862 first (but maybe not strictly necessary?).

Thinking about this more, I don't think this is even necessary. The API will be more useful, but it's pretty much orthogonal to this issue.

I'm curious how this will translate over to x/net/quic

Oh well, nothing we have to worry about here :)
We'll continue evolving the API here whenever we think it makes sense.

[rthellend]

A simple way to implement the original idea would be to use http3.connection:

• Call Server.ConnContext() in Server.handleConn() right before newConnection() is called.
• Set the returned value as hconn.requestBaseCtx

Then in Server.handleRequest():

        ctx := conn.requestBaseCtx
        ctx = context.WithValue(ctx, ServerContextKey, s)
        ctx = context.WithValue(ctx, http.LocalAddrContextKey, conn.LocalAddr())
        ctx = context.WithValue(ctx, RemoteAddrContextKey, conn.RemoteAddr())
        req = req.WithContext(ctx)

Arguably, the other values could also be set in handleConn.

[rthellend]

Nevermind. My suggestion won't work. The only way this can work is if the stream ctx is tied to the conn ctx.

[marten-seemann]

I opened two PRs, #4507 and #4510.

#4507 adds a ConnContext function to the quic.Transport, allowing the application to control the context used at the QUIC layer.
#4510 uses the connection context instead of the stream context for the server's http.Request. I'm not sure why the original issue here states that we can remove the context from the http3.Server. Am I missing something?

@mattrobenolt @rthellend Could you take a look at these PRs, and maybe even try them out?

[rthellend]

The problem with #4510 is that the context in the http.Request is no longer tied to the stream. The main purpose of the Request's context is to signal when the stream is closed so that any operation started by the request handler can be aborted. So, any solution to this issue must preserve that functionality.

The solution needs to satisfy these requirements:

• the context of http.Request is the quic.Stream context or a descendant of it.
• the context of http.Request is the context returned by ConnContext() or a descendant of it.
• ConnContext() must be called with the quic.Connection that owns the stream.

The current implementation satisfies these requirements. What we're adding is a new requirement that says ConnContext() must be called at most once per connection.

To me, that means that:

1. the value returned by ConnContext() must be an ancestor of the quic.Stream context, and
2. ConnContext() must be called before any stream is created on the connection.

Part 2 can be done at the quic level or at the http3 level, depending on how part 1 is implemented.

It's possible that #4507 is doing the implementation at the quic level, but I'll need more time to understand what it is doing.

[rthellend]

I think #4507 would be an improvement, but it wouldn't help my use-case. The ctx is buried too deep in the quic implementation.

I have an alternative suggestion.

It looks like the connection's context is used as base context for the streams.

So, if we added a WithContext() (similar to https://pkg.go.dev/net/http#Request.WithContext) to quic.Connection, we could do something like this:

diff --git a/http3/server.go b/http3/server.go
index 4042da33..8372955a 100644
--- a/http3/server.go
+++ b/http3/server.go
@@ -422,6 +422,15 @@ func (s *Server) removeListener(l *QUICEarlyListener) {
 }
 
 func (s *Server) handleConn(conn quic.Connection) error {
+       ctx := context.WithValue(conn.Context(), ServerContextKey, s)
+       if s.ConnContext != nil {
+               ctx = s.ConnContext(ctx, conn)
+               if ctx == nil {
+                       panic("http3: ConnContext returned nil")
+               }
+       }
+       conn = conn.WithContext(ctx)
+
        // send a SETTINGS frame
        str, err := conn.OpenUniStream()
        if err != nil {
@@ -534,15 +543,8 @@ func (s *Server) handleRequest(conn *connection, str quic.Stream, datagrams *dat
        }
 
        ctx := str.Context()
-       ctx = context.WithValue(ctx, ServerContextKey, s)
        ctx = context.WithValue(ctx, http.LocalAddrContextKey, conn.LocalAddr())
        ctx = context.WithValue(ctx, RemoteAddrContextKey, conn.RemoteAddr())
-       if s.ConnContext != nil {
-               ctx = s.ConnContext(ctx, conn.Connection)
-               if ctx == nil {
-                       panic("http3: ConnContext returned nil")
-               }
-       }
        req = req.WithContext(ctx)
        r := newResponseWriter(hstr, conn, req.Method == http.MethodHead, s.Logger)
        handler := s.Handler

Edit: it might be easier to implement a SetContext() function instead of WithContext()

[marten-seemann]

The problem with #4510 is that the context in the http.Request is no longer tied to the stream. The main purpose of the Request's context is to signal when the stream is closed so that any operation started by the request handler can be aborted. So, any solution to this issue must preserve that functionality.

This never properly worked. The context was derived from the send side of the stream, but you really want it to be derived from the receive side of the stream (which doesn't have a context though). #4510 removes the incorrect behavior, and the proper fix is tracked in #4508.

To me, that means that:

1. the value returned by ConnContext() must be an ancestor of the quic.Stream context, and
2. ConnContext() must be called before any stream is created on the connection.

This doesn't work. The connections ConnContext is called before even processing the first QUIC frame. At this point, we don't even know if the connection will negotiate HTTP/3 or some other application protocol.

---

I don't understand where the requirement comes from that the request context is derived from the stream's context. Yes, we need to get cancellation right (see #4508), but we can do this at the HTTP/3 layer. In fact, there's no way around that, since the request needs to be cancelled when ServeHTTP returns.

[rthellend]

This never properly worked. The context was derived from the send side of the stream, but you really want it to be derived from the receive side of the stream (which doesn't have a context though). #4510 removes the incorrect behavior, and the proper fix is tracked in #4508.

I wasn't aware that it wasn't working correctly. The point remains though. The Request context must be canceled when the stream is closed (i.e. when we can't send the response to the client anymore). That's usually done by making the request context a descendant of the stream context, but you're right, there are other ways to make it work.

To me, that means that:

1. the value returned by ConnContext() must be an ancestor of the quic.Stream context, and
2. ConnContext() must be called before any stream is created on the connection.

This doesn't work. The connections ConnContext is called before even processing the first QUIC frame. At this point, we don't even know if the connection will negotiate HTTP/3 or some other application protocol.

Which part doesn't work? If we want to call ConnContext only once per connection, it has to be before the first request is handled. Depending on how the request context is created, ConnContext could be called lazily after the first AcceptStream().

I don't understand where the requirement comes from that the request context is derived from the stream's context. Yes, we need to get cancellation right (see #4508), but we can do this at the HTTP/3 layer. In fact, there's no way around that, since the request needs to be cancelled when ServeHTTP returns.

The http.Request context must be canceled when the response can't be sent to the client anymore. This is so that expensive work in the request handler can be aborted when it won't be needed. How it gets canceled doesn't really matter. IMO, it would be easiest and most intuitive to cancel the stream context and let it cascade automatically to the request context. But, it would also be completely OK to cancel the stream context and then also cancel the request context separately.

[marten-seemann]

This never properly worked. The context was derived from the send side of the stream, but you really want it to be derived from the receive side of the stream (which doesn't have a context though). #4510 removes the incorrect behavior, and the proper fix is tracked in #4508.

I wasn't aware that it wasn't working correctly. The point remains though. The Request context must be canceled when the stream is closed (i.e. when we can't send the response to the client anymore). That's usually done by making the request context a descendant of the stream context, but you're right, there are other ways to make it work.

For sure, and we won't release anything that would break this. That's why #4508 is included in the next milestone. I'll also make sure to add an extra integration test for this. Just trying to disentangle things a bit to keep PRs to a manageable size, sorry for the confusion created.

To me, that means that:

1. the value returned by ConnContext() must be an ancestor of the quic.Stream context, and
2. ConnContext() must be called before any stream is created on the connection.

This doesn't work. The connections ConnContext is called before even processing the first QUIC frame. At this point, we don't even know if the connection will negotiate HTTP/3 or some other application protocol.

Which part doesn't work? If we want to call ConnContext only once per connection, it has to be before the first request is handled. Depending on how the request context is created, ConnContext could be called lazily after the first AcceptStream().

The QUIC layer would call ConnContext right when accepting the connection. That way, the context can be use on the tls.ClientHelloInfo.Context and when initializing the QUIC tracer, both of which happen during the handshake. However, streams can only be accepted after handshake completion, so that would be too late.

The http.Request context must be canceled when the response can't be sent to the client anymore. This is so that expensive work in the request handler can be aborted when it won't be needed. How it gets canceled doesn't really matter. IMO, it would be easiest and most intuitive to cancel the stream context and let it cascade automatically to the request context. But, it would also be completely OK to cancel the stream context and then also cancel the request context separately.

Ok, let me prepare a fix for #4508, building on top of the other two PRs. Then we can see if all the parts work together as I imagine it right now. Would be great if you could test it again then! :)