Commit Graph

632 Commits

Author SHA1 Message Date
Norman Maurer
0cf919c389
HTTP2: Guard against multiple ctx.close(...) calls with the same ChannelPromise (#10201)
Motivation:

Http2ConnectionHandler may call ctx.close(...) with the same promise instance multiple times if the timeout for gracefulShutdown elapse and the listener itself is notified. This can cause problems as other handlers in the pipeline may queue these promises and try to notify these later via setSuccess() or setFailure(...) which will then throw an IllegalStateException if the promise was notified already

Modification:

- Add boolean flag to ensure doClose() will only try to call ctx.close(...) one time

Result:

Don't call ctx.close(...) with the same promise multiple times when gradefulShutdown timeout elapses.
2020-04-23 11:25:39 +02:00
root
9c5008b109 [maven-release-plugin] prepare for next development iteration 2020-04-22 09:57:54 +00:00
root
d0ec961cce [maven-release-plugin] prepare release netty-4.1.49.Final 2020-04-22 09:57:26 +00:00
root
14e4afeba2 [maven-release-plugin] prepare for next development iteration 2020-03-17 09:20:54 +00:00
root
c10c697e5b [maven-release-plugin] prepare release netty-4.1.48.Final 2020-03-17 09:18:28 +00:00
feijermu
679c606f13
Add test case for Http2StreamChannelId. (#10108)
Motivation:

Http2StreamChannelId is Serializable. A test case is needed to ensure that it works.

Modification:

Add a test case about serialization.

Result:

Improve test coverage slightly.
2020-03-16 09:31:38 +01:00
root
c623a50d19 [maven-release-plugin] prepare for next development iteration 2020-03-09 12:13:56 +00:00
root
a401b2ac92 [maven-release-plugin] prepare release netty-4.1.47.Final 2020-03-09 12:13:26 +00:00
Bryce Anderson
7b946a781e
Make sure we always flush window update frames in AbstractHttp2StreamChannel (#10075)
Motivation:

Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.

Modifications:

- Ensure we flush if we exit the `doBeginRead()` method.
- Account for the Http2FrameCodec always synchronously finishing writes
  of window update frames.

Result:

Fixes #10072
2020-03-04 10:50:41 +01:00
root
e0d73bca4d [maven-release-plugin] prepare for next development iteration 2020-02-28 06:37:33 +00:00
root
ebe7af5102 [maven-release-plugin] prepare release netty-4.1.46.Final 2020-02-28 06:36:45 +00:00
Hyunjin Choi
1859170e69
Change String comparison to equals() from == (#10022)
Motivation:

Even if it was stored in the string constant pool, I thought it was safe to compare it through the Equals() method.

Modification:

So, I changed "==" comparison to equals() comparison

Result:

It has become safer to compare String values with different references and with the same values.
2020-02-13 09:57:14 +01:00
Norman Maurer
2d4b5abc99
HTTP/2 child channel may discard flush when done from an arbitrary thread (#10019)
Motivation:

We need to carefully manage flushes to ensure we not discard these by mistake due wrongly implemented consolidation of flushes.

Modifications:

- Ensure we reset flag before we actually call flush0(...)
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/10015
2020-02-11 14:43:04 +01:00
Bryce Anderson
238aa73e36 DefaultHttp2ConnectionDecoder notifies frame listener before connection of GOAWAYS (#10009)
Motivation:

Users of the DefaultHttp2ConnectionDecodcer are notified of inbound GoAwayFrames
after the connection has already closed any ignored streams, potentially
losing the signal that some streams may have been ignored by the peer and
are thus retryable.

Modifications:

Reorder the notifications of the frame and connection listeners to
propagate the frame first, giving the frame listeners the opportunity to
clean up ignored streams in their own way.

Result:
Fixes #9986
2020-02-10 10:26:54 +01:00
Norman Maurer
e7f291629b ClearTextHttp2ServerUpgradeHandler can be merged with inner PriorKnowledgeHandler (#10008)
Motivation:

ClearTextHttp2ServerUpgradeHandler is currently more complex then needed. We can simplify it by directly implement the prior-knowledge logic as part of the handler.

Modifications:

Merge inner PriorKnowledgeHandler logic into ClearTextHttp2ServerUpgradeHandler by extending ByteToMessageDecoder directly

Result:

Cleaner code and less pipeline operations
2020-02-08 08:49:55 +01:00
Norman Maurer
5bc644ba41
Ensure ChannelOptions are applied in the same order as configured in Http2StreamChannelBootstrap (#9998) (#10001)
Motivation:

https://github.com/netty/netty/pull/9848 changed how we handled ChannelOptions internally to use a ConcurrentHashMap. This unfortunally had the side-effect that the ordering may be affected and not stable anymore. Here the problem is that sometimes we do validation based on two different ChannelOptions (for example we validate high and low watermarks against each other). Thus even if the user specified the options in the same order we may fail to configure them.

Modifications:

- Use again a LinkedHashMap to preserve order

Result:

Apply ChannelOptions in correct and expected order
2020-02-07 09:14:16 +01:00
root
9b1ea10a12 [maven-release-plugin] prepare for next development iteration 2020-01-13 09:13:53 +00:00
root
136db8680a [maven-release-plugin] prepare release netty-4.1.45.Final 2020-01-13 09:13:30 +00:00
Ikhun Um
7f241f1f3c Fix typos in javadocs (#9900)
Motivation:

Javadocs should have no typos.

Modifications:

Fix two typos

Result:

Less typos.
2019-12-23 08:34:16 +01:00
root
79d4e74019 [maven-release-plugin] prepare for next development iteration 2019-12-18 08:32:54 +00:00
root
5ddf45a2d5 [maven-release-plugin] prepare release netty-4.1.44.Final 2019-12-18 08:31:43 +00:00
Norman Maurer
3cad3c0ae7
Revert "Validate pseudo and conditional HTTP/2 headers (#8619)" (#9893)
This reverts commit ffc3b2da72.
2019-12-17 17:43:52 +01:00
Sergey Skrobotov
84547029d9 Change DefaultByteBufHolder.equals() to treat instances of different classes as not equal (#9855)
# Motivation:
`DefaultByteBufHolder.equals()` considers another object equal if it's an instance of `ByteBufferHolder` and if the contents of two objects are equal. However, the behavior of `equals` method is not a part of the `ByteBufHolder` contract so `DefaultByteBufHolder`'s version may be causing violation of the symmetric property if other classes have different logic.
There are already a few classes that are affected by this: `DefaultHttp2GoAwayFrame`, `DefaultHttp2UnknownFrame`, and `SctpMessage` are all overriding `equals` method breaking the symmetric property.
Another effect of this behavior is that all instances with empty data are considered equal. That may not be desireable in the situations when instances are created for predefined constants, e.g. `FullBulkStringRedisMessage.NULL_INSTANCE` and `FullBulkStringRedisMessage.EMPTY_INSTANCE` in `codec-redis`. 

# Modification:
Make `DefaultByteBufHolder.equals()` implementation only work for the objects of the same class.

# Result:
- The symmetric property of the `equals` method is restored for the classes in question.
- Instances of different classes are not considered equal even if the content of the data they hold are the same.
2019-12-10 11:29:44 +01:00
时无两丶
0cde4d9cb4 Uniform null pointer check. (#9840)
Motivation:
Uniform null pointer check.

Modifications:

Use ObjectUtil.checkNonNull(...)

Result:
Less code, same result.
2019-12-09 09:47:35 +01:00
Norman Maurer
e875e59a9b
DefaultHttp2ConnectionEncoder writeHeaders method always send an head frame with a priority (#9852)
Motivation:

The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values even if the user didnt want too.

Modifications:

- Change DefaultHttp2ConnectionEncoder to call the correct Http2FrameWriter method depending on if the user wants to use priorities or not
- Adjust tests

Result:

Fixes https://github.com/netty/netty/issues/9842
2019-12-08 07:43:11 +01:00
Norman Maurer
e69c4173ea
Replace synchronized with ConcurrentHashMap in Http2StreamChannelBootstrap (#9848)
Motivation:

97361fa2c8 replace synchronized with ConcurrentHashMap in *Bootstrap classes but missed to do the same for the Http2 variant.

Modifications:

- Use ConcurrentHashMap
- Simplify code in *Bootstrap classes

Result:

Less contention
2019-12-06 10:59:55 +01:00
Norman Maurer
d0f94200e8
Call ctx.flush() when onStreamClosed(...) produces a window update frame (#9818)
Motivation:

We use the onStreamClosed(...) callback to return unconsumed bytes back to the window of the connection when needed. When this happens we will write a window update frame but not automatically call ctx.flush(). As the user has no insight into this it could in the worst case result in a "deadlock" as the frame is never written out ot the socket.

Modifications:

- If onStreamClosed(...) produces a window update frame call ctx.flush()
- Add unit test

Result:

No stales possible due unflushed window update frames produced by onStreamClosed(...) when not all bytes were consumed before the stream was closed
2019-11-28 11:11:32 +01:00
Norman Maurer
3654d2c245
Use EmbeddedChannel.finishAndReleaseAll() to remove boiler-plate code (#9824)
Motivation:

We can make use of EmbeddedChannel.finishAndReleaseAll() and so remove some code

Modifications:

Use finishAndReleaseAll()

Result:

Less code to maintain
2019-11-28 11:03:39 +01:00
Norman Maurer
98615a0de5
Don't send window update frame for unconsumed bytes when stream is already closed (#9816)
Motivation:

At the moment we send a window update frame for the connection + stream when a stream is closed and there are unconsumed bytes left. While we need to do this for the connection it makes no sense to write a window update frame for the stream itself as it is already closed

Modifications:

- Don't write the window update frame for the stream when the stream is closed
- Add unit test

Result:

Don't write the window frame for closed streams
2019-11-28 09:06:32 +01:00
Norman Maurer
5f5776c3e6
Correctly guard against multiple RST frames for the same stream (#9811)
Motivation:

Http2ConnectionHandler tries to guard against sending multiple RST frames for the same stream. Unfortunally the code is not 100 % correct as it only updates the state after it calls write. This may lead to the situation of have an extra RST frame slip through if the second write for the RST frame is done from a listener that is attached to the promise.

Modifications:

- Update state before calling write
- Add unit test

Result:

Only ever send one RST frame per stream
2019-11-27 06:54:19 +01:00
Bryce Anderson
09eed19438 Log expected STREAM_CLOSED exceptions for already closed streams at DEBUG level (#9798)
Motivation:

There is an intrinsic race between a local session resetting a stream
and the peer no longer sending any frames. This can result in the
session receiving frames for a stream that the local peer no longer
tracks. This results in a StreamException being thrown which triggers a
RST_STREAM frame, which is a good thing, but also logging at level WARN,
which is noisy for an expected and benign condition.

Modification:

Change the log level to DEBUG when logging stream errors with code
STREAM_CLOSED. All others are more interesting and will continue to be
logged at level WARN.

Additionally, it was found that DATA frames for streams that could not
have existed only resulted in a StreamException when the spec is clear
that such a situation should be fatal to the connection, resulting in a
GOAWAY(PROTOCOL_ERROR).

Fixes #8025.
2019-11-25 09:01:42 +01:00
monkey-mas
dc618d6046 Add test to check Connection-Specific headers are removed in HTTP/2 (by HttpConversionUtil.toHttp2Headers) (#9766)
Motivation:
To avoid regression regarding connection-specific headers[1], we should add a test.
[1] https://tools.ietf.org/html/rfc7540#section-8.1.2.2

Modification:
Add test that checks the following headers are removed.
- Connection
- Host
- Keep-Alive
- Proxy-Connection
- Transfer-Encoding
- Upgrade

Result:
There's no functional change.
2019-11-08 10:14:26 +01:00
zhangheng
044beae313 Remove padding when writing CONTINUATION frame (#9752)
Motivation:

Padding was removed from CONTINUATION frame in http2-spec, as showed in [PR](https://github.com/http2/http2-spec/pull/510). We should follow it.

Modifications:

- Remove padding when writing CONTINUATION frame in DefaultHttp2FrameWriter
- Add a unit test for writing large header with padding

Result:

More spec-compliant
2019-11-05 15:20:36 +01:00
monkey-mas
2796407fe8 Remove unnecessary line in Http2ClientUpgradeCodec (#9750)
Motivation:
To clean up code.

Modification:
Remove unnecessary line.

Result:
There's no functional change.
2019-11-04 11:20:04 +01:00
Norman Maurer
4f6c21fa97
Fix Http2Headers.method(...) javadocs (#9718)
Motivation:

The javadocs of Http2Headers.method(...) are incorrect, we should fix these.

Modifications:

Correct javadocs

Result:

Fixes https://github.com/netty/netty/issues/8068.
2019-10-29 19:51:28 +01:00
Norman Maurer
bf07592668 Fix typo in test which did introduce a failing test after ffc3b2da72 2019-10-28 09:26:29 +01:00
Julien Hoarau
ffc3b2da72 Validate pseudo and conditional HTTP/2 headers (#8619)
Motivation:

Netty HTTP/2 implementation is not 100% compliant to the spec. This
commit improves the compliance regarding headers validation,
in particular pseudo-headers and connection ones.

According to the spec:
   All HTTP/2 requests MUST include exactly one valid value for the
   ":method", ":scheme", and ":path" pseudo-header fields, unless it is
   a CONNECT request (Section 8.3).  An HTTP request that omits
   mandatory pseudo-header fields is malformed (Section 8.1.2.6).

Modifications:

- Introduce Http2HeadersValidator class capable of validating HTTP/2
headers
- Invoke validation from DefaultHttp2ConnectionDecoder#onHeadersRead
- Modify tests to use valid headers when required
- Modify HttpConversionUtil#toHttp2Headers to not add :scheme and
:path header on CONNECT method in order to conform to the spec

Result:

- Initial requests without :method, :path, :scheme will fail
- Initial requests with multiple values for :method, :path, :scheme
will fail
- Initial requests with an empty :path fail
- Requests with connection-specific header field will fail
- Requests with TE header different than "trailers" will fail
-
- Fixes 8.1.2.2 tests from h2spec #5761
- Fixes 8.1.2.3 tests from h2spec #5761
2019-10-27 16:13:01 +01:00
root
844b82b986 [maven-release-plugin] prepare for next development iteration 2019-10-24 12:57:00 +00:00
root
d066f163d7 [maven-release-plugin] prepare release netty-4.1.43.Final 2019-10-24 12:56:30 +00:00
Carl Mastrangelo
e8e7a206b3 Use fast HPACK comparisons when not checking sensitive headers (#9259)
Motivation:
Constant time comparison functions are used to compare HTTP/2 header
values, even if they are not sensitive.

Modification:
After checking for sensitivity, use fast comparison.

Result: Faster HPACK table reads/writes
2019-10-23 23:37:57 -07:00
Norman Maurer
9bf10f2dc5
Correctly propagate failures while update the flow-controller to the … (#9664)
Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes https://github.com/netty/netty/issues/9663
2019-10-22 05:40:14 -07:00
Norman Maurer
95230e01da
Try to reduce GC produced while writing headers (#9682)
Motivation:

bbc34d0eda introduced correct handling of "in process" setup of streams but there is some room for improvements. Often the writeHeaders(...) is completed directly which means there is not need to create the extra listener object.

Modifications:

- Only create the listener if we really need too.

Result:

Less GC
2019-10-17 10:12:20 -07:00
Matthew Miller
bbc34d0eda HTTP/2: Prevent memory leak when trying to create new streams on a connection that received a GOAWAY. (#9674)
Motivation:

In https://github.com/netty/netty/issues/8692, `Http2FrameCodec` was
updated to keep track of all "being initialized" streams, allocating
memory before initialization begins, and releasing memory after
initialization completes successfully.

In some instances where stream initialization fails (e.g. because this
connection has received a GOAWAY frame), this memory is never released.

Modifications:

This change updates the `Http2FrameCodec` to use a separate promise
for monitoring the success of sending HTTP2 headers. When sending of
headers fails, we now make sure to release memory allocated for stream
initialization.

Result:

After this change, failures in writing HTTP2 Headers (e.g. because this
connection has received a GOAWAY frame) will no longer leak memory.
2019-10-16 19:32:45 -07:00
康智冬
bd8cea644a Fix typos in javadocs (#9527)
Motivation:

We should have correct docs without typos

Modification:

Fix typos and spelling

Result:

More correct docs
2019-10-09 17:12:52 +04:00
Norman Maurer
271d8de9ec
Ensure we finish setup mock before we use it in Http2ConnectionRoundtripTest.headersWriteForPeerStreamWhichWasResetShouldNotGoAway (#9645)
Motivation:

We did dispatch the client code before we did finish setup the mock and so may end up with org.mockito.exceptions.misusing.UnfinishedStubbingException if the connect happens quickly enough.

See https://ci.netty.io/job/netty-centos6-java8-prb/1637/testReport/junit/io.netty.handler.codec.http2/Http2ConnectionRoundtripTest/headersWriteForPeerStreamWhichWasResetShouldNotGoAway/

Modifications:

First finish setup the mock and the dispatch.

Result:

Fix flacky test
2019-10-09 09:10:38 +04:00
Carl Mastrangelo
27397e87b2 Remember to return writability events to flow controller in HTTP2 Multiplexer (#9642)
Motivation:

Http2MultiplexCodec extends Http2FrameCodec extends Http2ConnectionHandler.  It appears  Http2MultiplexCodec overrode the channelWritabilityChanged method, which prevented the flow controller from becoming active.  In the case the parent channel becomes unwritable, and then later becomes writable, it needs to indicate that the child channels can still write data.   This is slightly confusing, because the child channels may still themselves be unwritable, but should still drain their data to the parent channel.

Modification:

Still propagate writability changes to the HTTP/2 flow controller

Result:

Fixes https://github.com/netty/netty/issues/9636
2019-10-08 18:40:23 +04:00
Pete Woods
45be693889 Add io.netty.handler.codec.http2.Http2ConnectionHandler for runtime GraalVM compilation (#9621)
Motivation:

Native image compilation is failing without extra flags:

```
Warning: Aborting stand-alone image build. No instances of io.netty.buffer.UnpooledHeapByteBuf are allowed in the image heap as this class should be initialized at image runtime. Object has been initialized by the io.netty.handler.codec.http2.Http2ConnectionHandler class initializer with a trace: 
 	at io.netty.buffer.Unpooled.wrappedBuffer(Unpooled.java:157)
	at io.netty.handler.codec.http2.Http2ConnectionHandler.<clinit>(Http2ConnectionHandler.java:74)
.  To fix the issue mark io.netty.buffer.UnpooledHeapByteBuf for build-time initialization with --initialize-at-build-time=io.netty.buffer.UnpooledHeapByteBuf or use the the information from the trace to find the culprit and --initialize-at-run-time=<culprit> to prevent its instantiation.

Detailed message:
Trace: 	object io.netty.buffer.ReadOnlyByteBuf
	object io.netty.buffer.UnreleasableByteBuf
	method io.netty.handler.codec.http2.Http2ConnectionHandler.access$500()
Call path from entry point to io.netty.handler.codec.http2.Http2ConnectionHandler.access$500(): 
	at io.netty.handler.codec.http2.Http2ConnectionHandler.access$500(Http2ConnectionHandler.java:66)
	at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.readClientPrefaceString(Http2ConnectionHandler.java:299)
	at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.decode(Http2ConnectionHandler.java:239)
	at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:505)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:444)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:283)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
	at io.netty.channel.AbstractChannelHandlerContext.access$600(AbstractChannelHandlerContext.java:56)
	at io.netty.channel.AbstractChannelHandlerContext$7.run(AbstractChannelHandlerContext.java:365)
	at com.oracle.svm.core.jdk.RuntimeSupport.executeHooks(RuntimeSupport.java:144)
	at com.oracle.svm.core.jdk.RuntimeSupport.executeStartupHooks(RuntimeSupport.java:89)
	at com.oracle.svm.core.JavaMainWrapper.runCore(JavaMainWrapper.java:143)
	at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:186)
	at com.oracle.svm.core.code.IsolateEnterStub.JavaMainWrapper_run_5087f5482cc9a6abc971913ece43acb471d2631b(generated:0)
```

Modification:

Add `io.netty.handler.codec.http2.Http2ConnectionHandler` for runtime compilation, as the buffer library's `io.netty.buffer.UnpooledHeapByteBuf` is also marked for runtime.

Result:

Native image compilation works again.
2019-10-07 09:03:07 +02:00
Nikolay Fedorovskikh
1fb5ff15a8 Fix possible NPE in DefaultHttp2UnknownFrame#equals (#9625)
Motivation:
`DefaultHttp2UnknownFrame#equals` may produce NPE due to
incorrect comparison of `stream` field.

Modification:
- Fix the `stream` field compare.
- Cleanup usage of class fields: use direct access instead of getters
(because the class is final).

Result:
No NPE in `equals` method.
2019-10-07 09:00:59 +02:00
Nikolay Fedorovskikh
4dc1eccf60 Make some inner classes static (#9624)
Motivation:
Classes `AbstractHttp2StreamChannel.Http2StreamChannelConfig`
and `DnsNameResolver.AddressedEnvelopeAdapter` may be static:
it doesn't reference its enclosing instance.

Modification:
Add `static` modifier.

Result:
Prevents a possible memory leak and uses less memory per class instance.
2019-10-07 08:14:02 +02:00
Norman Maurer
622cc232f0
Use configured ByteBufAllocator in InboundHttp2ToHttpAdapter (#9611)
Motivation:

At the moment we use Unpooled.buffer(...) in InboundHttp2ToHttpAdapter when we need to do a copy of the message. We should better use the configured ByteBufAllocator for the Channel

Modifications:

Change internal interface to also take the ByteBufAllocator as argument and use it when we need to allocate a ByteBuf.

Result:

Use the "correct" ByteBufAllocator in InboundHttp2ToHttpAdapter in all cases
2019-09-26 22:11:41 +02:00