Commit Graph

426 Commits

Author SHA1 Message Date
Julien Hoarau
61dba95091 Fix h2spec test 4.3 about invalid header block
Motivation:

HPackDecoder works on entire header block, we shouldn't encounter
incomplete header fields. If we do we should treat it as
a decoding error and according to the specification:

A decoding error in a header block MUST be treated as
a connection error (Section 5.4.1) of type COMPRESSION_ERROR.

Modifications:

* Check final state in HpackDecoder once we've decoded all the data.

Result:

* Throw a connection error if we receive incomplete header fields
* H2spec 4.3 tests all passes
2018-01-18 11:01:20 -08:00
Scott Mitchell
d6e600548b HTTP/2 Remove Http2FrameStream#CONNECTION_STREAM
Motivation:
Http2FrameStream#CONNECTION_STREAM is required to identify the
connection stream. However this leads to inconsistent usage from a user
perspective. When a user creates a Http2Frame for a non-connection
stream, the Http2MultiplexCodec automatically sets the stream, and the
user is never exposed to the Http2FrameStream object. However when the
user writes a Http2Frame for a connection stream they are required to
set the Http2FrameStream object. We can remove the Http2FrameStream#CONNECTION_STREAM
and keep the Http2FrameStream object internal, and therefore consistent
between the connection and non-connection use cases.

Modifications:
- Remove Http2FrameStream#CONNECTION_STREAM
- Update Http2FrameCodec to handle Http2Frame#stream() which returns
null

Result:
More consistent usage on http2 parent channel and http2 child channel.
2018-01-14 13:31:30 +01:00
Scott Mitchell
adf2596c36 Http2FrameCodecTest increase timeout
Motivation:
Http2FrameCodecTest#newOutboundStream has a timeout of 1 second and has been observed to timeout on CI servers.

Modifications:
- Increase the timeout to 5 seconds

Result:
Less false positive test failures on CI servers.
2017-12-22 19:32:18 +01:00
Scott Mitchell
336cee9b1f HpackDecoder#addHeader has an unused parameter
Motivation:
HpackDecoder#addHeader takes in the streamId as a parameter but no longer uses it.

Modifications:
- Remove the streamId parameter from HpackDecoder#addHeader

Result:
Less unused parameters in HpackDecoder.
2017-12-22 07:17:24 +01:00
Julien Hoarau
6b033c51a5 Add 32 bytes overhead per header entry when calculating headers length in HPackDecoder
Motivation:

According to the HTTP/2 Spec:

SETTINGS_MAX_HEADER_LIST_SIZE (0x6): This advisory setting informs a
peer of the maximum size of header list that the sender is
prepared to accept, in octets. The value is based on the
uncompressed size of header fields, including the length of the
name and value in octets plus an overhead of 32 octets for each
header field.

We were accounting for the 32 bytes when encoding in HpackEncoder,
but not when decoding in HPackDecoder.

Modifications:

- Add 32 bytes to the header list length for each entry when decoding
with HPackDecoder.

Result:

- We account for the 32 bytes overhead by header entry in HPackDecoder
2017-12-21 17:50:16 -08:00
Norman Maurer
942b993f2b Only enable validation of headers if original headers were validating as well.
Motiviation:

In our replace(...) methods we always used validation for the newly created headers while the original headers may not use validation at all.

Modifications:

- Only use validation if the original headers used validation as well.
- Ensure we create a copy of the headers in replace(...).

Result:

