Motivation:
`DefaultHttp2UnknownFrame#equals` may produce NPE due to
incorrect comparison of `stream` field.
Modification:
- Fix the `stream` field compare.
- Cleanup usage of class fields: use direct access instead of getters
(because the class is final).
Result:
No NPE in `equals` method.
Motivation:
Classes `AbstractHttp2StreamChannel.Http2StreamChannelConfig`
and `DnsNameResolver.AddressedEnvelopeAdapter` may be static:
it doesn't reference its enclosing instance.
Modification:
Add `static` modifier.
Result:
Prevents a possible memory leak and uses less memory per class instance.
Motivation:
At the moment we use Unpooled.buffer(...) in InboundHttp2ToHttpAdapter when we need to do a copy of the message. We should better use the configured ByteBufAllocator for the Channel
Modifications:
Change internal interface to also take the ByteBufAllocator as argument and use it when we need to allocate a ByteBuf.
Result:
Use the "correct" ByteBufAllocator in InboundHttp2ToHttpAdapter in all cases
Motivation:
At the moment we set release to false before we call writeData(...). This could let to the sitatuation that we will miss to release the message if writeData(...) throws. We should set release to false after we called writeData(...) to ensure the ownership of the buffer is correctly transferred.
Modifications:
- Set release to false after writeData(...) was successfully called only
Result:
No possibility for a buffer leak
Motivation:
We did miss to take Http2FrameCodecBuilder.isValidateHeaders() into account when a Http2FrameWriter was set on the builder and always assumed validation should be enabled.
Modifications:
Remove hardcode value and use configured value
Result:
Http2FrameCodecBuilder.isValidateHeaders() is respected in all cases
Motivation:
Error: Class that is marked for delaying initialization to run time got initialized during image building: io.netty.handler.codec.http2.Http2CodecUtil. Try marking this class for build-time initialization with --initialize-at-build-time=io.netty.handler.codec.http2.Http2CodecUtil
Error: Use -H:+ReportExceptionStackTraces to print stacktrace of underlying exception
Error: Image build request failed with exit status 1
Modification:
After debugging, it seems the culprit is io.netty.handler.codec.http2.Http2ClientUpgradeCodec, which also needs runtime initialisation.
Result:
Fixes #micronaut-projects/micronaut-grpc#8
Motivation:
Right now you can cancel the Future returned by
`Http2StreamChannelBootstrap.open()` and that will race with the
registration of the stream channel with the event loop, potentially
culminating in an `IllegalStateException` and potential resource leak.
Modification:
Ensure that the returned promise is uncancellable.
Result:
Should no longer see `IllegalStateException`s.
Motivation:
In the release (4.1.37) we introduced Http2MultiplexHandler as a
replacement of Http2MultiplexCodec. This did split the frame parsing from
the multiplexing to allow a more flexible way to handle frames and to make
the code cleaner. Unfortunally we did miss to special handle this in
Http2ClientUpgradeCodec and so did not correctly add Http2MultiplexHandler
to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...).
This did lead to the situation that we did not correctly receive the event
on the Http2MultiplexHandler and so did not correctly created the
Http2StreamChannel for the upgrade stream. Because of this we ended up
with an NPE if a frame was dispatched to the upgrade stream later on.
Modifications:
- Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...)
Result:
Fixes#9495.
Motivation:
Some of the links in javadoc point to the obsolete drafts of HTTP/2
specifications. We should point them to the latest RFC 7540 or 7541.
Modifications:
Update links from `draft-ietf-httpbis-*` to the `rfc7540` and `rfc7541`.
Result:
Correct links in javadoc.
Motivation:
We should better update the flow-controller on Channel.read() to reduce overhead and memory overhead.
See https://github.com/netty/netty/pull/9390#issuecomment-513008269
Modifications:
Move updateLocalWindowIfNeeded() to doBeginRead()
Result:
Reduce memory overhead
Motivation:
As we decorate the Http2FrameListener under the covers we should ensure the user can still access the original Http2FrameListener.
Modifications:
- Unwrap the Http2FrameListener in frameListener()
- Add unit test
Result:
Less suprises for users.
Motivation:
We recently introduced Http2ControlFrameLimitEncoderTest which did not correctly notify the goAway promises and so leaked buffers.
Modifications:
Correctly notify all promises and so release the debug data.
Result:
Fixes leak in HTTP2 test
Motivation:
It is possible for a remote peer to flood the server / client with empty DATA frames (without end_of_stream flag) set and so cause high CPU usage without the possibility to ever hit a limit. We need to guard against this.
See CVE-2019-9518
Modifications:
- Add a new config option to AbstractHttp2ConnectionBuilder and sub-classes which allows to set the max number of consecutive empty DATA frames (without end_of_stream flag). After this limit is hit we will close the connection. A limit of 10 is used by default.
- Add unit tests
Result:
Guards against CVE-2019-9518
Motivation:
Due how http2 spec is defined it is possible by a remote peer to flood us with frames that will trigger control frames as response, the problem here is that the remote peer can also just stop reading these (while still produce more of these) and so may drive us to the pointer where we either run out of memory or burn all CPU. To protect against this we need to implement some kind of limit that will tear down connections that cause the above mentioned situation.
See CVE-2019-9512 / CVE-2019-9514 / CVE-2019-9515
Modifications:
- Add Http2ControlFrameLimitEncoder which limits the number of queued control frames that were caused because of the remote peer.
- Allow to insert ths Http2ControlFrameLimitEncoder by setting AbstractHttp2ConnectionBuilder.encoderEnforceMaxQueuedControlFrames(...) to a number higher then 0. The default is 10000 which provides some protection by default but will hopefully not cause too many false-positives.
- Add unit tests
Result:
Protect against DDOS due control frames. Fixes CVE-2019-9512 / CVE-2019-9514 / CVE-2019-9515 .
Motivation:
We should delay the firing of the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent by one EventLoop tick when using the Http2FrameCodec to ensure all handlers are added to the pipeline before the event is passed through it.
This is needed to workaround a race that could happen when the preface is send in handlerAdded(...) but a later handler wants to act on the event.
Modifications:
Offload firing of the event to the EventExecutor.
Result:
Fixes https://github.com/netty/netty/issues/9432.
Motivation:
306299323c introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call.
Beside this we also did not use the correct handler for the upgrade stream in some cases
Modifications:
- Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already.
- Consolidate logic of creating stream channels
- Adjust unit test to capture the bug
Result:
Fixes https://github.com/netty/netty/issues/9395
Motivation:
When using the HTTP/2 multiplex implementation we need to ensure we correctly drain the buffered inbound data even if the RecvByteBufallocator.Handle tells us to stop reading in between.
Modifications:
Correctly loop through the buffered inbound data until the user does stop to request from it.
Result:
Fixes https://github.com/netty/netty/issues/9387.
Co-authored-by: Bryce Anderson <banderson@twitter.com>
Motivation:
We can easily reuse the Http2FrameStreamEvent instances and so reduce GC pressure as there may be multiple events per streams over the life-time.
Modifications:
Reuse instances
Result:
Less allocations
Motivation:
At the moment we lookup the ChannelHandlerContext used in Http2StreamChannelBootstrap each time the open(...) method is invoked. This is not needed and we can just cache it for later usage.
Modifications:
Cache ChannelHandlerContext in volatile field.
Result:
Speed up open(...) method implementation when called multiple times
Motivation:
If a read triggers a AbstractHttp2StreamChannel to close we can
get an NPE in the read loop.
Modifications:
Make sure that the inboundBuffer isn't null before attempting to
continue the loop.
Result:
No NPE.
Fixes#9337
Motivation:
The Http2FrameCodec should be responsible to create the upgrade stream.
Modifications:
Move code to create stream to Http2FrameCodec
Result:
More correct responsibility
Motivation:
Mark Http2StreamChannelBootstrap.open0(...) as deprecated as the user should not use it. It was marked as public by mistake.
Modifications:
Add deprecation warning.
Result:
User will be aware the method should not be used directly.
Motivation:
There are situations where the user may want to be more flexible when to send the PING acks for various reasons or be able to attach a listener to the future that is used for the ping ack. To be able to do so we should allow to manage the acks manually.
Modifications:
- Add constructor to DefaultHttp2ConnectionDecoder that allows to disable the automatically sending of ping acks (default is to send automatically to not break users)
- Add methods to AbstractHttp2ConnectionHandlerBuilder (and sub-classes) to either enable ot disable auto acks for pings
- Make DefaultHttp2PingFrame constructor public that allows to write acks.
- Add unit test
Result:
More flexible way of handling acks.
Motivation:
Netty homepage(netty.io) serves both "http" and "https".
It's recommended to use https than http.
Modification:
I changed from "http://netty.io" to "https://netty.io"
Result:
No effects.
Motivation:
Http2ConnectionHandler (and sub-classes) allow to configure a graceful shutdown timeout but only apply it if there is at least one active stream. We should always apply the timeout. This is also true when we try to send a GO_AWAY and close the connection because of an connection error.
Modifications:
- Always apply the timeout if one is configured
- Add unit test
Result:
Always respect gracefulShutdownTimeoutMillis
Motivation:
b3dba317d7 introduced the concept of Http2SettingsReceivedConsumer but did not correctly inplement DecoratingHttp2ConnectionEncoder.consumeRemoteSettings(...).
Modifications:
- Add missing `else` around the throws
- Add unit tests
Result:
Correctly implement DecoratingHttp2ConnectionEncoder.consumeRemoteSettings(...)
Motivation
The nice change made by @carl-mastrangelo in #9307 for lookup-table
based HPACK Huffman decoding can be simplified a little to remove the
separate flags field and eliminate some intermediate operations.
Modification
Simplify HpackHuffmanDecoder::decode logic including de-dup of the
per-nibble part.
Result
Less code, possibly better performance though not noticeable in a quick
benchmark.
Motivation:
We don't need the extra ChannelPromise when writing headers anymore in Http2FrameCodec. This also means we cal re-use a ChannelFutureListener and so not need to create new instances all the time.
Modifications:
- Just pass the original ChannelPromise when writing headers
- Reuse the ChannelFutureListener
Result:
Two less objects created when writing headers for an not-yet created stream.
Motivation:
ff0045e3e1 changed HpackHuffmanDecoder to use a lookup-table which greatly improved performance. We can squeeze out another 3% win by using an ByteProcessor which will reduce the number of bound-checks / reference-count-checks needed by processing byte-by-byte.
Modifications:
Implement logic with ByteProcessor
Result:
Another ~3% perf improvement which shows up when using h2load to simulate load.
`h2load -c 100 -m 100 --duration 60 --warm-up-time 10 http://127.0.0.1:8080`
Before:
```
finished in 70.02s, 620051.67 req/s, 20.70MB/s
requests: 37203100 total, 37203100 started, 37203100 done, 37203100 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 37203100 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 1.21GB (1302108500) total, 41.84MB (43872600) headers (space savings 90.00%), 460.24MB (482598600) data
min max mean sd +/- sd
time for request: 404us 24.52ms 15.93ms 1.45ms 87.90%
time for connect: 0us 0us 0us 0us 0.00%
time to 1st byte: 0us 0us 0us 0us 0.00%
req/s : 6186.64 6211.60 6199.00 5.18 65.00%
```
With this change:
```
finished in 70.02s, 642103.33 req/s, 21.43MB/s
requests: 38526200 total, 38526200 started, 38526200 done, 38526200 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 38526200 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 1.26GB (1348417000) total, 42.39MB (44444900) headers (space savings 90.00%), 466.25MB (488893900) data
min max mean sd +/- sd
time for request: 370us 24.89ms 15.52ms 1.35ms 88.02%
time for connect: 0us 0us 0us 0us 0.00%
time to 1st byte: 0us 0us 0us 0us 0.00%
req/s : 6407.06 6435.19 6419.74 5.62 67.00%
```
Motivation:
In the latest release we introduced Http2MultiplexHandler as a replacement of Http2MultiplexCodec. This did split the frame parsing from the multiplexing to allow a more flexible way to handle frames and to make the code cleaner. Unfortunally we did miss to special handle this in Http2ServerUpgradeCodec and so did not correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpServerUpgrade(...). This did lead to the situation that we did not correctly receive the event on the Http2MultiplexHandler and so did not correctly created the Http2StreamChannel for the upgrade stream. Because of this we ended up with an NPE if a frame was dispatched to the upgrade stream later on.
Modifications:
- Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpServerUpgrade(...)
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/9314.
Motivation:
b3dba317d7 added AbstractHttp2ConnectionBuilder.autoAckSettingsFrame(...) as protected method and made it public for Http2MultiplexCodecBuilder. Unfortunally it did miss to also make it public in Http2FrameCodecBuilder
Modifications:
Correctly override autoAckSettingsFrame in Http2FrameCodecBuilder and so make it usable when building Http2FrameCodec.
Result:
Be able to also configure autoAckSettingsFrame when Http2FrameCodec is used.
Motivation:
There is some manual coping of elements of Collections which can be replaced by Collections.addAll(...) and also some unnecessary semicolons.
Modifications:
- Simplify branches
- Use Collections.addAll
- Code cleanup
Result:
Code cleanup
Motivation:
We should not propage Http2WindowUpdateFrames to the child channels at all as these are not really use-ful and should not be flow-controlled via `read()` anyway. In the other hand Http2ResetFrame is very useful but should be propagated via an user event so the user is aware of it directly even if the user stops reading.
Modifications:
- Dont propagate Http2WindowUpdateFrames when using Http2MultiplexHandler
- Use user event for Http2ResetFrame when using Http2MultiplexHandler
- Adjust javadoc of Http2MultiplexHandler
- Add unit tests
Result:
Fixes https://github.com/netty/netty/pull/8889 and https://github.com/netty/netty/pull/7635
Motivation:
Http2MultiplexCodec and Http2MultiplexHandler had a very strong coupling with Http2FrameCodec which we can reduce easily. The end-goal should be to have no coupling at all.
Modifications:
- Reduce coupling by move some common logic to Http2CodecUtil
- Move logic to check if a stream may have existed before to Http2FrameCodec
- Use ArrayDeque as replacement for custom double-linked-list which makes the code a lot more readable
- Use WindowUpdateFrame to signal consume bytes (just as users do when they use Http2FrameCodec directly)
Result:
Less coupling and cleaner code.
Motivation:
In the past we had the following class hierarchy:
Http2ConnectionHandler --- Http2FrameCodec -- Http2MultiplexCodec
This hierarchy makes it impossible to plug in any code that would like to act on Http2Frame and Http2StreamFrame which can be quite useful for various situations (like metrics, logging etc). Beside this it also made the implementtion very hacky. To allow easier maintainance and also allow more flexible costumizations we should split Http2MultiplexCodec and Http2FrameCode.
Modifications:
- Introduce Http2MultiplexHandler (which is a replacement for Http2MultiplexCodec when used together with Http2FrameCodec)
- Mark Http2MultiplexCodecBuilder and Http2MultiplexCodec as deprecated. People should use Http2FrameCodecBuilder / Http2FrameCodec together with Http2MultiplexHandlder in the future
- Adjust / Add tests
- Adjust examples
Result:
More flexible usage possible and less hacky / coupled implementation for http2 multiplexing
Motivation:
For HTTP/2 messages with multiple cookies HttpConversionUtil.addHttp2ToHttpHeaders spends a good portion of time creating throwaway StringBuilders.
Modification:
Handle cookies lazily by using a ThreadLocal StringBuilder and then converting it to the H1 header at the end.
Result:
Less allocations.
Motivation:
f945a071db decoupled the writability state from the flow controller but could lead to the situation of a lot of writability updates events were propagated to the child channels. This change ensure we only take into account if the parent channel becomes writable again before we try to set the child channels to writable.
Modifications:
Only listen for channel writability changes for if the parent channel becomes writable again.
Result:
Less writability updates.
Motivation:
We should decouple the writability state of the http2 child channels from the flow-controller and just tie it to its own pending bytes counter that is decremented by the parent Channel once the bytes were written.
Modifications:
- Decouple writability state of child channels from flow-contoller
- Update tests
Result:
Less coupling and more correct behavior. Fixes https://github.com/netty/netty/issues/8148.
Motivation:
b4e3c12b8e introduced code to avoid coupling
close() to graceful close. It also added some code which attempted to infer when
a graceful close was being done in writing of a GOAWAY to preserve the
"connection is closed when all streams are closed behavior" for the child
channel API. However the implementation was too overzealous and may preemptively
close the connection if there are not currently any open streams (and close if
there are any frames which create streams in flight).
Modifications:
- Decouple writing a GOAWAY from trying to infer if a graceful close is being
done and closing the connection. Even if we could enhance this logic (e.g.
wait to close until the second GOAWAY with no error) it is possible the user
doesn't want the connection to be closed yet. We can add a means for the codec
to orchestrate the graceful close in the future (e.g. write some special "close
the connection when all streams are closed") but for now we can just let the
application handle this.
Result:
Fixes https://github.com/netty/netty/issues/9207
Motivation:
The first final version of GraalVM was released which deprecated some flags. We should use the new ones.
Modifications:
Removes the use of deprecated GraalVM native-image flags
Adds a flag to initialize netty at build time.
Result:
Do not use deprecated flags
Motivation:
OOME is occurred by increasing suppressedExceptions because other libraries call Throwable#addSuppressed. As we have no control over what other libraries do we need to ensure this can not lead to OOME.
Modifications:
Only use static instances of the Exceptions if we can either dissable addSuppressed or we run on java6.
Result:
Not possible to OOME because of addSuppressed. Fixes https://github.com/netty/netty/issues/9151.
Motivation:
Http2MultiplexCodec.DefaultHttp2StreamChannel currently only act on ClosedChannelException exceptions when checking for isAutoClose(). We should widen the scope here to IOException to be more consistent with AbstractChannel.
Modifications:
Replace instanceof ClosedChannelException with instanceof IOException
Result:
More consistent handling of isAutoClose()
Motivation:
GraalVM native images are a new way to deliver java applications. Netty is one of the most popular libraries however there are a few limitations that make it impossible to use with native images out of the box. Adding a few metadata (in specific modules will allow the compilation to success and produce working binaries)
Modification:
Added properties files in `META-INF` and substitutions classes (under `internal.svm`) will solve the compilation issues. The substitutions classes are not visible and do not have a public constructor so they are not visible to end users.
Result:
Fixes#8959
This fix is very conservative as it applies the minimum config required to build:
* pure netty servers
* vert.x applications
* grpc applications
The build is having trouble due to checkstyle which does not seem to be able to find the copyright notice on property files.
Motivation:
Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close
logic. This coupling means that a user would have to override
Http2ConnectionHandler#close(..) to modify the behavior, and the
Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot
override at this layer. Ideally we can totally decouple the close(..) of the
transport and the GOAWAY graceful closure process completely, but to preserve
backwards compatibility we can add an opt-out option to decouple where the
application is responsible for sending a GOAWAY with error code equal to
NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in
order to initiate graceful close.
Modifications:
- Http2ConnectionHandler supports an additional boolean constructor argument to
opt out of close(..) going through the graceful close path.
- Http2FrameCodecBuilder and Http2MultiplexCodec expose
gracefulShutdownTimeoutMillis but do not hook them up properly. Since these
are already exposed we should hook them up and make sure the timeout is applied
properly.
- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should
initiate the graceful closure process after writing a GOAWAY frame if the error
code is NO_ERROR. This means that writing a Http2GoAwayFrame from
Http2FrameCodec will initiate graceful close.
Result:
Http2ConnectionHandler#close(..) can now be decoupled from the graceful close
process, and immediately close the underlying transport if desired.
Motivation:
Http2FrameCodec currently fails the write promise associated with creating a
stream with a Http2NoMoreStreamIdsException. However this means the user code
will have to listen to all write futures in order to catch this scenario which
is the same as receiving a GOAWAY frame. We can also simulate receiving a GOAWAY
frame from our remote peer and that allows users to consolidate graceful close
logic in the GOAWAY processing.
Modifications:
- Http2FrameCodec should simulate a DefaultHttp2GoAwayFrame when trying to
create a stream but the stream IDs have been exhausted.
Result:
Applications can rely upon GOAWAY for graceful close processing instead of also
processing write futures.
Motivaiton:
DefaultHttp2ConnectionEncoder uses SimpleChannelPromiseAggregator to combine two
operations into a single future status. However it directly uses the
SimpleChannelPromiseAggregator object instead of using the newPromise() method
in one case. This may result in premature completion of the aggregated future.
Modifications:
- DefaultHttp2ConnectionEncoder to use
SimpleChannelPromiseAggregator#newPromise() instead of directly using the
SimpleChannelPromiseAggregator instance when writing the settings ACK frame
Result:
More correct status for the SETTING ACK frame writing when auto settings ACK is
disabled.
Motivation:
The HTTP/2 codec will synchronously respond to a SETTINGS frame with a SETTINGS
ACK before the application sees the SETTINGS frame. The application may need to
adjust its state depending upon what is in the SETTINGS frame before applying
the remote settings and responding with an ACK (e.g. to adjust for max
concurrent streams). In order to accomplish this the HTTP/2 codec should allow
for the application to opt-in to sending the SETTINGS ACK.
Modifications:
- DefaultHttp2ConnectionDecoder should support a mode where SETTINGS frames can
be queued instead of immediately applying and ACKing.
- DefaultHttp2ConnectionEncoder should attempt to poll from the queue (if it
exists) to apply the earliest received but not yet ACKed SETTINGS frame.
- AbstractHttp2ConnectionHandlerBuilder (and sub classes) should support a new
option to enable the application to opt-in to managing SETTINGS ACK.
Result:
HTTP/2 allows for asynchronous SETTINGS ACK managed by the application.
Motivation:
com.puppycrawl.tools checkstyle < 8.18 was reported to contain a possible security flaw. We should upgrade.
Modifications:
- Upgrade netty-build and checkstyle.
- Fix checkstyle errors
Result:
Fixes https://github.com/netty/netty/issues/8968.
Motivation:
PromiseCombiner is not thread-safe and even assumes all added Futures are using the same EventExecutor. This is kind of fragile as we do not enforce this. We need to enforce this contract to ensure it's safe to use and easy to spot concurrency problems.
Modifications:
- Add new contructor to PromiseCombiner that takes an EventExecutor and deprecate the old non-arg constructor.
- Check if methods are called from within the EventExecutor thread and if not fail
- Correctly dispatch on the right EventExecutor if the Future uses a different EventExecutor to eliminate concurrency issues.
Result:
More safe use of PromiseCombiner + enforce correct usage / contract.
Motivation:
When more than one connection header is present in h2c upgrade request, upgrade fails. This is to fix that.
Modification:
In HttpServerUpgradeHandler's upgrade() method, check whether any of the connection header value is upgrade, not just the first header value which might return a different value other than upgrade.
Result:
Fixes#8846.
With this PR, now when multiple connection headers are sent with the upgrade request, upgrade will not fail.
Motivation:
We can replace some "hand-rolled" integer checks with our own static utility method to simplify the code.
Modifications:
Use methods provided by `ObjectUtil`.
Result:
Cleaner code and less duplication
Motivation:
We need to update to a new checkstyle plugin to allow the usage of lambdas.
Modifications:
- Update to new plugin version.
- Fix checkstyle problems.
Result:
Be able to use checkstyle plugin which supports new Java syntax.
Motivation:
When a write error happens during writing of flowcontrolled data frames we miss to correctly detect this in the write loop which may result in an infinite loop as we will never detect that the frame should be removed from the queue.
Modifications:
- When we fail a flowcontrolled data frame we ensure that the next frame.write(...) call will signal back that the whole frame was handled and so can be removed.
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8707.
Motivation:
In Http2FrameCodec we made the incorrect assumption that we can only have 1 buffered outboundstream as maximum. This is not correct and we need to account for multiple buffered streams.
Modifications:
- Use a map to allow buffer multiple streams
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8692.
Motiviation:
Http2FrameCodecTest and Http2MultiplexCodecTest were quite fragile and often not went through the whole pipeline which made testing sometimes hard and error-prone.
Modification:
- Refactor tests to have data flow through the whole pipeline and so made the test more robust (by testing the while implementation).
Result:
Easier to write tests for the codecs in the future and more robust testing in general.
Beside this it also fixes https://github.com/netty/netty/issues/6036.
Motivation:
We should always call ctx.read() even when AUTO_READ is false as flow-control is enforced by the HTTP/2 protocol.
See also https://tools.ietf.org/html/rfc7540#section-5.2.2.
We already did this before but not explicit and only did so because of some implementation details of ByteToMessageDecoder. It's better to be explicit here to not risk of breakage later on.
Modifications:
- Ensure we always call ctx.read() when AUTO_READ is false
- Add unit test.
Result:
No risk of staling the connection when HTTP/2 is used.
Motivation:
In windows if the project is in a path that contains whitespace,
resources cannot be accessed and tests fail.
Modifications:
Adds ResourcesUtil.java in netty-common. Tests use ResourcesUtil.java to access a resource.
Result:
Being able to build netty in a path containing whitespace
Motivation:
9f9aa1a did some changes related to fixing how we handle ctx.read() in child channel but did incorrectly change some assert.
Modifications:
Fix assert to be correct.
Result:
Code does not throw an AssertionError due incorrect assert check.
Motivation:
Most of the maven modules do not explicitly declare their
dependencies and rely on transitivity, which is not always correct.
Modifications:
For all maven modules, add all of their dependencies to pom.xml
Result:
All of the (essentially non-transitive) depepdencies of the modules are explicitly declared in pom.xml
Motivation:
We did not correct respect ctx.read() calls while processing a read for a child Channel. This could lead to read stales when auto read is disabled and no other read was requested.
Modifications:
- Keep track of extra read() calls while processing reads
- Add unit tests that verify that read() is respected when triggered either in channelRead(...) or channelReadComplete(...)
Result:
Fixes https://github.com/netty/netty/issues/8209.
Motivation
DefaultHttp2FrameReader currently does a fair amount of "intermediate"
slicing which can be avoided.
Modifications
Avoid slicing the input buffer in DefaultHttp2FrameReader until
necessary. In one instance this also means retainedSlice can be used
instead (which may also avoid allocating).
Results
Less allocations when using http2.
Motivation:
When the DefaultHttp2ConnectionEncoder writes the initial headers for a new
locally created stream we create the stream in the half-closed state if the
end-stream flag is set which signals to the life cycle manager that the headers
have been sent. However, if we synchronously fail to write the headers the
life cycle manager then sends a RST_STREAM on our behalf which is a connection
level PROTOCOL_ERROR because the peer sees the stream in an IDLE state.
Modification:
Don't open the stream in the half-closed state if the end-stream flag is
set and let the life cycle manager take care of it.
Result:
Cleaner state management in the DefaultHttp2ConnectionEncoder.
Fixes#8434.
Motivation:
The `Http2StreamFrameToHttpObjectCodec` is marked `@Sharable` but mutates
an internal `HttpScheme` field every time it is added to a pipeline.
Modifications:
Instead of storing the `HttpScheme` in the handler we store it as an
attribute on the parent channel.
Result:
Fixes#8480.
Motivation:
There are log messages emitted from Http2ConnectionDecoder of the form
```
INF i.n.h.c.h.DefaultHttp2ConnectionDecoder ignoring HEADERS frame for stream RST_STREAM sent. {}
```
Modifications:
Remove the trailing `{}` in the log message that doesn't have a value.
Result:
Log messages no longer have a trailing `{}`.
Motivation:
When writing an HTTP/2 HEADERS with END_STREAM=1, the application expects
the stream to be closed afterward. However, the write can fail locally
due to HPACK encoder and similar. When that happens we need to make sure
to issue a RST_STREAM otherwise the stream can be closed locally but
orphaned remotely. The RST_STREAM is typically handled by
Http2ConnectionHandler.onStreamError, which will only send a RST_STREAM
if that stream still exists locally.
There are two possible flows for trailers, one handled immediately and
one going through the flow controller. Previously they behaved
differently, with the immedate code calling the error handler after
closing the stream. The immediate code also used a listener for calling
closeStreamLocal while the flow controlled code did so immediately after
the write.
The two code paths also differed in their VoidChannelPromise handling,
but both were broken. The immediate code path called unvoid() only if
END_STREAM=1, however it could always potentially add a listener via
notifyLifecycleManagerOnError(). And the flow controlled code path
unvoided incorrectly, changing the promise completion behavior. It also
passed the wrong promise to closeStreamLocal() in FlowControlledBase.
Modifications:
Move closeStreamLocal handling after calls to onError. This is the
primary change.
Now call closeStreamLocal immediately instead of when the future
completes. This is the more likely correct behavior as it matches that
of DATA frames.
Fix all the VoidChannelPromise handling.
Result:
Http2ConnectionHandler.onStreamError sees the same state as the remote
and issues a RST_STREAM, properly cleaning up the stream.
