Commit Graph

709 Commits

Author SHA1 Message Date
Aayush Atharva
25a0a6d425 Make variables final (#11548)
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`.
2021-08-06 09:28:12 +02:00
Chris Vest
ef203fa6cb
Fix a number of javadoc issues (#11544)
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.
2021-08-06 09:14:04 +02:00
Aayush Atharva
6d6d2060af Remove unnecessary toString calls (#11550)
Motivation:
We should get rid of the unnecessary toString calls because they're redundant in nature.

Modification:
Removed unnecessary toString calls.

Result:
Better code
2021-08-05 14:17:23 +02:00
Aayush Atharva
b700793951 Remove Unused Imports (#11546)
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.
2021-08-05 14:08:07 +02:00
Chris Vest
6b11f7fbc2
All *Bootstrap methods that used to return ChannelFuture now return Future<Channel> (#11517)
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.
2021-08-03 19:43:38 +02:00
Heng Zhang
d920cbf143 Distribue weight to children when closing stream (#11490)
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>
2021-07-20 14:18:13 +02:00
Norman Maurer
0c411859eb SelfSignedCertificate should work in http2 tests (#11486)
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
2021-07-14 07:46:31 +02:00
Norman Maurer
1f6577ee92 Remove rest of junit4 usage (#11484)
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
2021-07-13 21:00:53 +02:00
skyguard1
154a3e0cab Add zstd http content compression support (#11470)
Motivation:

netty needs to support zstd content-encoding http content compression

Modification:

Add ZstdOptions, and modify HttpContentCompressor and CompressorHttp2ConnectionEncoder to support zstd compression

Result:

netty supports zstd http content compression

Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
2021-07-12 09:03:44 +02:00
Aayush Atharva
55c4e2ca82 Introduce BrotliEncoder (#11256)
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>
2021-07-08 12:01:28 +02:00
Norman Maurer
6ac8ef54f7
Remove throws Exception from ChannelHandler methods that handle o… (#11417)
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.
2021-07-08 10:16:00 +02:00
Norman Maurer
4aac55dfca Migrate the rest of codec-http2 to junit5 (#11424)
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
2021-06-30 11:11:39 +02:00
Norman Maurer
6909e51e09 Migrate codec-http2 to junit5 (#11422)
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
2021-06-30 10:43:57 +02:00
Norman Maurer
07baabaac5
Remove Progressive*Promise / Progressive*Future (#11374)
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
2021-06-09 08:32:38 +02:00
Norman Maurer
abdaa769de
Remove Void*Promise (#11348)
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
2021-06-08 14:22:16 +02:00
Norman Maurer
7b3e28f96f
Disable two tests which currently fail in master (#11344)
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
2021-06-01 13:41:20 +02:00
Idel Pivnitskiy
824fe7a0eb Don't iterate through active h2-streams if lastStreamId is MAX_VALUE (#11304)
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`.
2021-05-26 12:06:24 +02:00
Stephane Landelle
92ff402f0f Introduce BrotliDecoder (#10960)
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.
2021-05-10 15:39:01 +02:00
Idel Pivnitskiy
d2643ed835 Decrease visibility of Http2FrameCodecBuilder default ctor to protected (#11220)
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.
2021-05-04 07:37:47 +02:00
Violeta Georgieva
a0516ee414 Verify SslHandler#unwrap send fireChannelRead event after a notification for a handshake success (#11203)
Motivation:

Modifications:

- Add test

Result:

Verify fix in #11210

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
2021-04-29 12:21:00 +02:00
Nitesh Kant
672a325e93 Improve Http2FrameCodecBuilder usability (#11195)
__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.
2021-04-29 10:27:40 +02:00
Trustin Lee
6389f18a16 Add a new HTTP/2 pseudo header :protocol (#11192)
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.
2021-04-26 09:29:05 +02:00
Boris Unckel
c1a04297e7 Utilize i.n.u.internal.ObjectUtil to assert Preconditions (codec*) (#11170) (#11185)
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
2021-04-22 16:05:06 +02:00
Scott Mitchell
8c12ad4cee SimpleChannelPromiseAggregator use first exception instead of last (#11168)
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.
2021-04-22 12:18:28 +02:00
Liyuan
120fb6fcdf Remove duplicate HTTP2 payload length check against max frame size
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.
2021-04-21 16:13:46 +02:00
Liyuan
0fdf2e7f3a Used already calculated SETTINGS frame payload length when allocating ByteBuf
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
2021-04-21 16:13:39 +02:00
ZHANG Dapeng
c2f893fcf3 Fix StreamBufferingEncoder GOAWAY bug (#11144)
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>
2021-04-19 16:06:13 +02:00
Norman Maurer
7971a8ca49 Merge pull request from GHSA-f256-j965-7f32
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
2021-03-30 11:55:49 +02:00
Norman Maurer
0e16f0e819 Allow to have an empty path when convert a CONNECT request (#11108)
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.
2021-03-24 10:52:08 +01:00
Norman Maurer
9c39c0de35 Ensure we can correctly propagate exceptions to streams even if endStream flag is set (#11105)
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
2021-03-23 20:28:00 +01:00
Bennett Lynch
7753431e48 Do not send GOAWAY frame before connection preface (#11107)
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>
2021-03-23 09:05:49 +01:00
Eric Anderson
1914679b00 codec-http2: Exception as cause, not message format argument (#11084)
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.
2021-03-14 14:15:26 +01:00
Norman Maurer
8da6ed3fc0 Merge pull request from GHSA-wm47-8v5p-wjpj
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
2021-03-14 14:15:22 +01:00
Norman Maurer
5de2ddeac2 Add unit tests that validate that header validation exception is propagated to the child channels (#11076)
Motivation:

We should validate the exception is actually propagted

Modifications:

Add unit tests

Result:

Verify all works as expected
2021-03-12 11:40:05 +01:00
Norman Maurer
7f2925350a Propagate SSLException to the Http2StreamChannels (#11023)
Motivation:

When TLSv1.3 is used (or TLS_FALSE_START) together with mTLS the handshake is considered successful before the server actually did verify the key material that was provided by the client. If the verification fails we currently will just close the stream without any extra information which makes it very hard to debug on the client side.

Modifications:

- Propagate SSLExceptions to the active streams
- Add unit test

Result:

Better visibility into why a stream was closed
2021-02-19 08:41:55 +01:00
Norman Maurer
b19b8c39cb Ignore priority frames for non existing streams and so prevent a NPE (#10943)
Motivation:

https://github.com/netty/netty/pull/10765 added support for push promise and priority frames when using the Http2FrameCodec. Unfortunally it didnt correctly guard against the possibility to receive a priority frame for an non-existing stream, which resulted in a NPE

Modifications:

- Ignore priority frame for non existing stream
- Correctly implement equals / hashcode for DefaultHttp2PriorityFrame
- Add unit tests

Result:

Fixes https://github.com/netty/netty/issues/10941
2021-01-18 14:14:00 +01:00
Norman Maurer
f6c1c0ea9c Use GracefulShutdown when stream space is exhausted (#10946)
Motivation:

We should use GracefulShutdown when we try to create a stream and fail it because the stream space is exhausted as we may still want to process the active streams.

Modifications:

- Use graceful shutdown
- Add unit test

Result:

More graceful handling of stream creation failure due stream space exhaustation
2021-01-16 09:55:53 +01:00
Aayush Atharva
2139ff9fe3 Add PushPromise and Priority Frame support in Http2FrameCodec (#10765)
Motivation:
Right now, we don't have to handle Push Promise Read in `Http2FrameCodec`. Push Promise is one of the key features of HTTP/2 and we should support it in our `Http2FrameCodec`.

Modification:
Added `Http2PushPromiseFrame` and `Http2PushPromiseFrame` to handle Push Promise and Promise Frame.

Result:
Fixes #10748
2020-12-27 14:38:41 +01:00
Oleksii Kachaiev
85ec20ecbd Improve performance of HPACK static table lookup (#10840)
Motivation:

HPACK static table is organized in a way that fields with the same
name are sequential. Which means when doing sequential scan we can
short-circuit scan on name mismatch.

Modifications:

* `HpackStaticTable.getIndexIndensitive` returns -1 on name mismatch
rather than keep scanning.
* `HpackStaticTable` statically defined max position in the array
where name duplication is possible (after the given index there's
no need to check for other fields with the same name)
* Benchmark for different lookup patterns

Result:

Better HPACK static table lookup performance.

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
2020-12-21 15:34:31 +01:00
valerauko
28ef4d18f9 Let Http2ConnectionHandler close stream with voidPromise (#10819)
Motivation:

Http2ConnectionHandler tries to addListener to the future without checking if it's void. If it is void, this will fail and generate an exception. 
 
Modifications:
Unvoid the promise in writeData()
 
Result:

Fixes #10816, Writing with a voidPromise no longer generates exceptions.
2020-12-02 10:12:23 +01:00
Aayush Atharva
3f9522cd66 Fix comment typo DelegatingDecompressorFrameListener (#10789)
Motivation:
`DelegatingDecompressorFrameListener#initDecompressor` has multiple dots `.` in comments. However, it should not have that.

Modification:
Removed multiple dots.

Result:
Clean comment
2020-11-16 09:03:54 +01:00
Norman Maurer
eeece4cfa5 Use http in xmlns URIs to make maven release plugin happy again (#10788)
Motivation:

https in xmlns URIs does not work and will let the maven release plugin fail:

```
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.779 s
[INFO] Finished at: 2020-11-10T07:45:21Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.5.3:prepare (default-cli) on project netty-parent: Execution default-cli of goal org.apache.maven.plugins:maven-release-plugin:2.5.3:prepare failed: The namespace xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" could not be added as a namespace to "project": The namespace prefix "xsi" collides with an additional namespace declared by the element -> [Help 1]
[ERROR]
```

See also https://issues.apache.org/jira/browse/HBASE-24014.

Modifications:

Use http for xmlns

Result:

Be able to use maven release plugin
2020-11-10 10:51:05 +01:00
Aayush Atharva
884800221f Add ByteBuf parameter HttpConversionUtil (#10785)
Motivation:
`HttpConversionUtil#toFullHttpResponse` and `HttpConversionUtil#toFullHttpRequest` has `ByteBufAllocator` which is used for building `FullHttpMessage` and then data can be appended with `FullHttpMessage#content`. However, there can be cases when we already have `ByteBuf` ready with data. So we need a parameter to add `ByteBuf` directly into `FullHttpMessage` while creating it.

Modification:
Added `ByteBuf` parameter,

Result:
More functionality for handling `FullHttpMessage` content.
2020-11-10 07:56:48 +01:00
Aayush Atharva
12c3b9c7e6 Add HttpScheme Support in HttpToHttp2ConnectionHandler (#10641)
Motivation:
We should have a method to add `HttpScheme` if `HttpRequest` does not contain `x-http2-scheme` then we should use add it if `HttpToHttp2ConnectionHandler` is build using specified `HttpScheme`.

Modification:
Added `HttpScheme` in `HttpToHttp2ConnectionHandlerBuilder`.

Result:
Automatically add `HttpScheme` if missing in `HttpRequest`.
2020-11-05 15:54:05 +01:00
Eric Anderson
0754dac14d codec-http2: Correct last-stream-id for HEADERS-triggered connection error (#10775)
Motivation:

When parsing HEADERS, connection errors can occur (e.g., too large of
headers, such that we don't want to HPACK decode them). These trigger a
GOAWAY with a last-stream-id telling the client which streams haven't
been processed.

Unfortunately that last-stream-id didn't include the stream for the
HEADERS that triggered the error. Since clients are free to silently
retry streams not included in last-stream-id, the client is free to
retransmit the request on a new connection, which will fail the
connection with the wrong last-stream-id, and the client is still free
to retransmit the request.

Modifications:

Have fatal connection errors (those that hard-cut the connection)
include all streams in last-stream-id, which guarantees the HEADERS'
stream is included and thus should not be silently retried by the HTTP/2
client.

This modification is heavy-handed, as it will cause racing streams to
also fail, but alternatives that provide precise last-stream-id tracking
are much more invasive. Hard-cutting the connection is already
heavy-handed and so is rare.

Result:

Fixes #10670
2020-11-05 09:54:49 +01:00
Arthur Gonigberg
f053870f38 Drop unknown frames on connection stream (#10771)
Motivation:

We received a [bug report](https://bugs.chromium.org/p/chromium/issues/detail?id=1143320) from the Chrome team at Google, their canary builds are failing [HTTP/2 GREASE](https://tools.ietf.org/html/draft-bishop-httpbis-grease-00) testing to netflix.com.

The reason it's failing is that Netty can't handle unknown frames without an active stream created. Let me know if you'd like more info, such as stack traces or repro steps. 

Modification:

The change is minor and simply ignores unknown frames on the connection stream, similarly to `onWindowUpdateRead`.

Result:

I figured I would just submit a PR rather than filing an issue, but let me know if you want me to do that for tracking purposes.
2020-11-04 14:01:33 +01:00
Chris Vest
10af555f46
ByteProcessor shouldn't throw checked exception (#10767)
Motivation:
There is no need for ByteProcessor to throw a checked exception.
The declared checked exception causes unnecessary code complications just to propagate it.
This can be cleaned up.

Modification:
ByteProcessor.process no longer declares to throw a checked exception, and all the places that were trying to cope with the checked exception have been simplified.

Result:
Simpler code.
2020-11-03 18:54:16 +01:00
Chris Vest
ff2e790e89 Revert "ByteProcessor shouldn't throw checked exception"
This reverts commit b70d0fa6e3.
2020-11-03 16:12:54 +01:00
Chris Vest
b70d0fa6e3 ByteProcessor shouldn't throw checked exception
Motivation:
There is no need for ByteProcessor to throw a checked exception.
The declared checked exception causes unnecessary code complications just to propagate it.
This can be cleaned up.

Modification:
ByteProcessor.process no longer declares to throw a checked exception, and all the places that were trying to cope with the checked exception have been simplified.

Result:
Simpler code.
2020-11-03 16:12:13 +01:00
Aayush Atharva
2a1e031fa3 HttpConversionUtil#toHttpResponse should use false in isRequest parameter (#10760)
Motivation:
`HttpConversionUtil#toHttpResponse` translates `Http2Headers` to `HttpResponse`. It uses `#addHttp2ToHttpHeaders(..., boolean isRequest)` to do so. However, `isRequest` field is set to `true` instead of `false`. It should be set to `false` because we're doing conversion of Response not Request.

Modification:
Changed `true` to `false`.

Result:
Correctly translates `Http2Headers` to `HttpResponse`.
2020-11-03 09:39:03 +01:00