Motivation:
`Http2MultiplexHandler` have to 2 empty lines at the end instead of 1.
Modification:
Removed 1 extra line.
Result:
Little better code style.
Motivation:
We can use ObjectUtil#checkPositive instead of manually checking maxContentLength. Also, there was a missing JavaDoc for Http2Exception,
Modification:
Used ObjectUtil#checkPositive for checking maxContentLength.
Added missing JavaDoc.
Result:
More readable code.
Motivation:
We wish to use Unsafe as little as possible, and Java 8 allows us
to take some short-cuts or play some tricks with generics,
for the purpose of working around having to declare all checked
exceptions. Ideally all checked exceptions would be declared, but
the code base is not ready for that yet.
Modification:
The call to UNSAFE.throwException has been removed, so when we need
that feature, we instead use the generic exception trick.
In may cases, Java 8 allows us to throw Throwable directly. This
happens in cases where no exception is declared to be thrown in a
scope.
Finally, some warnings have also been fixed, and some imports have
been reorganised and cleaned up while I was modifying the files
anyway.
Result:
We no longer use Unsafe for throwing any exceptions.
Motivation:
be able to modify Http2Settings of Http2ConnectionHandlerBuilder.
Modifications:
make initialSettings() of Http2ConnectionHandlerBuilder public.
Result:
public initialSettings() of Http2ConnectionHandlerBuilder allow Http2Settings modification.
Motivation:
Fix a TODO that was due since the "master" branch is baselined on at least Java 8.
Modification:
Remove our own copy of the Consumer interface and fix usage sites to use j.u.Consumer.
Also some cleanup.
Result:
Cleaner code.
Motivation
ByteBuf has an isAccessible method which was introduced as part of ref
counting optimizations but there are some places still doing
accessibility checks by accessing the volatile refCnt() directly.
Modifications
- Have PooledNonRetained(Duplicate|Sliced)ByteBuf#isAccessible() use
their refcount delegate's isAccessible() method
- Add static isAccessible(buf) and ensureAccessible(buf) methods to
ByteBufUtil
(since ByteBuf#isAccessible() is package-private)
- Adjust DefaultByteBufHolder and similar classes to use these methods
rather than access refCnt() directly
Result
- More efficient accessibility checks in more places
Motivation:
There should be a validation for the input arguments when constructing Http2FrameLogger
Modification:
Check that the provided arguments are not null
Result:
Proper validation when constructing Http2FrameLogger
Motivation:
We should include TLSv1.3 ciphers as well as recommented ciphers these days for HTTP/2. That is especially true as Java supports TLSv1.3 these days out of the box
Modifications:
- Add TLSv1.3 ciphers that are recommended by mozilla as was for HTTP/2
- Add unit test
Result:
Include TLSv1.3 ciphers as well
Motivation:
testStreamIsNotCreatedIfParentConnectionIsClosedConcurrently() made some assumptions about sequencing which may not be true all the time and racy.
Modifications:
Fix the testcase so its not racy anymore
Result:
Fix build failure on master branch
Motivation:
Setting a dependency on the connection is normal and permitted; streams
actually default to depending on the connection. Using a PRIORITY frame
with a dependency on the connection could reset a previous PRIORITY,
change the relative weight, or make all streams dependent on one stream.
The previous code was disallowing these usages as it considered
depending on the connection to be a validation failure.
Modifications:
Loosen validation check to also allow depending on the connection. Fix
error message when the validation check fails.
Result:
Setting a dependency on connection would be permitted. Fixes#10416
Motivation:
If a request to open a new h2 stream was made from outside of the
EventLoop it will be scheduled for future execution on the EventLoop.
However, during the time before the `open0` task will be executed the
parent channel may already be closed. As the result,
`Http2MultiplexHandler#newOutboundStream()` will throw an
`IllegalStateException` with the message that is hard to
interpret correctly for this use-case: "Http2FrameCodec not found. Has
the handler been added to a pipeline?".
Modifications:
- Check that the parent h2 `Channel` is still active before creating a
new stream when `open0` task is picked up by EventLoop;
Result:
Users see a correct `ClosedChannelException` in case the parent h2
`Channel` was closed concurrently with a request for a new stream.
Motivation:
`Http2StreamChannelBootstrap#open0` invokes
`Http2MultiplexHandler#newOutboundStream()` which may throw an
`IllegalStateException`. In this case, it will never complete
the passed promise.
Modifications:
- `try-catch` all invocations of `newOutboundStream()` and fail
promise in case of any exception;
Result:
New H2 stream promise always completes.
Motivation:
`DefaultHttp2FrameStream.stream` is not used outside of its class and
therefore can be private.
Modifications:
- Make `DefaultHttp2FrameStream.stream` private;
Result:
Correct visibility scoping for `DefaultHttp2FrameStream.stream`.
Motivation:
From the recent benchmark using gRPC-Java based on Netty's HTTP2, it appears that it prefers `ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256` over `ECDHE_ECDSA_WITH_AES_128_GCM_SHA256` since it uses the Netty HTTPS Cipher list as is. Both are considered safe but `ECDHE_ECDSA_WITH_AES_128_GCM_SHA256` has a good chance to get more optimized implementation. (e.g. AES-NI) When running both on GCP Intel Haswell VM, `TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256` spent 3x CPU time than `TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256`. (Note that this VM supports AES-NI)
From the cipher suites listed on `Intermediate compatibility (recommended)` of [Security/Server Side TLS](https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility), they have a cipher preference which is aligned with this PR.
```
0x13,0x01 - TLS_AES_128_GCM_SHA256 TLSv1.3 Kx=any Au=any Enc=AESGCM(128) Mac=AEAD
0x13,0x02 - TLS_AES_256_GCM_SHA384 TLSv1.3 Kx=any Au=any Enc=AESGCM(256) Mac=AEAD
0x13,0x03 - TLS_CHACHA20_POLY1305_SHA256 TLSv1.3 Kx=any Au=any Enc=CHACHA20/POLY1305(256) Mac=AEAD
0xC0,0x2B - ECDHE-ECDSA-AES128-GCM-SHA256 TLSv1.2 Kx=ECDH Au=ECDSA Enc=AESGCM(128) Mac=AEAD
0xC0,0x2F - ECDHE-RSA-AES128-GCM-SHA256 TLSv1.2 Kx=ECDH Au=RSA Enc=AESGCM(128) Mac=AEAD
0xC0,0x2C - ECDHE-ECDSA-AES256-GCM-SHA384 TLSv1.2 Kx=ECDH Au=ECDSA Enc=AESGCM(256) Mac=AEAD
0xC0,0x30 - ECDHE-RSA-AES256-GCM-SHA384 TLSv1.2 Kx=ECDH Au=RSA Enc=AESGCM(256) Mac=AEAD
0xCC,0xA9 - ECDHE-ECDSA-CHACHA20-POLY1305 TLSv1.2 Kx=ECDH Au=ECDSA Enc=CHACHA20/POLY1305(256) Mac=AEAD
0xCC,0xA8 - ECDHE-RSA-CHACHA20-POLY1305 TLSv1.2 Kx=ECDH Au=RSA Enc=CHACHA20/POLY1305(256) Mac=AEAD
0x00,0x9E - DHE-RSA-AES128-GCM-SHA256 TLSv1.2 Kx=DH Au=RSA Enc=AESGCM(128) Mac=AEAD
0x00,0x9F - DHE-RSA-AES256-GCM-SHA384 TLSv1.2 Kx=DH Au=RSA Enc=AESGCM(256) Mac=AEAD
```
Modification:
Moving up `AES_128_GCM_SHA256` in the CIPHERS of HTTPS so that it gets priority.
Result:
When connecting to the server supporting both `ECDHE_ECDSA_WITH_AES_128_GCM_SHA256` and `ECDSA_WITH_CHACHA20_POLY1305_SHA256` and respecting the client priority of cipher suites, it will be able to save significant cpu time when running it on machines with AES-NI support.
Motivation:
The result of `header.size()` is already cached in `headerSize`. There is no need to call it again actually.
Modification:
Replace the second `header.size()` with `headerSize` directly.
Result:
Improve performance slightly.
Motivation:
`getEntry(...)` may throw an IndexOutOfBoundsException without any error messages.
Modification:
Add detailed error message corresponding to the IndexOutOfBoundsException while calling `getEntry(...)` in HpackDynamicTable.java.
Result:
Easier to debug
Motivation:
`HpackDynamicTable` needs some test cases to ensure bug-free.
Modification:
Add unit test for `HpackDynamicTable`.
Result:
Improve test coverage slightly.
Motivation:
After searching the whole netty project, I found that the private method `translateHeader(...)` with empty body is never used actually. So I think it could be safely removed.
Modification:
Just remove this unused method.
Result:
Clean up the code.
Motivation:
`io.netty.handler.codec.http2.Http2Stream.State` is never used in DefaultHttp2LocalFlowController.java, and `io.netty.handler.codec.http2.HpackUtil.equalsConstantTime` is never used in HpackStaticTable.java.
Modification:
Just remove these unused imports.
Result:
Make imports cleaner.
Motivation:
`io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT` is never used in DefaultHttp2ConnectionEncoder.java.
Modification:
Just remove this unused import.
Result:
Make the DefaultHttp2ConnectionEncoder.java's imports clean.
Motivation:
Http2ConnectionHandler may call ctx.close(...) with the same promise instance multiple times if the timeout for gracefulShutdown elapse and the listener itself is notified. This can cause problems as other handlers in the pipeline may queue these promises and try to notify these later via setSuccess() or setFailure(...) which will then throw an IllegalStateException if the promise was notified already
Modification:
- Add boolean flag to ensure doClose() will only try to call ctx.close(...) one time
Result:
Don't call ctx.close(...) with the same promise multiple times when gradefulShutdown timeout elapses.
Motivation:
Http2StreamChannelId is Serializable. A test case is needed to ensure that it works.
Modification:
Add a test case about serialization.
Result:
Improve test coverage slightly.
Motivation:
Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.
Modifications:
- Ensure we flush if we exit the `doBeginRead()` method.
- Account for the Http2FrameCodec always synchronously finishing writes
of window update frames.
Result:
Fixes#10072
Motivation:
At the moment the next / prev references are not set to "null" in the DefaultChannelHandlerContext once the ChannelHandler is removed. This is bad as it basically let users still use the ChannelHandlerContext of a ChannelHandler after it is removed and may produce very suprising behaviour.
Modifications:
- Fail if someone tries to use the ChannelHandlerContext once the ChannelHandler was removed (for outbound operations fail the promise, for inbound fire the error through the ChannelPipeline)
- Fix some handlers to ensure we not use the ChannelHandlerContext after the handler was removed
- Adjust DefaultChannelPipeline / DefaultChannelHandlerContext to fixes races with removal / replacement of handlers
Result:
Cleanup behaviour and make it more predictable for pipeline modifications
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.
