One channel per HTTP2 stream #6842
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
|
for review: it may be easier to look at 4013917 as its own change, it does most of the refactoring of the HTTP pipeline setup. The subsequent commits change the pipeline setup for the multiplexing. |
yawkat
added
the
status: next major version
|
SonarCloud Quality Gate failed. |
yawkat
added a commit
that referenced
this pull request
Jun 30, 2022
The changes in #6842 planned for 4.0 will break compatibility of `ChannelPipelineCustomizer` users that modify the server pipeline. This patch introduces a new `ServerNettyCustomizer` API that is more generic and, importantly, allows for more granularity when waiting for the stages of pipeline construction. Also included in this patch is an extensive test suite that tests the logbook customizer example from the documentation in all scenarios we support (raw http1, tls alpn, and h2c support).
graemerocher
pushed a commit
that referenced
this pull request
Jul 28, 2022
The changes in #6842 planned for 4.0 will break compatibility of `ChannelPipelineCustomizer` users that modify the server pipeline. This patch introduces a new `ServerNettyCustomizer` API that is more generic and, importantly, allows for more granularity when waiting for the stages of pipeline construction. Also included in this patch is an extensive test suite that tests the logbook customizer example from the documentation in all scenarios we support (raw http1, tls alpn, and h2c support).
# Conflicts: # http-server-netty/src/main/java/io/micronaut/http/server/netty/HttpPipelineBuilder.java # http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/FileTypeHandler.java # http-server-netty/src/main/java/io/micronaut/http/server/netty/types/stream/NettyStreamedCustomizableResponseType.java
|
Kudos, SonarCloud Quality Gate passed! |
|
@graemerocher @timyates can you review this, it's blocking #5621 |
timyates
approved these changes
Sep 27, 2022
...-netty/src/test/groovy/io/micronaut/http/server/netty/handler/accesslog/AccessLogSpec.groovy
Show resolved
Hide resolved
graemerocher
approved these changes
Sep 27, 2022
yawkat
added a commit
that referenced
this pull request
Oct 19, 2022
This PR adds a new connection pooling infrastructure to the HTTP client. The biggest changes are in ConnectionManager, which contains network setup code, and in the new PoolResizer class, which handles pool dynamics (it calls into ConnectionManager.Pool to assign requests, establish new connections, etc, but does no network setup by itself). Changes in no particular order: - The `http-version` config setting has been replaced by two settings. `plaintext-mode` applies to `http` URLs and can be set to either `HTTP_1` or `H2C`. `alpn-modes` applies to `https` URLs, and determines the protocols that should be supported in protocol negotiation (it's a list, with the supported values `h2` and `http/1.1`). The old http-version setting remains for compatibility, and should remain for 4.0. It is also used in some test cases. - connection pooling now applies to all connections (except websockets), not just exchange calls, and it is enabled by default. - HTTP2 connections now use a channel-per-stream model (like #6842), and allow concurrent requests on one connection. - Connection pool configuration is more fine-grained, with separate settings for HTTP1 and HTTP2. - Pooled connections are now tracked using netty's resource leak detection, to avoid "dangling" connections in the pool (note: resource leak detection is still only tested widely for the tests in the http-server-netty module atm). - One minor fix in the http server that came up in the test suite because connection pooling is on by default now. There are still some TODOs in this PR. Some relate to pending netty improvements (netty/netty#12827 , netty/netty#12830), but work fine for the moment. Some are future improvements that could be made, but that I want to do in separate PRs.
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.
This PR refactors the
NettyHttpServerto have a more straight-forward pipeline setup in a separate class (HttpPipelineBuilder), and then uses netty'sHttp2MultiplexHandlerto multiplex each HTTP2 stream from one connection to its ownChannel.Multiplexing improves compatibility with HTTP 1 handlers downstream. While HTTP 1 handlers do not need to be aware of multiple concurrent request or response streams, HTTP 2 handlers do. With multiplexing, the HTTP 1 handlers only manage one HTTP 2 stream each, so concurrent requests are not an issue anymore.
This change prevents a class of bugs related to faulty
STREAM_IDhandling, such as #6785. It also adds a new feature, concurrent streaming responses (tested in this PR byHttp2ConcurrentStreamSpec).While all tests pass, this is a bit of a risky change. We expose a lot of the handler pipeline to downstream users, and this PR changes that pipeline fundamentally. For example, the
SslHandlerisn't visible on the pipeline anymore, which might lead downstream code to think SSL is disabled (this caused the test failure fixed in c9456e2). It's possible that there are similar downstream dependencies on the structure of the pipeline in other modules. This is why I'm making this PR to 3.4.x, so that we can see any issues as early as possible.