Motivation:
If we don't need the scheduled future, then it will be one less complication when we change Netty Future to no longer extend JDK Future.
It would also result in fewer elements in our API.
Modification:
There was only one real use of ScheduledFuture in our code, in Cache.
This has been changed to wrap an ordinary future with a deadline for implementing the Delayed interface.
All other places were effectively overspecifying by relying on ScheduledFuture.
A few places were also specifying JDK Futures - these have been changed to specify Netty Futures.
Result:
Reduced dependency on the ScheduledFuture interfaces.
Motivation:
There is a memory leak while writing empty data frame with padding.
The empty data frame was occurred because we are running a proxy server
built by netty, and found that google services always sent data frames
followed by an empty data frame.
Modifications:
Calls the ctx.write even the payload is empty.
Result:
Fix memory leak.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
We did recently change the Channel / ChannelHandler API to always act on the Future only. We should do the same for our handlers.
Modifications:
- Adjust http2 API
- Adjust other handlers API
Result:
Easier to use API and more consistent
Motivation:
The need of cascade from a Future to a Promise exists. We should add some default implementation for it.
Modifications:
- Merge PromiseNotifier into Futures
- Add default cascadeTo(...) methods to Future
- Add tests to FuturesTest
- Replace usage of PromiseNotifier with Future.cascadeTo
- Use combination of map(...) and cascadeTo(...) in *Bootstrap to reduce code duplication
Result:
Provide default implementation of cascadeTo.
Motivation:
The expression "not is success" can mean that either the future failed, or it has not yet completed.
However, many places where such an expression is used is expecting the future to have completed.
Specifically, they are expecting to be able to call `cause()` on the future.
It is both more correct, and semantically clearer, to call `isFailed()` instead of `!isSuccess()`.
Modification:
Change all places that used `!isSuccess()` to mean that the future had failed, to use `isFailed()`.
A few places are relying on `isSuccess()` returning `false` for _incomplete_ futures, and these places have been left unchanged.
Result:
Clearer code, with potentially fewer latent bugs.
Motivation:
We should just add `executor()` to the `ChannelOutboundInvoker` interface and override this method in `Channel` to return `EventLoop`.
Modifications:
- Add `executor()` method to `ChannelOutboundInvoker`
- Let `Channel` override this method and return `EventLoop`.
- Adjust all usages of `eventLoop()`
- Add some default implementations
Result:
API cleanup
Motivation:
At the moment the outbound operations of ChannelHandler take a Promise as argument. This Promise needs to be carried forward to the next handler in the pipeline until it hits the transport. This is API choice has a few quirks which we should aim to remove:
- There is a difference between if you add a FutureListener to the Promise or the Future that is returned by the outbound method in terms of the ordering of execution of the listeners. Sometimes we add the listener to the promise while in reality we usually always want to add it to the future to ensure the listerns are executed in the "correct order".
- It is quite easy to "loose" a promise by forgetting to use the right method which also takes a promise
- We have no idea what EventExecutor is used for the passed in Promise which may invalid our assumption of threading.
While changing the method signature of the outbound operations of the ChannelHandler is a good step forward we should also take care of just remove all the methods from ChannelOutboundInvoker (and its sub-types) that take a Promise and just always use the methods that return a Future only.
Modifications:
- Change the signature of the methods that took a Promise to not take one anymore and just return a Future
- Remove all operations for ChannelOutboundInvoker that take a Promise.
- Adjust all code to cope with the API changes
Result:
Cleaner API which is easier to reason about and easier to use.
Motivation:
Since most futures in Netty are of the `Void` type, methods like `getNow()` and `cause()` cannot distinguish if the future has finished or not.
This can cause data race bugs which, in the case of `Void` futures, can be silent.
Modification:
The methods `getNow()` and `cause()` now throw an `IllegalStateException` if the future has not yet completed.
Most use of these methods are inside listeners, and so are not impacted.
One place in `AbstractBootstrap` was doing a racy read and has been adjusted.
Result:
Data race bugs around `getNow()` and `cause()` are no longer silent.
Motivation:
We can make things easier for implementations by providing some default methods
Modifications:
- Add default methods to Channel
- Remove code from AbstractChannel
Result:
Easier to implement custom Channel
Motivation:
The generics for the existing futures, promises, and listeners are too complicated.
This complication comes from the existence of `ChannelPromise` and `ChannelFuture`, which forces listeners to care about the particular _type_ of future being listened on.
Modification:
* Add a `FutureContextListener` which can take a context object as an additional argument. This allows our listeners to have the channel piped through to them, so they don't need to rely on the `ChannelFuture.channel()` method.
* Make the `FutureListener`, along with the `FutureContextListener` sibling, the default listener API, retiring the `GenericFutureListener` since we no longer need to abstract over the type of the future.
* Change all uses of `ChannelPromise` to `Promise<Void>`.
* Change all uses of `ChannelFuture` to `Future<Void>`.
* Change all uses of `GenericFutureListener` to either `FutureListener` or `FutureContextListener` as needed.
* Remove `ChannelFutureListener` and `GenericFutureListener`.
* Introduce a `ChannelFutureListeners` enum to house the constants that previously lived in `ChannelFutureListener`. These constants now implement `FutureContextListener` and take the `Channel` as a context.
* Remove `ChannelPromise` and `ChannelFuture` — all usages now rely on the plain `Future` and `Promise` APIs.
* Add static factory methods to `DefaultPromise` that allow us to create promises that are initialised as successful or failed.
* Remove `CompleteFuture`, `SucceededFuture`, `FailedFuture`, `CompleteChannelFuture`, `SucceededChannelFuture`, and `FailedChannelFuture`.
* Remove `ChannelPromiseNotifier`.
Result:
Cleaner generics and more straight forward code.
Motivation:
At present, the verification methods of `ZstdOptions` and `BrotliOptions` are not consistent, and the processing methods of `ZstdOptions` and `BrotliOptions` in `HttpContentCompressor` are also inconsistent.
The http2 module does not add zstd-jni dependency, so `ClassNotFoundException` may be thrown
Modification:
Added `Zstd.isAvailable()` check in `ZstdOptions` to be consistent, and added zstd-jni dependency in http2 module
Result:
The verification methods of `ZstdOptions` and `BrotliOptions` are consistent, and `ClassNotFoundException` will not be thrown
Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
Motivation:
Opportunities for clean up found while working on a different PR.
Modification:
* Dead code has been removed.
* Unnecessary parenthesis, qualifiers, etc. removed.
* Unused imports removed.
* Override annotations added where missing.
Result:
Cleaner code
Motivation:
There are lots of redundant variable declarations which should be inlined to make good look better.
Modification:
Made variables inlined.
Result:
Less redundant variable and more readable code.
Motivation:
io.netty.http2.validateContentLength SystemProperty was added as a way
to opt-out for compabitility for libraries/applications that violated
the RFC's content-length matching requirements [1] but have not yet been
fixed. This SystemProperty has been around for a few months now and it
is assumed these issues have now been addressed in 3rd party code.
[1] https://tools.ietf.org/html/rfc7540#section-8.1.2.6
Modifications:
- Remove the io.netty.http2.validateContentLength SystemProperty,
preventing folks from opting out of RFC's required content-length
matching.
Result:
No more escape hatch in H2 for content-length matching enforcement.
Motivation:
We should make variables `final` which are not reinstated again in code to match the code style and makes the code look better.
Modification:
Made couples of variables as `final`.
Result:
Variables marked as `final`.
Motivation:
Let's have fewer warnings about broken, missing, or abuse of javadoc comments.
Modification:
Added descriptions to throws clauses that were missing them.
Remove link clauses from throws clauses - these are implied.
Turned some javadoc comments into block comments because they were not applied to APIs.
Use code clauses instead of code tags.
Result:
Fewer javadoc crimes.
Motivation:
We should get rid of the unnecessary toString calls because they're redundant in nature.
Modification:
Removed unnecessary toString calls.
Result:
Better code
Motivation:
There are lots of imports which are unused. We should get rid of them to make the code look better,
Modification:
Removed unused imports.
Result:
No unused imports.
Bootstrap methods now return Future<Channel> instead of ChannelFuture
Motivation:
In #8516 it was proposed to at some point remove the specialised ChannelFuture and ChannelPromise.
Or at least make them not extend Future and Promise, respectively.
One pain point encountered in this discussion is the need to get access to the channel object after it has been initialised, but without waiting for the channel registration to propagate through the pipeline.
Modification:
Add a Bootstrap.createUnregistered method, which will return a Channel directly.
All other Bootstrap methods that previously returned ChannelFuture now return Future<Channel>
Result:
It's now possible to obtain an initialised but unregistered channel from a bootstrap, without blocking.
And the other bootstrap methods now only release their channels through the result of their futures, preventing racy access to the channels.
Motivation:
As suggested in [section 5.3.4 in http2 spec](https://datatracker.ietf.org/doc/html/rfc7540#section-5.3.4):
> When a stream is removed from the dependency tree, its dependencies can be moved to become dependent on the parent of the closed stream. The weights of new dependencies are recalculated by distributing the weight of the dependency of the closed stream proportionally based on the weights of its dependencies.
For example, we have stream A and B depend on connection stream with default weights (16), and stream C depends on A with max weight (256). When stream A was closed, we move stream C to become dependent on the connection stream, then we should distribute the weight of stream A to its children (only stream C), so the new weight of stream C will be 16. If we keep the weight of stream C unchanged, it will get more resource than stream B
Modification:
- distribute weight to its children when closing a stream
- add a unit test for the case above and fix other related unit tests
Result:
More spec-compliant and more appropriate stream reprioritization
Co-authored-by: Heng Zhang <zhangheng@imo.im>
Motivation:
We need to add `--add-exports java.base/sun.security.x509=ALL-UNNAMED` when running the tests for codec-http2 as some of the tests use SelfSignedCertificate.
Modifications:
- Add `--add-exports java.base/sun.security.x509=ALL-UNNAMED` when running the tests for codec-http2
- Ensure we export correct when running with JDK12, 13, 14 and 15 as well
Result:
No more tests failure due not be able to access classes
Motivation:
We did migrate all these modules to junit5 before but missed a few usages of junit4
Modifications:
Replace all junit4 imports by junit5 apis
Result:
Part of https://github.com/netty/netty/issues/10757
Motivation:
Currently, Netty only has BrotliDecoder which can decode Brotli encoded data. However, BrotliEncoder is missing which will encode normal data to Brotli encoded data.
Modification:
Added BrotliEncoder and CompressionOption
Result:
Fixes#6899.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
At the moment all methods in `ChannelHandler` declare `throws Exception` as part of their method signature. While this is fine for methods that handle inbound events it is quite confusing for methods that handle outbound events. This comes due the fact that these methods also take a `ChannelPromise` which actually need to be fullfilled to signal back either success or failure. Define `throws...` for these methods is confusing at best. We should just always require the implementation to use the passed in promise to signal back success or failure. Doing so also clears up semantics in general. Due the fact that we can't "forbid" throwing `RuntimeException` we still need to handle this in some way tho. In this case we should just consider it a "bug" and so log it and close the `Channel` in question. The user should never have an exception "escape" their implementation and just use the promise. This also clears up the ownership of the passed in message etc.
As `flush(ChannelHandlerContext)` and `read(ChannelHandlerContext)` don't take a `ChannelPromise` as argument this also means that these methods can never produce an error. This makes kind of sense as these really are just "signals" for the underlying transports to do something. For `RuntimeException` the same rule is used as for other outbound event handling methods, which is logging and closing the `Channel`.
Motifications:
- Remove `throws Exception` from signature
- Adjust code to not throw and just notify the promise directly
- Adjust unit tests
Result:
Much cleaner API and semantics.
Motivation:
8c73dbe9bd did migrate the codec-http2 code to use junit5 but missed two classes.
Modifications:
Adjust the rest of codec-http2 tests to use junit5
Result:
Part of https://github.com/netty/netty/issues/10757
Motivation:
We should update to use junit5 in all modules.
Modifications:
Adjust codec-http2 tests to use junit5
Result:
Part of https://github.com/netty/netty/issues/10757
Motivation:
This special case implementation of Promise / Future requires the implementations responsible for completing the promise to have knowledge of this class to provide value. It also requires that the implementations are able to provide intermediate status while the work is being done. Even throughout the core of Netty it is not really supported most of the times and so just brings more complexity without real gain.
Let's remove it completely which is better then only support it sometimes.
Modifications:
Remove Progressive* API
Result:
Code cleanup.... Fixes https://github.com/netty/netty/issues/8519
Motivation:
Sometime in the past we introduced the concept of Void*Promise. As it turned out this was not a good idea at all as basically each handler in the pipeline need to be very careful to correctly handle this. We should better just remove this "optimization".
Modifications:
- Remove Void*Promise and all the related APIs
- Remove tests which were related to Void*Promise
Result:
Less error-prone API
Motivation:
We have currently two test-failures in master. Let's disable these and then open a PR with a fix once we know why. This way we can make progress in master
Modifications:
Disable the two failing tests
Result:
Master builds again
Motivation:
Incoming `Http2GoAwayFrame#lastStreamId()` tells what was the last
streamId that the remote peer takes for processing [1]. We fire a
userEvent for all streams above that value to let users know those are
safe to retry on another connection. There is no need to go through
`forEachActiveStream` if `lastStreamId == Integer.MAX_VALUE` because
none of the streams can have id greater that MAX_VALUE.
1. https://datatracker.ietf.org/doc/html/rfc7540#section-6.8
Modifications:
- Return fast from `onHttp2GoAwayFrame` in `Http2MultiplexCodec` and
`Http2MultiplexHandler` if `lastStreamId() == Integer.MAX_VALUE`;
Result:
No unnecessary iteration over active streams on GO_AWAY if
`lastStreamId() == Integer.MAX_VALUE`.
Motivation:
Netty lacks client side support for decompressing Brotli compressed response bodies.
Modification:
* Introduce optional dependency to brotli4j by @hyperxpro. It will be up to the user to provide the brotli4j libraries for the target platform in the classpath. brotli4j is currently available for Linux, OSX and Windows, all for x86 only.
* Introduce BrotliDecoder in codec module
* Plug it onto `HttpContentDecompressor` for HTTP/1 and `DelegatingDecompressorFrameListener` for HTTP/2
* Add test in `HttpContentDecoderTest`
* Add `BrotliDecoderTest` that doesn't extend `AbstractDecoderTest` that looks flaky
Result:
Netty now support decompressing Brotli compressed response bodies.
Motivation:
`Http2FrameCodecBuilder` defines static factory methods `forClient()`
and `forServer()` that should be used to create a new instance.
The default ctor is useful only when users need to override behavior
of the existing builder. Those users should define another way to create
an instance.
Modifications:
- Decrease visibility of `Http2FrameCodecBuilder` default ctor from
`public` to `protected`;
- Add javadoc to clarity responsibilities;
Result:
Users of `Http2FrameCodecBuilder` are not confused why
`new Http2FrameCodecBuilder().build()` works for the server-side, but
does not work for the client-side.
__Motivation__
`Http2FrameCodecBuilder` constructor calls `server()` internally which disallows using certain methods on the builder later. Additionally, the constructor is package private which limits extension of the `Builder` as well as usage outside the available `forClient()` and `forServer()` static methods.
__Modification__
- Introduce a `public` no-arg constructor to `Http2FrameCodecBuilder`.
__Result__
`Http2FrameCodecBuilder` can now be used to create the codec with `Http2Connection` or `Http2ConnectionDecoder` and `Http2ConnectionEncoder` which was earlier not possible due to implicit call to `server()` by the `Http2FrameCodecBuilder` constructor.
Motivation:
RFC 8411 defines a new HTTP/2 pseudo header called `:protocol`:
- https://datatracker.ietf.org/doc/rfc8441/
Netty currently raises an exception when validating an `Http2Headers`.
Modifications:
- Added `Http2Headers.PseudoHeaderNames.PROTOCOL` so that `:protocol`
pseudo header is not rejected.
Result:
- A user can implement WebSockets with HTTP/2.
Motivation:
NullChecks resulting in a NullPointerException or IllegalArgumentException, numeric ranges (>0, >=0) checks, not empty strings/arrays checks must never be anonymous but with the parameter or variable name which is checked. They must be specific and should not be done with an "OR-Logic" (if a == null || b == null) throw new NullPointerEx.
Modifications:
* import static relevant checks
* Replace manual checks with ObjectUtil methods
Result:
All checks needed are done with ObjectUtil, some exception texts are improved.
Fixes#11170
Motivation:
SimpleChannelPromiseAggregator implements the promise API and allows for
multiple operations to share a common promise. It currently propagates
the last exception to occur, but this may mask the original exception
which lead to the last exception and make debugging more difficult.
Modifications:
- SimpleChannelPromiseAggregator propagates the first exception instead
of the last exception.
Result:
Fixes https://github.com/netty/netty/issues/11161.
Motivation:
In the method processHeaderState(), we have checked the http2 payload length against max frame size. But later for
different types of frames, we checked this again.
Modifications:
Removed the duplicate check in verify*() methods. And removed verifyPayloadLength() method, since it will not be used anymore.
Result:
Remove duplicate check and make the code cleaner.
Motivation: We have already calculated the payload length. So no need to calculate again when allocating ByteBuf
Modification: Used payloadLength variable instead of calculating the payload length again
Result: Re-use the variable value and make the code cleaner
Motivation:
There is a bug in `StreamBufferingEncoder` such that when client receives GOWAY while there are pending streams due to MAX_CONCURRENT_STREAMS, we see the following error:
```
io.netty.handler.codec.http2.Http2Exception$StreamException: Maximum active streams violated for this endpoint.
at io.netty.handler.codec.http2.Http2Exception.streamError(Http2Exception.java:147)
at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.checkNewStreamAllowed(DefaultHttp2Connection.java:896)
at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.createStream(DefaultHttp2Connection.java:748)
at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.createStream(DefaultHttp2Connection.java:668)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders0(DefaultHttp2ConnectionEncoder.java:201)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders(DefaultHttp2ConnectionEncoder.java:167)
at io.netty.handler.codec.http2.DecoratingHttp2FrameWriter.writeHeaders(DecoratingHttp2FrameWriter.java:53)
at io.netty.handler.codec.http2.StreamBufferingEncoder.writeHeaders(StreamBufferingEncoder.java:153)
at io.netty.handler.codec.http2.StreamBufferingEncoder.writeHeaders(StreamBufferingEncoder.java:141)
at io.grpc.netty.NettyClientHandler.createStreamTraced(NettyClientHandler.java:584)
at io.grpc.netty.NettyClientHandler.createStream(NettyClientHandler.java:567)
at io.grpc.netty.NettyClientHandler.write(NettyClientHandler.java:328)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:717)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:709)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:792)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:702)
at io.netty.channel.DefaultChannelPipeline.write(DefaultChannelPipeline.java:1015)
at io.netty.channel.AbstractChannel.write(AbstractChannel.java:289)
at io.grpc.netty.WriteQueue$AbstractQueuedCommand.run(WriteQueue.java:213)
at io.grpc.netty.WriteQueue.flush(WriteQueue.java:128)
at io.grpc.netty.WriteQueue.drainNow(WriteQueue.java:114)
at io.grpc.netty.NettyClientHandler.goingAway(NettyClientHandler.java:783)
at io.grpc.netty.NettyClientHandler.access$300(NettyClientHandler.java:91)
at io.grpc.netty.NettyClientHandler$3.onGoAwayReceived(NettyClientHandler.java:280)
at io.netty.handler.codec.http2.DefaultHttp2Connection.goAwayReceived(DefaultHttp2Connection.java:236)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.onGoAwayRead0(DefaultHttp2ConnectionDecoder.java:218)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onGoAwayRead(DefaultHttp2ConnectionDecoder.java:551)
at io.netty.handler.codec.http2.Http2InboundFrameLogger$1.onGoAwayRead(Http2InboundFrameLogger.java:119)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readGoAwayFrame(DefaultHttp2FrameReader.java:591)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:272)
at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160)
at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41)
at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:174)
at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:378)
at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1486)
at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1235)
at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1282)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:792)
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:475)
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:378)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Unknown Source)
```
The bug should come from the way that `StreamBufferingEncoder.writeHeaders()` handles the condition `connection().goAwayReceived()`. The current behavior is to delegate to `super.writeHeaders()` and let the stream fail, but this will end up with `Http2Exception` with the message "Maximum active streams violated for this endpoint" which is horrible. See e5951d46fc/codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java (L152-L155)
Modification:
Abort new stream immediately if goaway received *and* MAX_CONCURRENT_STREAM reached in `StreamBufferingEncoder` rather than delegating to the `writeHeaders()` method of its super class.
Result:
In the situation when GOAWAY received as well as MAX_CONCURRENT_STREAM exceeded, the client will fail the buffered streams with `Http2Error.NO_ERROR` and message "GOAWAY received" instead of "Maximum active streams violated for this endpoint".
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
We also need to ensure that all the header validation is done when a single header with the endStream flag is received
Modifications:
- Adjust code to always enforce the validation
- Add more unit tests
Result:
Always correctly validate
Motivation:
CONNECT requests have no path defined as stated in the HTTP/2 spec, at the moment we will throw an exception if we try to convert such a request to HTTP/1.1
Modifications:
- Don't throw an exception if we try to convert a HTTP/2 CONNECT request that has no path
- Add unit test
Result:
Related to https://github.com/netty/netty-incubator-codec-http3/pull/112.
Motivation:
We need to ensure we are still be able to correctly map errors to streams in all cases. The problem was that we sometimes called closeStreamRemote(...) in a finally block and so closed the underyling stream before the actual exception was propagated. This was only true in some cases and not in all. Generally speaking we should only call closeStreamRemote(...) if there was no error as in a case of error we should generate a RST frame.
Modifications:
- Only call closeStreamRemote(...) if no exeption was thrown and so let the Http2ConnectionHandler handle the exception correctly
- Add unit tests
Result:
Correctly handle errors even when endStream is set to true
Motivation
A GOAWAY frame (or any other HTTP/2 frame) should not be sent before the
connection preface. Clients that immediately close the channel may
currently attempt to send a GOAWAY frame before the connection preface,
resulting in servers receiving a seemingly-corrupt connection preface.
Modifications
* Ensure that the preface has been sent before attempting to
automatically send a GOAWAY frame as part of channel shutdown logic
* Add unit test that only passes with new behavior
Result
Fixes https://github.com/netty/netty/issues/11026
Co-authored-by: Bennett Lynch <Bennett-Lynch@users.noreply.github.com>
Motivation:
There are several overloads of streamError(), with one receiving the
Throwable to be made the cause of the new exception. However, the wrong
overload was being called and instead the IllegalArgumentException was
being passed as a message format argument which was summarily thrown
away as the message format didn't reference it.
Modifications:
Move IllegalArgumentException to proper argument position.
Result:
A useful exception, with the underlying cause available.
Motivation:
As stated by https://tools.ietf.org/html/rfc7540#section-8.1.2.6 we should report a stream error if the content-length does not match the sum of all data frames.
Modifications:
- Verify that the sum of data frames match if a content-length header was send.
- Handle multiple content-length headers and also handle negative values
- Add io.netty.http2.validateContentLength system property which allows to disable the more strict validation
- Add unit tests
Result:
Correctly handle the case when the content-length header was included but not match what is send and also when content-length header is invalid
