Motivation:
Even if it was stored in the string constant pool, I thought it was safe to compare it through the Equals() method.
Modification:
So, I changed "==" comparison to equals() comparison
Result:
It has become safer to compare String values with different references and with the same values.
Motivation:
We need to carefully manage flushes to ensure we not discard these by mistake due wrongly implemented consolidation of flushes.
Modifications:
- Ensure we reset flag before we actually call flush0(...)
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/10015
Motivation:
Users of the DefaultHttp2ConnectionDecodcer are notified of inbound GoAwayFrames
after the connection has already closed any ignored streams, potentially
losing the signal that some streams may have been ignored by the peer and
are thus retryable.
Modifications:
Reorder the notifications of the frame and connection listeners to
propagate the frame first, giving the frame listeners the opportunity to
clean up ignored streams in their own way.
Result:
Fixes#9986
Motivation:
ClearTextHttp2ServerUpgradeHandler is currently more complex then needed. We can simplify it by directly implement the prior-knowledge logic as part of the handler.
Modifications:
Merge inner PriorKnowledgeHandler logic into ClearTextHttp2ServerUpgradeHandler by extending ByteToMessageDecoder directly
Result:
Cleaner code and less pipeline operations
Motivation:
https://github.com/netty/netty/pull/9848 changed how we handled ChannelOptions internally to use a ConcurrentHashMap. This unfortunally had the side-effect that the ordering may be affected and not stable anymore. Here the problem is that sometimes we do validation based on two different ChannelOptions (for example we validate high and low watermarks against each other). Thus even if the user specified the options in the same order we may fail to configure them.
Modifications:
- Use again a LinkedHashMap to preserve order
Result:
Apply ChannelOptions in correct and expected order
Motivation:
In next major version of netty users should use ChannelHandler everywhere. We should ensure we do the same
Modifications:
Replace usage of deprecated classes / interfaces with ChannelHandler
Result:
Use non-deprecated code
Motivation:
ByteToMessageDecoder requires using an intermediate List to put results into. This intermediate list adds overhead (memory/CPU) which grows as the number of objects increases. This overhead can be avoided by directly propagating events through the ChannelPipeline via ctx.fireChannelRead(...). This also makes the semantics more clear and allows us to keep track if we need to call ctx.read() in all cases.
Modifications:
- Remove List from the method signature of ByteToMessageDecoder.decode(...) and decodeLast(...)
- Adjust all sub-classes
- Adjust unit tests
- Fix javadocs.
Result:
Adjust ByteToMessageDecoder as noted in https://github.com/netty/netty/issues/8525.
# Motivation:
`DefaultByteBufHolder.equals()` considers another object equal if it's an instance of `ByteBufferHolder` and if the contents of two objects are equal. However, the behavior of `equals` method is not a part of the `ByteBufHolder` contract so `DefaultByteBufHolder`'s version may be causing violation of the symmetric property if other classes have different logic.
There are already a few classes that are affected by this: `DefaultHttp2GoAwayFrame`, `DefaultHttp2UnknownFrame`, and `SctpMessage` are all overriding `equals` method breaking the symmetric property.
Another effect of this behavior is that all instances with empty data are considered equal. That may not be desireable in the situations when instances are created for predefined constants, e.g. `FullBulkStringRedisMessage.NULL_INSTANCE` and `FullBulkStringRedisMessage.EMPTY_INSTANCE` in `codec-redis`.
# Modification:
Make `DefaultByteBufHolder.equals()` implementation only work for the objects of the same class.
# Result:
- The symmetric property of the `equals` method is restored for the classes in question.
- Instances of different classes are not considered equal even if the content of the data they hold are the same.
Motivation:
We should use Objects.requireNonNull(...) as we require java8
Modifications:
Replace ObjectUtil.checkNonNull(...) with Objects.requireNonNull(...)
Result:
Code cleanup
Motivation:
The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values even if the user didnt want too.
Modifications:
- Change DefaultHttp2ConnectionEncoder to call the correct Http2FrameWriter method depending on if the user wants to use priorities or not
- Adjust tests
Result:
Fixes https://github.com/netty/netty/issues/9842
Motivation:
97361fa2c8 replace synchronized with ConcurrentHashMap in *Bootstrap classes but missed to do the same for the Http2 variant.
Modifications:
- Use ConcurrentHashMap
- Simplify code in *Bootstrap classes
Result:
Less contention
Motivation:
We use the onStreamClosed(...) callback to return unconsumed bytes back to the window of the connection when needed. When this happens we will write a window update frame but not automatically call ctx.flush(). As the user has no insight into this it could in the worst case result in a "deadlock" as the frame is never written out ot the socket.
Modifications:
- If onStreamClosed(...) produces a window update frame call ctx.flush()
- Add unit test
Result:
No stales possible due unflushed window update frames produced by onStreamClosed(...) when not all bytes were consumed before the stream was closed
Motivation:
We can make use of EmbeddedChannel.finishAndReleaseAll() and so remove some code
Modifications:
Use finishAndReleaseAll()
Result:
Less code to maintain
Motivation:
At the moment we send a window update frame for the connection + stream when a stream is closed and there are unconsumed bytes left. While we need to do this for the connection it makes no sense to write a window update frame for the stream itself as it is already closed
Modifications:
- Don't write the window update frame for the stream when the stream is closed
- Add unit test
Result:
Don't write the window frame for closed streams
Motivation:
Http2ConnectionHandler tries to guard against sending multiple RST frames for the same stream. Unfortunally the code is not 100 % correct as it only updates the state after it calls write. This may lead to the situation of have an extra RST frame slip through if the second write for the RST frame is done from a listener that is attached to the promise.
Modifications:
- Update state before calling write
- Add unit test
Result:
Only ever send one RST frame per stream
Motivation:
There is an intrinsic race between a local session resetting a stream
and the peer no longer sending any frames. This can result in the
session receiving frames for a stream that the local peer no longer
tracks. This results in a StreamException being thrown which triggers a
RST_STREAM frame, which is a good thing, but also logging at level WARN,
which is noisy for an expected and benign condition.
Modification:
Change the log level to DEBUG when logging stream errors with code
STREAM_CLOSED. All others are more interesting and will continue to be
logged at level WARN.
Additionally, it was found that DATA frames for streams that could not
have existed only resulted in a StreamException when the spec is clear
that such a situation should be fatal to the connection, resulting in a
GOAWAY(PROTOCOL_ERROR).
Fixes#8025.
Motivation:
To avoid regression regarding connection-specific headers[1], we should add a test.
[1] https://tools.ietf.org/html/rfc7540#section-8.1.2.2
Modification:
Add test that checks the following headers are removed.
- Connection
- Host
- Keep-Alive
- Proxy-Connection
- Transfer-Encoding
- Upgrade
Result:
There's no functional change.
Motivation:
Padding was removed from CONTINUATION frame in http2-spec, as showed in [PR](https://github.com/http2/http2-spec/pull/510). We should follow it.
Modifications:
- Remove padding when writing CONTINUATION frame in DefaultHttp2FrameWriter
- Add a unit test for writing large header with padding
Result:
More spec-compliant
Motivation
Per javadoc in 4.1.x SimpleChannelInboundHandler:
"Please keep in mind that channelRead0(ChannelHandlerContext, I) will be
renamed to messageReceived(ChannelHandlerContext, I) in 5.0."
Modifications
Rename aforementioned method and all references/overrides.
Result
Method is renamed.
Motivation:
The javadocs of Http2Headers.method(...) are incorrect, we should fix these.
Modifications:
Correct javadocs
Result:
Fixes https://github.com/netty/netty/issues/8068.
Motivation:
Netty HTTP/2 implementation is not 100% compliant to the spec. This
commit improves the compliance regarding headers validation,
in particular pseudo-headers and connection ones.
According to the spec:
All HTTP/2 requests MUST include exactly one valid value for the
":method", ":scheme", and ":path" pseudo-header fields, unless it is
a CONNECT request (Section 8.3). An HTTP request that omits
mandatory pseudo-header fields is malformed (Section 8.1.2.6).
Modifications:
- Introduce Http2HeadersValidator class capable of validating HTTP/2
headers
- Invoke validation from DefaultHttp2ConnectionDecoder#onHeadersRead
- Modify tests to use valid headers when required
- Modify HttpConversionUtil#toHttp2Headers to not add :scheme and
:path header on CONNECT method in order to conform to the spec
Result:
- Initial requests without :method, :path, :scheme will fail
- Initial requests with multiple values for :method, :path, :scheme
will fail
- Initial requests with an empty :path fail
- Requests with connection-specific header field will fail
- Requests with TE header different than "trailers" will fail
-
- Fixes 8.1.2.2 tests from h2spec #5761
- Fixes 8.1.2.3 tests from h2spec #5761
Motivation:
Constant time comparison functions are used to compare HTTP/2 header
values, even if they are not sensitive.
Modification:
After checking for sensitivity, use fast comparison.
Result: Faster HPACK table reads/writes
Motivation:
We may fail to update the flow-controller and in this case need to notify the stream channel and close it.
Modifications:
Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it
Result:
Fixes https://github.com/netty/netty/issues/9663
Motivation:
bbc34d0eda introduced correct handling of "in process" setup of streams but there is some room for improvements. Often the writeHeaders(...) is completed directly which means there is not need to create the extra listener object.
Modifications:
- Only create the listener if we really need too.
Result:
Less GC
Motivation:
In https://github.com/netty/netty/issues/8692, `Http2FrameCodec` was
updated to keep track of all "being initialized" streams, allocating
memory before initialization begins, and releasing memory after
initialization completes successfully.
In some instances where stream initialization fails (e.g. because this
connection has received a GOAWAY frame), this memory is never released.
Modifications:
This change updates the `Http2FrameCodec` to use a separate promise
for monitoring the success of sending HTTP2 headers. When sending of
headers fails, we now make sure to release memory allocated for stream
initialization.
Result:
After this change, failures in writing HTTP2 Headers (e.g. because this
connection has received a GOAWAY frame) will no longer leak memory.
Motivation:
Http2MultiplexCodec extends Http2FrameCodec extends Http2ConnectionHandler. It appears Http2MultiplexCodec overrode the channelWritabilityChanged method, which prevented the flow controller from becoming active. In the case the parent channel becomes unwritable, and then later becomes writable, it needs to indicate that the child channels can still write data. This is slightly confusing, because the child channels may still themselves be unwritable, but should still drain their data to the parent channel.
Modification:
Still propagate writability changes to the HTTP/2 flow controller
Result:
Fixes https://github.com/netty/netty/issues/9636
Motivation:
Native image compilation is failing without extra flags:
```
Warning: Aborting stand-alone image build. No instances of io.netty.buffer.UnpooledHeapByteBuf are allowed in the image heap as this class should be initialized at image runtime. Object has been initialized by the io.netty.handler.codec.http2.Http2ConnectionHandler class initializer with a trace:
at io.netty.buffer.Unpooled.wrappedBuffer(Unpooled.java:157)
at io.netty.handler.codec.http2.Http2ConnectionHandler.<clinit>(Http2ConnectionHandler.java:74)
. To fix the issue mark io.netty.buffer.UnpooledHeapByteBuf for build-time initialization with --initialize-at-build-time=io.netty.buffer.UnpooledHeapByteBuf or use the the information from the trace to find the culprit and --initialize-at-run-time=<culprit> to prevent its instantiation.
Detailed message:
Trace: object io.netty.buffer.ReadOnlyByteBuf
object io.netty.buffer.UnreleasableByteBuf
method io.netty.handler.codec.http2.Http2ConnectionHandler.access$500()
Call path from entry point to io.netty.handler.codec.http2.Http2ConnectionHandler.access$500():
at io.netty.handler.codec.http2.Http2ConnectionHandler.access$500(Http2ConnectionHandler.java:66)
at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.readClientPrefaceString(Http2ConnectionHandler.java:299)
at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.decode(Http2ConnectionHandler.java:239)
at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:505)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:444)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:283)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.access$600(AbstractChannelHandlerContext.java:56)
at io.netty.channel.AbstractChannelHandlerContext$7.run(AbstractChannelHandlerContext.java:365)
at com.oracle.svm.core.jdk.RuntimeSupport.executeHooks(RuntimeSupport.java:144)
at com.oracle.svm.core.jdk.RuntimeSupport.executeStartupHooks(RuntimeSupport.java:89)
at com.oracle.svm.core.JavaMainWrapper.runCore(JavaMainWrapper.java:143)
at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:186)
at com.oracle.svm.core.code.IsolateEnterStub.JavaMainWrapper_run_5087f5482cc9a6abc971913ece43acb471d2631b(generated:0)
```
Modification:
Add `io.netty.handler.codec.http2.Http2ConnectionHandler` for runtime compilation, as the buffer library's `io.netty.buffer.UnpooledHeapByteBuf` is also marked for runtime.
Result:
Native image compilation works again.
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
Motiviation:
EmbeddedChannel currently is quite differently in terms of semantics to other Channel implementations. We should better change it to be more closely aligned and so have the testing code be more robust.
Modifications:
- Change EmbeddedEventLoop.inEventLoop() to only return true if we currenlty run pending / scheduled tasks
- Change EmbeddedEventLoop.execute(...) to automatically process pending tasks if not already doing so
- Adjust a few tests for the new semantics (which is closer to other Channel implementations)
Result:
EmbeddedChannel works more like other Channel implementations
Motivation:
At the moment it is quite easy to hit reentrance issues when you have multiple handlers in the pipeline and each of the handlers does not correctly protect against these. To make it easier for the user we should try to protect from these. The issue is usually if and inbound event will trigger and outbound event and this outbound event then against triggeres an inbound event. This may result in having methods in a ChannelHandler re-enter some method and so state can be corrupted or messages be re-ordered.
Modifications:
- Keep track of inbound / outbound operations in DefaultChannelHandlerContext and if reentrancy is detected break it by scheduling the action on the EventLoop. This will then be picked up once the method returns and so the reentrancy is broken up.
- Adjust tests which made strange assumptions about execution order
Result:
No more reentrancy of handlers possible.
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