Fixes [#5226]
2017-12-21 07:32:29 +01:00
Scott Mitchell
144716f668
HTTP/2 support pending data larger than Integer.MAX_VALUE
Motivation:
Currently the remote flow controller limits the maximum amount of pending data to Integer.MAX_VALUE. The overflow handling is also not very graceful in that it may lead to infinite loops, or otherwise no progress being made.

Modifications:
- StreamByteDistributor and RemoteFlowController should support pending bytes of type long.

Result:
Fixes https://github.com/netty/netty/issues/4283
2017-12-20 08:55:15 -08:00
Moses Nakamura
94ab0dc442 codec-http2: Better keep track of nameLength in HpackDecoder.decode
Motivation:

http/2 counts header sizes somewhat inconsistently.  Sometimes, headers
which are substantively less than the header list size will be measured
as longer than the header list size.

Modifications:

Keep better track of the nameLength of a given name, so that we don't
accidentally end up reusing a nameLength.

Result:

More consistent measurement of header list size.

Fixes #7511.
2017-12-18 17:41:09 -08:00
Norman Maurer
264a5daa41 [maven-release-plugin] prepare for next development iteration 2017-12-15 13:10:54 +00:00
Norman Maurer
0786c4c8d9 [maven-release-plugin] prepare release netty-4.1.19.Final 2017-12-15 13:09:30 +00:00
Norman Maurer
b2bc6407ab [maven-release-plugin] prepare for next development iteration 2017-12-08 09:26:15 +00:00
Norman Maurer
96732f47d8 [maven-release-plugin] prepare release netty-4.1.18.Final 2017-12-08 09:25:56 +00:00
Moses Nakamura
0cac1a6c8c H2C upgrades should be ineligible for flow control (#7400)
H2C upgrades should be ineligible for flow control

Motivation:

When the h2c upgrade request is too big, the Http2FrameCodec complains
it's too big for flow control reasons, even though it's ineligible for
flow control.

Modifications:

Specially mark upgrade streams and make Http2FrameCodec know not to try
to flow control on those streams.

Result:

Servers won't barf when they receive an upgrade request with a fat
payload.

[Fixes #7280]
2017-12-07 16:46:16 -08:00
Scott Mitchell
7cced5576f Http2ConnectionHandler Http2ConnectionPrefaceAndSettingsFrameWrittenEvent propagation
Motivation:
Http2ConnectionHandler uses ctx.fireUserEvent to propagate the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent through the pipeline. This will propagate the event to the next inbound handler in the pipeline. If the user extends Http2ConnectionHandler the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent may be missed and initialization dependent upon this event will not be run.

Modifications:
- Http2ConnectionHandler should use userEventTriggered instead of ctx.fireUserEvent

Result:
Classes that extend Http2ConnectionHandler will see the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent user event.
2017-12-02 08:23:28 +01:00
Tomasz Jędrzejewski
e8540c2b7a Adding stable JDK9 module names that follow reverse-DNS style
Automatic-Module-Name entry provides a stable JDK9 module name, when Netty is used in a modular JDK9 applications. More info: http://blog.joda.org/2017/05/java-se-9-jpms-automatic-modules.html

When Netty migrates to JDK9 in the future, the entry can be replaced by actual module-info descriptor.

Modification:

The POM-s are configured to put the correct module names to the manifest.

Result:

Fixes #7218.
2017-11-29 11:50:24 +01:00
Scott Mitchell
7d213240ca HttpConversionUtil TE filtering robustness
Motivation:
HttpConversionUtil#toHttp2Headers has special code to filter the TE header name. However this filtering code may result in adding the <TE, TRAILERS> tuple in scenarios that are not appropriate. For example if a value containing trailers is seen it will be added, but the value could not actually be equal to trailers. Also CSV values are not supported.

Modifications:
- Account for CSV header values
- Account for the value containing 'trailers' but not actually being equal to 'trailers'

Result:
More robust parsing of the TE header.
2017-11-22 08:45:11 +01:00
Scott Mitchell
a3e41ba6eb HttpConversionUtils avoid intermediate collection allocation
Modifications:
HttpConversionUtil#toLowercaseMap requires an intermediate List to be allocated. This can be avoided with the recently added value iterator methods.

Modifications:
- Use HttpHeaders#valueCharSequenceIterator instead of getAll

Result:
Less intermediate object allocation and copying.
2017-11-20 14:05:03 -08:00
Scott Mitchell
e6126215e0 DefaultHttp2FrameWriter reduce object allocation
Motivation:
DefaultHttp2FrameWriter#writeData allocates a DataFrameHeader for each write operation. DataFrameHeader maintains internal state and allocates multiple slices of a buffer which is a maximum of 30 bytes. This 30 byte buffer may not always be necessary and the additional slice operations can utilize retainedSlice to take advantage of pooled objects. We can also save computation and object allocations if there is no padding which is a common case in practice.

Modifications:
- Remove DataFrameHeader
- Add a fast path for padding == 0

Result:
Less object allocation in DefaultHttp2FrameWriter
2017-11-20 08:10:59 -08:00
Scott Mitchell
a8bb9dc180 AbstractByteBuf readSlice bound check bug
Motivation:
AbstractByteBuf#readSlice relied upon the bounds checking of the slice operation in order to detect index out of bounds conditions. However the slice bounds checking operation allows for the slice to go beyond the writer index, and this is out of bounds for a read operation.

Modifications:
- AbstractByteBuf#readSlice and AbstractByteBuf#readRetainedSlice should ensure the desired amount of bytes are readable before taking a slice

Result:
No reading of undefined data in AbstractByteBuf#readSlice and AbstractByteBuf#readRetainedSlice.
2017-11-18 09:03:42 +01:00
Norman Maurer
78522cf3f0 Remove unused allocation introduced by d976dc108d 2017-11-17 17:19:36 +01:00
Moses Nakamura
d976dc108d codec-http2: Improve h1 to h2 header conversion
Motivation:

Netty could handle "connection" or "te" headers more gently when
converting from http/1.1 to http/2 headers.  Http/2 headers don't
support single-hop headers, so when we convert from http/1.1 to http/2,
we should drop all single-hop headers.  This includes headers like
"transfer-encoding" and "connection", but also the headers that
"connection" points to, since "connection" can be used to designate
other headers as single-hop headers.  For the "te" header, we can more
permissively convert it by just dropping non-conforming headers (ie
non-"trailers" headers) which is what we do for all other headers when
we convert.

Modifications:

Add a new blacklist to the http/1.1 to http/2 conversion, which is
constructed from the values of the "connection" header, and stop
throwing an exception when a "te" header is passed with a non-"trailers"
value.  Instead, drop all values except for "trailers".  Add unit tests
for "connection" and "te" headers when converting from http/1.1 to http/2.

Result:

This will improve the h2c upgrade request, and also conversions from
http/1.1 to http/2.  This will simplify implementing spec-compliant
http/2 servers that want to share code between their http/1.1 and http/2
implementations.

[Fixes #7355]
2017-11-17 09:09:52 +01:00
Norman Maurer
188ea59c9d [maven-release-plugin] prepare for next development iteration 2017-11-08 22:36:53 +00:00
Norman Maurer
812354cf1f [maven-release-plugin] prepare release netty-4.1.17.Final 2017-11-08 22:36:33 +00:00
Ning Sun
73e8122fc1 Fix sharable check logic
Motivation:

There is an logic issue when checking if ChannelHandler is sharable.

Modification:

Corrected || to &&
2017-11-08 08:36:07 -08:00
Scott Mitchell
35b0cd58fb HTTP/2 write of released buffer should not write and should fail the promise
Motivation:
HTTP/2 allows writes of 0 length data frames. However in some cases EMPTY_BUFFER is used instead of the actual buffer that was written. This may mask writes of released buffers or otherwise invalid buffer objects. It is also possible that if the buffer is invalid AbstractCoalescingBufferQueue will not release the aggregated buffer nor fail the associated promise.

Modifications:
- DefaultHttp2FrameCodec should take care to fail the promise, even if releasing the data throws
- AbstractCoalescingBufferQueue should release any aggregated data and fail the associated promise if something goes wrong during aggregation

Result:
More correct handling of invalid buffers in HTTP/2 code.
2017-11-06 14:38:58 -08:00
Scott Mitchell
911b2acc50 HTTP/2 Child Channel reading and flushing
Motivation:
If a child channel's read is triggered outside the parent channel's read
loop then it is possible a WINDOW_UPDATE will be written, but not
flushed.
If a child channel's beginRead processes data from the inboundBuffer and
then readPending is set to false, which will result in data not being
delivered if in the parent's read loop and more data is attempted to be
delievered to that child channel.

Modifications:
- The child channel must force a flush if a frame is written as a result
of reading a frame, and this is not in the parent channel's read loop
- The child channel must allow a transition from dequeueing from
beginRead into the parent channel's read loop to deliver more data

Result:
The child channel flushes data when reading outside the parent's read
loop, and has frames delivered more reliably.
2017-10-26 10:06:22 -07:00
Lionel Li
424bb09d24 Http2StreamFrameToHttpObjectCodec should handle 100-Continue properly
Motivation:
Http2StreamFrameToHttpObjectCodec was not properly encoding and
decoding 100-Continue HttpResponse/Http2SettingsFrame properly. It was
encoding 100-Continue FullHttpResponse as an Http2SettingFrame with
endStream=true, causing the child channel to terminate. It was not
decoding 100-Continue Http2SettingsFrame (endStream=false) as
FullHttpResponse. This should be fixed as it would cause http2 child
stream to prematurely close, and could cause HttpObjectAggregator to
fail if it's in the pipeline.

Modification:
- Fixed encode() to properly encode 100-Continue FullHttpResponse as
  Http2SettingsFrame with endStream=false
- Reject 100-Continue HttpResponse that are NOT FullHttpResponse
- Fixed decode() to properly decode 100-Continue Http2SettingsFrame
  (endStream=false) as a FullHttpResponse
- made Http2StreamFrameToHttpObjectCodec sharable so that it can b used
  among child streams within the same Http2MultiplexCodec

Result:
Now Http2StreamFrameToHttpObjectCodec should be properly handling
100-Continue responses.
2017-10-25 06:56:02 +02:00
Lionel Li
baf273aea8 Trigger user event when H2 conn preface & SETTINGS frame are sent
Motivation:
Previously client Http2ConnectionHandler trigger a user event
immediately when the HTTP/2 connection preface is sent. Any attempt to
immediately send a new request could cause the server to terminate the
connection, as it might not have received the SETTINGS frame from the
client. Per RFC7540 Section 3.5, the preface "MUST be followed by a
SETTINGS frame (Section 6.5), which MAY be empty."
(https://tools.ietf.org/html/rfc7540#section-3.5)

This event could be made more meaningful if it also indicates that the
initial client SETTINGS frame has been sent to signal that the channel
is ready to send new requests.

Modification:
- Renamed event to Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.
- Modified Http2ConnectionHandler to trigger the user event only if it
  is a client and it has sent both the preface and SETTINGS frame.

Result:
It is now safe to use the event as an indicator that the HTTP/2
connection is ready to send new requests.
2017-10-24 09:17:06 +02:00
Norman Maurer
55b501d0d4 Correctly update Channel writability when queueing data in SslHandler.
Motivation:

A regression was introduced in 86e653e which had the effect that the writability was not updated for a Channel while queueing data in the SslHandler.

Modifications:

- Factor out code that will increment / decrement pending bytes and use it in AbstractCoalescingBufferQueue and PendingWriteQueue
- Add test-case

Result:

Channel writability changes are triggered again.
2017-10-24 09:13:15 +02:00
Idel Pivnitskiy
4793daa589 Make Comparators Serializable
Motivation:

Objects of java.util.TreeMap or java.util.TreeSet will become
non-Serializable if instantiated with Comparators, which are not also
 Serializable. This can result in unexpected and difficult-to-diagnose
 bugs.

Modifications:

Implements Serializable for all classes, which implements Comparator.

Result:

Proper Comparators which will not force collections to
non-Serializable mode.
2017-10-22 03:40:28 +02:00
Idel Pivnitskiy
50a067a8f7 Make methods 'static' where it possible
Motivation:

Even if it's a super micro-optimization (most JVM could optimize such
 cases in runtime), in theory (and according to some perf tests) it
 may help a bit. It also makes a code more clear and allows you to
 access such methods in the test scope directly, without instance of
 the class.

Modifications:

Add 'static' modifier for all methods, where it possible. Mostly in
test scope.

Result:

Cleaner code with proper 'static' modifiers.
2017-10-21 14:59:26 +02:00
Idel Pivnitskiy
558097449c Add missed 'serialVersionUID' field for Serializable classes
Motivation:

Without a 'serialVersionUID' field, any change to a class will make
previously serialized versions unreadable.

Modifications:

Add missed 'serialVersionUID' field for all Serializable
classes.

Result:

Proper deserialization of previously serialized objects.
2017-10-21 14:41:18 +02:00
Cory Benfield
1b0a545921 Do not send Content-Length: 0 on 101 responses.
Motivation:

During code read of the Netty codebase I noticed that the Netty
HttpServerUpgradeHandler unconditionally sets a Content-Length: 0
header on 101 Switching Protocols responses. This explicitly
contravenes RFC 7230 Section 3.3.2 (Content-Length), which notes
that:

    A server MUST NOT send a Content-Length header field in any
    response with a status code of 1xx (Informational) or 204
    (No Content).

While it is unlikely that any client will ever be confused by
this behaviour, there is no reason to contravene this part of the
specification.

Modifications:

Removed the line of code setting the header field and changed the
only test that expected it to be there.

Result:

When performing the server portion of HTTP upgrade, the 101
Switching Protocols response will no longer contain a
Content-Length: 0 header field.
2017-10-21 14:36:19 +02:00
Norman Maurer
625a7426cd [maven-release-plugin] prepare for next development iteration 2017-09-25 06:12:32 +02:00
Norman Maurer
f57d8f00e1 [maven-release-plugin] prepare release netty-4.1.16.Final 2017-09-25 06:12:16 +02:00
Lionel Li
e069079aff Adapt Http2ServerDowngrader to work with clients
Motivation:
Http2ServerDowngrader is specifically built for server channels where
inbound Http2StreamFrames are converted into HttpRequests, and outbound
HttpResponses are converted into Http2StreamFrames. It can be easily
made to be more generic to work with client channels where inbound
Http2StreamFrames are converted into HttpResponses, and outbound
HttpRequests are converted into Http2StreamFrames.

Modification:
- Renamed Http2ServerDowngrader to a more general
  Http2StreamFrameToHttpObjectCodec
- Made it take in an "isServer" parameter to determine whether encoding
  inbound Http2StreamFrames should create HttpRequests (for server) or
  HttpResponses (for client)
- Norman fixed a leak in the unit test. Thanks! :-)

Result:
Now Http2StreamFrameToHttpObjectCodec can be used to translate
Http2StreamFrame to HttpObject for both server and client.
2017-09-22 11:04:43 -07:00
Moses Nakamura
1ff2e1fb5d Match Http2ClientUpgradeCodec to the new upgrade policy
Motivation:

We changed Http2ConnectionHandler to expect the upgrade method to be
called *after* we send the preface (ie add the handler to the pipeline)
but we forgot to change the Http2ClientUpgradeCodec to match the new
policy.  This meant that client-side h2c upgrades failed.

Modifications:

Reverse sending the preface and calling the upgrade method to match the
new policy.

Result:

Clients can initiate h2c upgrades successfully.
2017-09-20 12:42:43 -07:00
durigon
282aa35682 Fix NPE in InboundHttp2ToHttpAdapter
Motiviation:

At the moment an NPE is thrown if someone tries to use the InboundHttp2ToHttpAdapter.

Modifications:
- Ensure the status was null in "InboundHttp2ToHttpAdapter::onPushPromiseRead" before calling "HttpConversionUtil.parseStatus" methods.
- Fix setting status to OK in "InboundHttp2ToHttpAdapter::onPushPromiseRead".

Result:
Fixes [#7214].
2017-09-17 09:07:11 -07:00
Scott Mitchell
44bb3b6f3a DefaultHeaders value iterator
Motivation:
The Headers interface supports an interface to get all the headers values corresponding to a particular name. This API returns a List which requires intermediate storage and increases GC pressure.

Modifications:
- Add a method which returns an iterator over all the values for a specific name

Result:
Ability to iterator over values for a specific name with no intermediate collection.
2017-09-16 16:46:19 -07:00
Norman Maurer
bf0a53179a Correctly update writability state of Http2StreamChannel created by Http2MultiplexCodec.
Motivation:

We missed to mark the Http2StreamChannel as writable in some cases which could lead to the situation that a Channel never becomes writable. Also when a Http2StreamChannel was created we always marked it non-writable at the beginning which means if the user will only start writing once the Channel becomes writable it will never happen as it only became writable after the first header was written.

Modifications:

- Correctly handle updates for writability in all cases
- Change unit tests to cover this.

Result:

Fixes [#7179].
2017-09-14 08:25:31 -07:00
Norman Maurer
15611dadbb Fix NPE when using Http2ServerUpgradeCodec with Http2FrameCodec and Http2MultiplexCodec
Motiviation:

At the moment an NPE is thrown if someone tries to use the Http2ServerUpgradeCodec with Http2FrameCodec and Http2MultiplexCodec.

Modifications:

- Ensure the handler was added to the pipeline before calling on*Upgrade(...) methods.
- Add tests
- Fix adding of handlers after upgrade.

Result:

Fixes [#7173].
2017-09-14 08:23:53 -07:00
Nikolay Fedorovskikh
de9e666493 Fix hashCode() in Http2StreamChannelId
Motivation:
In `Http2StreamChannelId` a `hashCode()` is not consistent with `equals()`.

Modifications:
Make a `Http2StreamChannelId.hashCode()` consistent with `equals()`.

Result:
Faster hash map's operations where the Http2StreamChannelId as keys.
2017-09-08 10:38:16 +02:00
Nikolay Fedorovskikh
e500755086 Remove double comparing of content out of the DefaultHttp2GoAwayFrame.equals()
Motivation:
In `DefaultHttp2GoAwayFrame.equals()` a content compared twice: explicitly and in the `super` method.

Modifications:
Remove explicit content comparision.
Make `hashCode()` consistent with `equals()`.

Result:
A `DefaultHttp2GoAwayFrame.equals()` work faster.
2017-09-07 08:30:43 +02:00
Norman Maurer
870b5f5e4b Not add inboundStreamHandler for outbound streams created by Http2MultiplexCodec.
Motivation:

We must not add the inboundStreamHandler for outbound streams creates by Http2MultiplexCodec as the user will specify a handler via Http2StreamChannelBootstrap.

Modifications:

- Check if the stream is for outbound and if so not add the inboundStreamHandler to the pipeline
- Update tests so this is covered.

Result:

Fixes [#7178]
2017-09-06 08:37:29 +02:00
Norman Maurer
5c572f0f63 Ensure the tests complete on java7 and java9 as well.
Motivation:

379ac890f4 introduced the usage of the inline mock maker. This unfortunally not work on java7 and java9.

Modifications:

Just use reflection to create the event for now.

Result:

Netty tests pass again on java7 and java9 as well.
2017-09-04 20:20:04 +02:00
Norman Maurer
379ac890f4 Fix reference count issue when using Http2FrameCodec / Http2MultiplexCodec with HttpServerUpgradeHandler
Motivation:

When using  Http2FrameCodec / Http2MultiplexCodec with HttpServerUpgradeHandler reference count exception will be triggered.

Modifications:

- Correctly retain before calling InboundHttpToHttp2Adapter.handle
- Add unit test

Result:

Fixes [#7172].
2017-09-04 13:30:18 +02:00
Norman Maurer
b967805f32 [maven-release-plugin] prepare for next development iteration 2017-08-24 15:38:22 +02:00
Norman Maurer
da8e010a42 [maven-release-plugin] prepare release netty-4.1.15.Final 2017-08-24 15:37:59 +02:00
Nikolay Fedorovskikh
ba27456653 Use the index-based AsciiString constructor instead of substring()
Motivation:
The construction `new AsciiString(string.substring(...))` can be replaced with the `new AsciiString(string, start, length)` to avoid extra allocation.

Modifications:
Apply the described replacement in `HttpConversionUtil#setHttp2Authority`.

Result:
Less allocations.
2017-08-18 09:48:05 +02:00
Nikolay Fedorovskikh
4875a2aad4 Immediate caching the strings wrapped to AsciiString
Motivation:
The `AsciiString#toString` method calculate string value and cache it into field. If an `AsciiString` created from the `String` value, we can avoid rebuilding strings if we cache them immediately when creating `AsciiString`. It would be useful for constants strings, which already stored in the JVMs string table, or in cases where an unavoidable `#toString `method call is assumed.

Modifications:
- Add new static method `AsciiString#cache(String)` which save string value into cache field.
- Apply a "benign" data race in the `#hashCode` and `#toString` methods.

Result:
Less memory usage in some `AsciiString` use cases.
2017-08-15 06:22:14 +02:00