-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
http3: add ConnContext to the server #4230
Conversation
ConnContext can be used to modify the context used by a new http Request.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This check is ✔️ now |
Have you seen the discussion in #3603? Is this PR attempting to resolve that issue? If so, can you explain how your PR resolves the problems raised there? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4230 +/- ##
==========================================
- Coverage 84.15% 84.15% -0.00%
==========================================
Files 149 149
Lines 15386 15378 -8
==========================================
- Hits 12948 12941 -7
+ Misses 1941 1940 -1
Partials 497 497 ☔ View full report in Codecov by Sentry. |
I had not actually seen that discussion. Thanks for pointing it out. I would generally agree with the motivation behind it. The way I use Here's a concrete example the wrapper for I have something similar for With
With
Then, when the handler needs to access the wrapper's data, it does something like:
The alternative I've been using so far is to create a new
It works, but it is not very elegant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add an integration test for this in integrationtests/self/http_test.go
? The test should also assert that this statement remains true:
The provided ctx has a ServerContextKey value.
done |
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rthellend! This looks good to me now.
I'll keep the PR open for a few more days to give @omz13, @carljkempe and @mattrobenolt a chance to chime in.
This will definitely make it into the next release, I just added it to the milestone.
@marten-seemann What happened to waiting a few days? A quick look and it looks ok. 👍 Now its in master I guess I have something to play with 🎉 |
### Description Now that `ConnContext` is available in `http3.Server`, we don't need to create a new `http3.Server` for every new connection. Depends on quic-go/quic-go#4230 ### Type of change * [ ] New feature * [x] Feature improvement * [ ] Bug fix * [ ] Documentation * [ ] Cleanup / refactoring * [ ] Other (please explain) ### How is this change tested ? * [x] Unit tests * [ ] Manual tests (explain) * [ ] Tests are not needed
* Add ConnContext to http3.Server ConnContext can be used to modify the context used by a new http Request. * Make linter happy * Add nil check and integration test * Add the ServerContextKey check to the ConnContext func * Update integrationtests/self/http_test.go Co-authored-by: Marten Seemann <[email protected]> * Update http3/server.go Co-authored-by: Marten Seemann <[email protected]> --------- Co-authored-by: Marten Seemann <[email protected]>
I might be misunderstanding the implementation here, but I just started poking around, and one thing different, is this ConnContext gets called per-http3 request, not per-QUIC connection, like I'd anticipate. In TCP based http handler, we get one ConnContext called the first time the TCP stream is connected, and that is called once for the duration of the TCP connection. From my testing now with quic-go, it seems every single HTTP3 request triggers the ConnContext, which is not what I'd anticipate. This also might be more my misunderstandings of how QUIC works or some implementation detail, just wanted to note the behavior differs than anticipated. Without a ton of understanding right now, the RemoteAddr() on the quic.Connection even reflects the same remote port, so it seems to suggest it's the same QUIC stream. But again, might be my lack of understanding of QUIC here. I've tested using |
@nanokatze Any thoughts on this? I would've expected |
@marten-seemann it doesn't happen in the http2 code path, it's purely in net/http right after Accepting a TCP connection. So it is called as the first thing once a TCP connection is accepted, and this context is used for the lifetime until it disconnects. For HTTP3/QUIC, I'd anticipate a similar behavior, once a QUIC connection is established, we'd share the context for the lifetime. |
Thank you @mattrobenolt! This makes sense to me. Unless anyone here disagrees, I'd say we should fix this as you suggest. I opened #4422 to track this. Given that I'm working on a big refactor of the http3 package for #3522, I suggest holding off on a fix until that refactor has landed, otherwise we'll have deal with a ton of merge conflicts. |
No worries. The implementation as-is replaces the hack I had in place which was also per-request and not per-connection. So swapping to using this should yield a free win once that's fixed to behave more correct. :) It's just some minor optimization for performance to do some computations and cache them for the lifetime of a connection when possible. |
The difference is that |
This is where my knowledge of QUIC and details may differ. So let me explain why I use this functionality and what my goal is. A simple example that we leverage is binding a logger to the context with the RemoteAddr. Then everywhere down the chain spawning from that connection should have the same IP address for multiple requests sharing that same connection. In structured logging sense, effectively doing a I would expect one QUIC connection to have the same RemoteAddr which multiple streams multiplex over. Or are my fundamental understandings here wrong? |
Not necessarily. In principle, QUIC allows connection migration, which means that the client could migrate, for example from a WiFi interface to a cellular interface. This of course will change its IP address. |
Right, so does that mean there is no start/end for a connection analagous to TCP? Is per-request the most general we can expect to get with QUIC? |
I wouldn't put it that way. You still have an underlying QUIC connection, it's just that the remote address is not necessarily stable. But to give a counter example, if you authenticate on that connection somehow (TLS client certs?), this would apply to all requests sent on this connection. |
That's a good example, we don't do that, but I'd say similar vibes. I understand and would expect the remote addr to not be stable. But I would anticipate some signal for connection/start and end. There are other similar things we can do per-connection that are similar to auth, but we utilize some other caches and memory pooling on a per-connection basis when possible. |
I’m not sure how the ConnContext would help with that. If you want to be able to perform some cleanup at the end of the connection, you can use the ServeQUICConn API. |
I'm not sure what you mean. Unless the reason is that QUIC behaves in a way that's different here than TCP. With TCP I can guarantee a single connection is at minimum the same client. By cleanup, in Go it's just GCing away. But binding stuff in this case to the Context using WithValue or whatever, and maintaining that same Context across multiple requests. Are you suggesting that the QUIC APIs just fundamentally can't support this with ConnContext? If not, this frankly doesn't differ from the context already on a Request and not sure what additional value a ConnContext brings other than having a hook to get the quic.Connection. I don't mind, to be clear, if this is a limitation. I'm just trying to understand the disconnect. As-is currently this doesn't mirror the behavior of the typical net/http ConnContext, and if the behavior is explicitly different it might just be worth documenting that difference. |
How does ConnContext allow you to find out when the underlying connection is closed? |
It doesn't. Maybe I confused things by mentioning that. My intent is that we use this hook to do effectively connection-local caching of some expensive computation rather than per request. In our case, we often have TCP connections lasting many hours to days with thousands to millions of requests per second over 1 connection. So being able to reuse anything connection wide is beneficial. Currently our workloads and traffic over QUIC and HTTP/3 is relatively low, so this optimization hasn't been too important yet. And we rely on just normal Go GC to cleanup the connection-local caching we do when the connection is closed and the Context falls out of scope. So we don't rely or expect this hook to handle connections closing. But as per the TCP hook, it's useful if we can establish one Context that's shared across every request over that same quic.Connection rather than a net.Conn. I hope that helps explain. |
ConnContext can be used to modify the context used by a new http Request. This is equivalent to
ConnContext
inhttp.Server
, but with aquic.Connection
instead of anet.Conn
.