Implement http3.Server.ServeListener #3349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ServeListener method added to http3.Server allowing serving from an existing listener ConfigureTLSConfig function added to http3 which should be used to create listeners meant for serving http3.
… context.Canceled
There was a problem hiding this comment.
This already looks very good to me. Thank you for working on this!
Two things:
- See my comment below regarding avoiding the massive callback?
- Can we have tests? ;) We definitely need a unit test in this package. Ideally, we'll also have an integration test in the
integrationtestspackage, where we swap out the server (basically the thing that Caddy will do).
Codecov Report
@@ Coverage Diff @@
## master #3349 +/- ##
==========================================
+ Coverage 85.50% 85.52% +0.02%
==========================================
Files 135 135
Lines 9830 9837 +7
==========================================
+ Hits 8405 8413 +8
+ Misses 1047 1046 -1
Partials 378 378
Continue to review full report at Codecov.
|
|
The tests are fixed now, doesn't seem like the CircleCI fail is related to my changes |
|
Thank you @renbou! |
nmldiegues
pushed a commit
to chungthuang/quic-go
that referenced
this pull request
Jun 6, 2022
* feat(http3): implement serving from quic.Listener ServeListener method added to http3.Server allowing serving from an existing listener ConfigureTLSConfig function added to http3 which should be used to create listeners meant for serving http3. * docs(http3): add note about using ConfigureTLSConfig to ServeListener * fix(http3): stop serving non-created listeners after Server.Close * refactor(http3): return ErrServerClosed once server closes instead of context.Canceled * feat(http3): close listeners from ServeListener as well * fix(http3): fix logger not being setup during ServeListener * test(http3): add unit tests for serving listeners * test(http3): add tests for ConfigureTLSConfig * test(http3): added server hotswapping integration test * fix: race condition in listener tests
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3347
Added a
http3.Server.ServeListener(http3.Server.Serveis already taken and servesnet.PacketConn) method which is can be used to serve content on an already createdquic.EarlyListenerwhich works likehttp.Server.Servein net/http.Originally, however, it was planned for these listeners not to be closed using
http3.Server.Closeso thathttp3.Servers can be switched between without closing/starting another listener. It turned out that this is easier implemented externally (for example Caddy uses "fake close" wrappers around listeners), since there isn't an easy way to interrupt theAcceptrunning inhttp3.Server.serveImplwhich was run using anquic.EarlyListenerwithout closing the listener.Note that
net/http'sServer.Closealso closes the listener passed toServer.Serve, so the behavior is even more similar this way.