-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Support interleaved requests in access logger #6785
Conversation
`HttpAccessLogHandler` used a channel attribute to manage the `AccessLog` instance for a request. Because `AccessLog` does not support concurrent requests, this breaks on HTTP1 pipelining and on concurrent HTTP2 streams. This patch multiplexes the requests onto multiple `AccessLog` instances. Fixes #6782
73229e8
to
9c6a3de
Compare
Okay there was another issue: It seems like we don't routinely add the stream ID to I will look at the HTTP2 concurrency issues further, but it won't make it into 3.3.0. |
...tty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/HttpAccessLogHandler.java
Show resolved
Hide resolved
...tty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/HttpAccessLogHandler.java
Outdated
Show resolved
Hide resolved
* Support interleaved requests in access logger `HttpAccessLogHandler` used a channel attribute to manage the `AccessLog` instance for a request. Because `AccessLog` does not support concurrent requests, this breaks on HTTP1 pipelining and on concurrent HTTP2 streams. This patch multiplexes the requests onto multiple `AccessLog` instances. Fixes #6782 * Test interleaved access logs with exclusions * Attach stream ID to publisher responses * checkstyle * remove `@Contract` Co-authored-by: Tim Yates <tim.yates@gmail.com>
This PR refactors the NettyHttpServer to have a more straight-forward pipeline setup in a separate class (HttpPipelineBuilder), and then uses netty's Http2MultiplexHandler to multiplex each HTTP2 stream from one connection to its own Channel. 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_ID handling, such as #6785. It also adds a new feature, concurrent streaming responses (tested in this PR by Http2ConcurrentStreamSpec). 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 SslHandler isn'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.
HttpAccessLogHandler
used a channel attribute to manage theAccessLog
instance for a request. BecauseAccessLog
does not support concurrent requests, this breaks on HTTP1 pipelining and on concurrent HTTP2 streams.This patch multiplexes the requests onto multiple
AccessLog
instances.Fixes #6782
I built this PR based on the branch of #6753 to avoid merge conflicts, that needs to be merged first. I'm making this a separate PR because it's a bug that's independent of the exclusion feature.