Commit Graph

683 Commits

Author SHA1 Message Date
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
Aayush Atharva
e4baa6737f Add toString method in DefaultHttp2WindowUpdateFrame (#10763)
Motivation:
We should have the `toString` method in `DefaultHttp2WindowUpdateFrame` because it makes debugging a little easy.

Modification:
Added `toString` method.

Result:
`toString` method to help in debugging.


Co-authored-by: Norman Maurer <norman_maurer@apple.com>
2020-11-03 09:37:42 +01:00
Aayush Atharva
3dce4de50f Remove extra empty line (#10754)
Motivation:
`Http2Frame` has extra empty line after `String name();`. However, it should not be there.

Modification:
Removed extra empty line.

Result:
Empty-line code style now matching with other classes.
2020-11-02 15:08:04 +01:00
Aayush Atharva
7d9b009cdf Fix typo in Http2FrameCodec#write(...) comment
Motivation:
`Http2FrameCodec#write(...)` has typo in comment.
`// In the event of manual SETTINGS ACK is is assumed the encoder will apply the earliest received but not`.
The typo is `is is`. However, it should be `it is`.

Modification:
Changed `is is` to `it is`.

Result:
Correct comment without typos.
2020-11-02 08:49:35 +01:00
Aayush Atharva
3aef41922b Fix typo in Http2DataFrame javadocs (#10755)
Motivation:
`Http2DataFrame#isEndStream()` JavaDoc says `Returns {@code true} if the END_STREAM flag ist set.`. The typo is `ist` word. However, it should be `is`.

Modification:
Changed `ist` to `is`.

Result:
Better JavaDoc by fixing the typo.
2020-10-30 15:59:46 +01:00
Aayush Atharva
1910528b30 Fix typo in Http2HeadersFrame javadocs (#10756)
Motivation:
`Http2HeadersFrame#isEndStream()` JavaDoc says `Returns {@code true} if the END_STREAM flag ist set.`. The typo is `ist` word. However, it should be `is`.

Modification:
Changed `ist` to `is`.

Result:
Better JavaDoc by fixing the typo.
2020-10-30 15:58:59 +01:00
Aayush Atharva
180f0b6ced Change variable name of Http2Headers (#10743)
Motivation:
`addHttp2ToHttpHeaders(int streamId, Http2Headers sourceHeaders, FullHttpMessage destinationMessage, boolean addToTrailer)` 
should match 
`addHttp2ToHttpHeaders(int streamId, Http2Headers inputHeaders, HttpHeaders outputHeaders, HttpVersion httpVersion, boolean isTrailer, boolean isRequest)`.

However, the `Http2Headers` variable name is different.

Modification:
Changed `sourceHeaders` to `inputHeaders`.

Result:
Variable and JavaDoc naming now correct.
2020-10-29 10:41:21 +01:00
Aayush Atharva
71f0e23bcb Make Http2ToHttpHeaderTranslator method package-private. (#10736)
Motivation:
Http2ToHttpHeaderTranslator is a private class but translateHeaders(Iterable<Entry<CharSequence, CharSequence>>) is public but it should be package-private.

Modification:
Removed public.

Result:
Correct access modifer.
2020-10-29 10:39:17 +01:00
Aayush Atharva
1bcfd7e39e Fix possible NPEs and IndexOutOfBoundsExceptions in HTTP/2 Codec (#10640)
Motivation:

There are possible NPEs and IndexOutOfBoundsExceptions in HTTP/2 code. 

Modification:
Fixed possible NPEs and IOOBEs

Result:
Better code
2020-10-26 14:42:14 +01:00
Aayush Atharva
9f8590e194 Fix JavaDoc of Http2Headers (#10711)
Motivation:
Http2Headers has JavaDoc error which says Sets the {@link PseudoHeaderName#AUTHORITY} header or {@code null} if there is no such header however it should be Sets the {@link PseudoHeaderName#AUTHORITY} header in Http2Headers#authority(CharSequence) methods because it only sets CharSequence.

This is true for all setters in Http2Headers.

Modification:
Fixed all JavaDoc errors.

Result:
Better JavaDoc.
2020-10-23 15:36:16 +02:00
Artem Smotrakov
b8ae2a2af4 Enable nohttp check during the build (#10708)
Motivation:

HTTP is a plaintext protocol which means that someone may be able
to eavesdrop the data. To prevent this, HTTPS should be used whenever
possible. However, maintaining using https:// in all URLs may be
difficult. The nohttp tool can help here. The tool scans all the files
in a repository and reports where http:// is used.

Modifications:

- Added nohttp (via checkstyle) into the build process.
- Suppressed findings for the websites
  that don't support HTTPS or that are not reachable

Result:

- Prevent using HTTP in the future.
- Encourage users to use HTTPS when they follow the links they found in
  the code.
2020-10-23 15:26:25 +02:00
Norman Maurer
3f2c5ccd46 Replace deprecated Assert.assertThat(...) with MatcherAssert.assertThat(...) (#10699)
Motivation:

junit deprecated Assert.assertThat(...)

Modifications:

Use MatcherAssert.assertThat(...) as replacement for deprecated method

Result:

Less deprecation warnings
2020-10-18 14:55:21 +02:00
Norman Maurer
5b00058fa7 Ensure we don't leak the ClassLoader in the backtrace (#10691)
Motivation:

We have a few classes in which we store and reuse static instances of various exceptions. When doing so it is important to also override fillInStacktrace() and so prevent the leak of the ClassLoader in the internal backtrace field.

Modifications:

- Add overrides of fillInStracktrace when needed
- Move ThrowableUtil usage in the static methods

Result:

Fixes https://github.com/netty/netty/pull/10686
2020-10-15 20:50:01 +02:00
Aayush Atharva
473471bac6 Use ObjectUtil for multiple operations (#10679)
Motivation:
We should use ObjectUtil for checking if Compression parameters are in range. This will reduce LOC and make code more readable.

Modification:
Used ObjectUtil

Result:
More readable code
2020-10-14 12:13:10 +02:00
Aayush Atharva
2464bee521 Simplify and Remove useless Bit operations from HpackHuffmanDecoder (#10656)
Motivation:
We should simplify and remove useless bit operations to make code more efficient, faster, and easier to understand.

Modification:
Simplified and Removed Useless Bit Operations

Result:
Simpler Code.
2020-10-12 11:45:08 +02:00
Aayush Atharva
423943e895 Remove extra line from end (#10673)
Motivation:
`Http2MultiplexHandler` have to 2 empty lines at the end instead of 1.

Modification:
Removed 1 extra line.

Result:
Little better code style.
2020-10-12 09:22:47 +02:00
Aayush Atharva
bb7a79452e Use ObjectUtil#checkPositive and Add missing JavaDoc In InboundHttp2ToHttpAdapter (#10637)
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.
2020-10-07 11:16:02 +02:00
Chris Vest
86c8f24d9a
Replace UNSAFE.throwException with alternatives supported on Java 8 (#10629)
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.
2020-10-02 08:29:07 +02:00
Maksym Ostroverkhov
a7d4c010a6 Http2ConnectionHandlerBuilder: make initialSettings() public (#10526)
Motivation:
be able to modify Http2Settings of Http2ConnectionHandlerBuilder.

Modifications:
make initialSettings() of Http2ConnectionHandlerBuilder public.

Result:
public initialSettings() of Http2ConnectionHandlerBuilder allow Http2Settings modification.
2020-09-02 21:13:14 +02:00
Chris Vest
0fd525f859
Use j.u.Consumer interface instead of bespoke copy (#10520)
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.
2020-08-31 16:42:58 +02:00
Nick Hill
d7c1407d4c Use ByteBuf#isAccessible() in more places (#10506)
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
2020-08-28 09:22:34 +02:00
Violeta Georgieva
a399175b52 Add validation when constructing Http2FrameLogger (#10495)
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
2020-08-21 16:11:12 +02:00
Norman Maurer
3c49d09eb9 Include TLSv1.3 ciphers as recommented ciphers for HTTP2 (#10480)
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
2020-08-13 20:33:21 +02:00
Norman Maurer
58654fff7d
Fix testStreamIsNotCreatedIfParentConnectionIsClosedConcurrently test (#10472)
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
2020-08-12 07:09:00 +02:00
Eric Anderson
adcffb6260 Permit h2 PRIORITY frames with a dependency on the connection (#10473)
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
2020-08-12 07:08:21 +02:00
Idel Pivnitskiy
e305b6af5e Create a new h2 stream only if the parent channel is still active (#10444)
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.
2020-08-06 14:04:31 +02:00
Idel Pivnitskiy
10354b62c8 New H2 stream promise may never complete (#10443)
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.
2020-08-06 08:34:44 +02:00