Commit Graph

664 Commits

Author SHA1 Message Date
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
Norman Maurer
08284dbbcd Ensure we call promise.setUncancellable() before trying to process in DefaultHttp2StreamChannel.
Motivation:

We should call promise.setUncancellable() in DefaultHttp2StreamChannel.Unsafe impl to detect if the operation was cancelled.

Modifications:

Add promise.setUncancellable() calls

Result:

More correct handling of cancelled promises
2017-08-12 09:19:36 +02:00
Norman Maurer
74f24a5c19 Finish work on http2 child channel implementation and http2 frame api.
Motivation:

Our http2 child channel implementation was not 100 % complete and had a few bugs. Beside this the performance overhead was non-trivial.

Modifications:

There are a lot of modifications, the most important....
  * Http2FrameCodec extends Http2ConnectionHandler and Http2MultiplexCodec extends Http2FrameCodec to reduce performance heads and inter-dependencies on handlers in the pipeline
  * Correctly handle outbound flow control for child channels
  * Support unknow frame types in Http2FrameCodec and Http2MultiplexCodec
  * Use a consistent way how to create Http2ConnectionHandler, Http2FrameCodec and Http2MultiplexCodec (via a builder)
  * Remove Http2Codec and Http2CodecBuilder as the user should just use Http2MultipleCodec and Http2MultiplexCodecBuilder now
  * Smart handling of flushes from child channels to reduce overhead
  * Reduce object allocations
  * child channels always use the same EventLoop as the parent Channel to reduce overhead and simplify implementation.
  * Not extend AbstractChannel for the child channel implementation to reduce overhead in terms of performance and memory usage
  * Remove Http2FrameStream.managedState(...) as the user of the child channel api should just use Channel.attr(...)

Result:

Http2MultiplexCodec (and so child channels) and Http2FrameCodec are more correct, faster and more feature complete.
2017-08-11 12:41:28 +02:00
buchgr
3a2b462a67 Remove the concept of pending streams. The close future can only be accessed once a stream is active. 2017-08-11 12:41:28 +02:00
buchgr
5380c7c3e3 HTTP/2 Child Channel and FrameCodec Feature Parity.
Motivation:

This PR (unfortunately) does 4 things:
1) Add outbound flow control to the Http2MultiplexCodec:
   The HTTP/2 child channel API should interact with HTTP/2 outbound/remote flow control. That is,
   if a H2 stream used up all its flow control window, the corresponding child channel should be
   marked unwritable and a writability-changed event should be fired. Similarly, a unwritable
   child channel should be marked writable and a writability-event should be fired, once a
   WINDOW_UPDATE frame has been received. The changes are (mostly) contained in ChannelOutboundBuffer,
   AbstractHttp2StreamChannel and Http2MultiplexCodec.

2) Introduce a Http2Stream2 object, that is used instead of stream identifiers on stream frames. A
   Http2Stream2 object allows an application to attach state to it, and so a application handler
   no longer needs to maintain stream state (i.e. in a map(id -> state)) himself.

3) Remove stream state events, which are no longer necessary due to the introduction of Http2Stream2.
   Also those stream state events have been found hard and complex to work with, when porting gRPC
   to the Http2FrameCodec.

4) Add support for HTTP/2 frames that have not yet been implemented, like PING and SETTINGS. Also add
   a Http2FrameCodecBuilder that exposes options from the Http2ConnectionHandler API that couldn't else
   be used with the frame codec, like buffering outbound streams, window update ratio, frame logger, etc.

Modifications:

1) A child channel's writability and a H2 stream's outbound flow control window interact, as described
   in the motivation. A channel handler is free to ignore the channel's writability, in which case the
   parent channel is reponsible for buffering writes until a WINDOW_UPDATE is received.

   The connection-level flow control window is ignored for now. That is, a child channel's writability
   is only affected by the stream-level flow control window. So a child channel could be marked writable,
   even though the connection-level flow control window is zero.

2) Modify Http2StreamFrame and the Http2FrameCodec to take a Http2Stream2 object intstead of a primitive
   integer. Introduce a special Http2ChannelDuplexHandler that has newStream() and forEachActiveStream()
   methods. It's recommended for a user to extend from this handler, to use those advanced features.

3) As explained in the documentation, a new inbound stream active can be detected by checking if the
   Http2Stream2.managedState() of a Http2HeadersFrame is null. An outbound stream active can be detected
   by adding a listener to the ChannelPromise of the write of the first Http2HeadersFrame. A stream
   closed event can be listened to by adding a listener to the Http2Stream2.closeFuture().

4) Add a simple Http2FrameCodecBuilder and implement the missing frame types.

Result:

1) The Http2MultiplexCodec supports outbound flow control.
2) The Http2FrameCodec API makes it easy for a user to manage custom stream specific state and to create
   new outbound streams.
3) The Http2FrameCodec API is much cleaner and easier to work with. Hacks like the ChannelCarryingHeadersFrame
   are no longer necessary.
4) The Http2FrameCodec now also supports PING and SETTINGS frames. The Http2FrameCodecBuilder allows the Http2FrameCodec
   to use some of the rich features of the Http2ConnectionHandler API.
2017-08-11 12:41:28 +02:00
Norman Maurer
348745608f Fix incorrect javadocs in Http2RemoteFlowController
Motivation:

The javadocs of Http2RemoteFlowController.isWritable(...) are incorrect.

Modifications:

Update javadocs to reflect reality.

Result:

Correct javadocs.
2017-08-08 07:47:18 +02:00
Scott Mitchell
0f8ecdcad5 Http2FrameLogger frame labels incorrect
Motivation:
The labels identifying the frame types in Http2FrameLogger are not always correct.

Modification:
- Correct the string labels to indicate the right frame type in Http2FrameLogger

Result:
Logs are more correct.
2017-08-07 10:24:17 -07:00
Norman Maurer
d56f403c69 First call channelReadComplete(...) before flush(...) for better performance
Motivation:

In Http2ConnectionHandler we call flush(...) in channelReadComplete(...) to ensure we update the flow-controller and write stuff to the remote peer. We should better flip the order and so may be able to pick up more bytes.

Modifications:

Change order of calls.

Result:

Better performance
2017-08-04 11:55:35 +02:00
Norman Maurer
52f384b37f [maven-release-plugin] prepare for next development iteration 2017-08-02 12:55:10 +00:00
Norman Maurer
8cc1071881 [maven-release-plugin] prepare release netty-4.1.14.Final 2017-08-02 12:54:51 +00:00
chhsiao90
8320a45c15 Configures HTTP2 pipeline with more proper way
Motivation:

When we use pipeline.replace and we still had ongoing inbound, then
there will be some problem that inbound message would go to wrong
handlers. So we add handler first, and remove self after add, so that
the next handler will be the correct one.

Modifications:

Uses remove after addAfter instead of replace.

Result:

Fixed #6881
2017-08-02 06:58:55 +02:00
Norman Maurer
2988fb8eeb Ensure Http2FrameCodec uses Http2Settings.defaultSettings()
Motivation:

Http2FrameCodec should use Http2Settings.defaultSettings() when no Http2Settings were specified by the user.

Modifications:

Replace new Http2Settings() with Http2Settings.defaultSettings()

Result:

Use correct Http2Settings by default when using Http2FrameCodec in all cases.
2017-08-01 07:07:05 +02:00
Vladimir Gordiychuk
fe8ecea366 Http2FrameLogger avoid hex dump of the ByteBufs when log disabled
Motivation:

Currentry logger create hex dump even if log write will not apply.
It's unecessary GC overhead.

Modifications:

Restore optimization from #3492

Result:

Fixes #7025
2017-07-26 21:21:04 +02:00
Spencer Fang
732b145842 Http2ConnectionHandler: allow graceful shutdown to wait forever
Motivation:

There should be a way to allow graceful shutdown to wait for all open streams to close without a timeout. Using gracefulShutdownTimeoutMillis with a large value is a bit of a hack, and has a gotcha that sufficiently large values will overflow the long, resulting in a ClosingChannelFutureListener that executes immediately.

Modification:

Allow to use gracefulShutdownTimeoutMillis(-1) to express waiting until all streams are closed.

Result:

We can now shutdown the connection without a forced timeout.
2017-07-26 20:40:24 +02:00
Scott Mitchell
a91df58ca1 HTTP/2 enforce HTTP message flow
Motivation:
codec-http2 currently does not strictly enforce the HTTP/1.x semantics with respect to the number of headers defined in RFC 7540 Section 8.1 [1]. We currently don't validate the number of headers nor do we validate that the trailing headers should indicate EOS.

[1] https://tools.ietf.org/html/rfc7540#section-8.1

Modifications:
- DefaultHttp2ConnectionDecoder should only allow decoding of a single headers and a single trailers
- DefaultHttp2ConnectionEncoder should only allow encoding of a single headers and optionally a single trailers

Result:
Constraints of RFC 7540 restricting the number of headers/trailers is enforced.
2017-07-19 13:37:23 -07:00
Norman Maurer
dbd82e07b1 Let Http2ServerUpgradeCodec support Http2FrameCodec
Motivation:

Http2ServerUpgradeCodec should support Http2FrameCodec.

Modifications:

- Add support for Http2FrameCodec
- Add example that uses Http2FrameCodec

Result:

More flexible use of Http2ServerUpgradeCodec
2017-07-19 11:12:10 +02:00
Norman Maurer
2a376eeb1b [maven-release-plugin] prepare for next development iteration 2017-07-06 13:24:06 +02:00
Norman Maurer
c7f8168324 [maven-release-plugin] prepare release netty-4.1.13.Final 2017-07-06 13:23:51 +02:00
Scott Mitchell
449befa003 Workaround IBM's J9 JVM getSupportedCipherSuites() returning SSL_ prefix cipher names
Motivation:
IBM's J9 JVM utilizes a custom cipher naming scheme with SSL_ prefix [1] instead of the TLS_ prefix defined by TLS RFCs and the JSSE cihper suite names [2]. IBM's documentation says that the SSL_ prefix are "interchangeable" with cipher names with the TLS_ prefix [1]. To work around this issue we parse the supported cipher list and see an SSL_ prefix we can also add the same cipher with the TLS_ prefix. For more details see a discussion on IBM's forums [3] and IBM's issue tracker [4].

[1] https://www.ibm.com/support/knowledgecenter/en/SSYKE2_8.0.0/com.ibm.java.security.component.80.doc/security-component/jsse2Docs/ciphersuites.html
[2] http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites
[3] https://www.ibm.com/developerworks/community/forums/html/topic?id=9b5a56a9-fa46-4031-b33b-df91e28d77c2
[4] https://www.ibm.com/developerworks/rfe/execute?use_case=viewRfe&CR_ID=71770

Modifications:
- When parsing the supported cipher list to get the supported ciphers and we encounter a SSL_ prefix we should also add a TLS_ prefix cipher.
- Remove SSL_ prefix ciphers from Http2SecurityUtil.

Result:
Work around for IBM JVM's custom naming scheme covers more cases for supported cipher suites.
2017-07-05 09:05:42 -04:00
Scott Mitchell
bc46a99eaa DefaultHttp2ConnectionEncoder#writeHeaders shouldn't send GO_AWAY if stream is closed
Motivation:
DefaultHttp2ConnectionEncoder#writeHeaders attempts to find a stream object, and if one doesn't exist it tries to create one. However in the event that the local endpoint has received a RST_STREAM frame before writing the response headers we attempt to create a stream. Since this stream ID is for the incorrect endpoint we then generate a GO_AWAY for what appears to be a protocol error, but can instead be failed locally.

Modifications:
- Just fail the local promise in the above situation instead of sending a GO_AWAY

Result:
Less severe consequences if the server asynchronously sends headers after a RST_STREAM has been received.
Fixes https://github.com/netty/netty/issues/6906.
2017-06-28 12:06:00 -04:00
Scott Mitchell
d3c44ef985 Update Http2SecurityUtil cipher suites
Motivation:
Mozilla's Server Side cipher suite recommendations have been updated [1].

[1] https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility

Modifications:
- Update Http2SecurityUtil to exclude older ciphers.
- Remove support for DHE ciphersuites because they are now Intermediate and BoringSSL dropped support for these ciphers [2]

[2] https://boringssl.googlesource.com/boringssl/+/7e06de5d2d1b53c57c0c81e8d6ba4122b64cf626

Result:
Updated default ciphers for HTTP/2.
2017-06-28 11:40:28 -04:00
Nikolay Fedorovskikh
01eb428b39 Move methods for decode hex dump into StringUtil
Motivation:

PR #6811 introduced a public utility methods to decode hex dump and its parts, but they are not visible from netty-common.

Modifications:

1. Move the `decodeHexByte`, `decodeHexDump` and `decodeHexNibble` methods into `StringUtils`.
2. Apply these methods where applicable.
3. Remove similar methods from other locations (e.g. `HpackHex` test class).

Result:

Less code duplication.
2017-06-23 18:52:42 +02:00
Scott Mitchell
5934ae8fd2 Http2FrameLogger Updates
Motivation:
The Http2FrameLogger uses a custom format when logging events. We should use the more familiar format of 'channel event type: details' and single line logging for more consistent debugging.

Modifications:
- Http2FrameLogger should not use a StringBuilder and instead should directly use the Logger
- Http2FrameLogger should use the more consistent format defined above

Result:
Http2FrameLogger's logging formate is more consistent with other log events.
2017-06-21 17:12:38 -07:00
Nikolay Fedorovskikh
8f8be31226 Remove unnecessary conversions
Motivation:

In a `HttpConversionUtil#toHttp2Headers` a status code conversion can be replaced with using `HttpResponseStatus#codeAsText` method.

Modifications:

Apply `HttpResponseStatus#codeAsText` method.

Result:

Less allocations.
2017-06-21 21:53:48 +02:00
Norman Maurer
575baf5050 Use more aggressive expanding strategy in HpackHuffmanDecoder
Motivation:

Before we always expanded the buffer by the initialCapacity which by default is 32 bytes. This may lead to many expansions of the buffer until we finally reached the point that the buffer can fin everything.

Modifications:

Double the buffer size until the threshold of >= 1024 is hit. After this will grow it by the initialCapacity

Result:

Less expansion of the buffer (and so allocations / copies) when the intialCapacity is not big enough. Fixes [#6864].
2017-06-15 06:56:44 +02:00
Norman Maurer
3f0085c267 Do proper bounds-checking in HpackHuffmanDecoder to reduce overhead of IndexOutOfBoundsException creation
Motivation:

HpackHuffmanDecoder.Decoder did not do any bound-checking but just catched IndexOutOfBoundsException to detect if the array needs to grow. This can be very expensive because of fillInStackTrace()

Modifications:

Add proper bounds checking and grow the array if needed without catching IndexOutOfBoundsException.

Result:

Less overhead if the array needs to grow.
2017-06-13 07:43:29 +02:00
Scott Mitchell
f00638af52 AbstractHttp2ConnectionHandlerBuilder support for HPACK huffman decoder initial size
Motivation:
Depending on the use case it may make sense to increase or decrease the initial size of the buffer used during the HPACK huffman decode process. This is currently not exposed through the AbstractHttp2ConnectionHandlerBuilder.

Modifications:
- Add a method to AbstractHttp2ConnectionHandlerBuilder which allows the initial size of the buffer used during the HPACK huffman decode prcoess to be configured.

Result:
AbstractHttp2ConnectionHandlerBuilder provides more control of codec-http2 knobs.
2017-06-12 16:36:43 -07:00
Norman Maurer
fd67a2354d [maven-release-plugin] prepare for next development iteration 2017-06-08 21:06:24 +02:00
Norman Maurer
3acd5c68ea [maven-release-plugin] prepare release netty-4.1.12.Final 2017-06-08 21:06:01 +02:00
Norman Maurer
0db2901f4d [maven-release-plugin] prepare for next development iteration 2017-05-11 16:00:55 +02:00
Norman Maurer
f7a19d330c [maven-release-plugin] prepare release netty-4.1.11.Final 2017-05-11 16:00:16 +02:00
Nikolay Fedorovskikh
94e9448ae3 Simplify JUnit assertions
Motivation:

Some JUnit assert calls can be replaced by simpler.

Modifications:

Replacement with a more suitable methods.

Result:

More informative JUnit reports.
2017-05-09 20:19:10 +02:00
Moses Nakamura
cf26227c6c Supply a builder for Http2Codec
Motivation:

DefaultHttp2FrameWriter has constructors that it would be a hassle to
expose as configuration parameters on Http2Codec. We should instead
make a builder for Http2Codec.

Modifications:

Get rid of the public constructors on Http2Codec and instead make sure
you can always use the builder where you would have used the constructor
before.

Result:

Http2Codec can be configured more flexibly, and the SensitivityDetector
can be configured.
2017-05-05 09:32:46 -07:00
Norman Maurer
6915ec3bb9 [maven-release-plugin] prepare for next development iteration 2017-04-29 14:10:00 +02:00
Norman Maurer
f30f242fee [maven-release-plugin] prepare release netty-4.1.10.Final 2017-04-29 14:09:32 +02:00
Scott Mitchell
d21f2adb98 HTTP/2 StreamByteDistributor improve parameter validation
Motivation:
Each StreamByteDistributor may allow for priority in different ways, but there are certain characteristics which are invalid regardless of the distribution algorithm. We should validate these invalid characteristics at the flow controller level.

Modifications:
- Disallow negative stream IDs from being used. These streams may be accepted by the WeightedFairQueueByteDistributor and cause state for other valid streams to be evicted.
- Improve unit tests to verify limits are enforced.

Result:
Boundary conditions related to the priority parameters are validated more strictly.
2017-04-24 17:17:18 -07:00
Eric Anderson
799350c369 Fix HTTP/2 dependency tree corruption
Motivation:

Chrome was randomly getting stuck loading the tiles examples.
Investigation showed that the Netty flow controller thought it had
nothing to send for the connection even though some streams has queued
data and window available.

Modifications:

Fixed an accounting error where an implicitly created parent was not
being added to the dependency tree, thus it and all of its children were
orphaned from the connection's tree and would never have data written.

Result:

Fixes #6621
2017-04-22 08:26:47 -07:00
Eric Anderson
c6ad9338b3 Avoid infinite loop in HTTP/2 distributor toString()
Motivation:

Although effectively unused, the toString() of
WeightedFairQueueByteDistributor.State is useful for debugging. It
accidentally had an infinite loop, as it would recurse infinitely
between a parent and its child, which makes it less useful for
debugging.

Modifications:

Prune the infinite loop by using the parent's streamId instead of the
parent's toString().

Result:

Faster, less stack-overflowing toString()
2017-04-22 08:16:41 -07:00
Nikolay Fedorovskikh
0692bf1b6a fix the typos 2017-04-20 04:56:09 +02:00
Scott Mitchell
1bc5bc69e3 HTTP/2 Allow more time for EventLoopGroup to shutdown in test that use LocalChannel
Motivation:
The CI servers have reported leaks while building the HTTP/2 unit tests. The unit tests attempt to wait for the channels to be closed before exiting the test, but we should wait in case there are any tasks pending on the EventLoopGroup's task queues.

Modifications:
- Change the Future.sync() operations to Future.syncUninterruptibly()
- HTTP/2 unit tests which use local channel should wait for 5 seconds before shutting down the EventLoopGroups

Result:
More likely that any cleanup related tasks will execute before the unit tests are shutdown.
2017-04-06 17:56:21 -07:00
Scott Mitchell
225d10e1ad HTTP/2 Make DefaultHttp2HeadersDecoder's Http2Headers object creation extensible
Motivation:
It is generally useful to override DefaultHttp2HeadersDecoder's creation of a new Http2Headers object so more optimized versions can be substituted if the use case allows for it.

Modifications:
- DefaultHttp2HeadersDecoder should support an overridable method to generate the new Http2Headers object for each decode operation

Result:
DefaultHttp2HeadersDecoder is more extensible.
Fixes https://github.com/netty/netty/issues/6591.
2017-04-03 11:19:14 -07:00
Scott Mitchell
e8da5e5162 Revert "Expose HTTP/2 HpackDecoder (#6589)"
This reverts commit f4c635d30b.
2017-04-03 11:19:09 -07:00
Scott Mitchell
36c6a61d33 HTTP/2 remove unnecessary buffer operations
Motivation:
codec-http2 has some helper methods to write to ByteBuf in a big endian fashion. This is the default memory structure for ByteBuf so these helper methods are not necessary.

Modifications:
- remove writeUnsignedInt and writeUnsignedShort

Result:
codec-http2 has less ByteBuf helper methods which are not necessary.
2017-03-31 15:23:39 -07:00
chhsiao90
0ee36fef00 Accept two ways to start HTTP/2 over clear text
Motivation:

HTTP/2 support two ways to start on a no-tls tcp connection,
http/1.1 upgrade and prior knowlege methodology to start HTTP/2.
Currently, the http2-server from example only support
starting by upgrade. I think we can do a simple dispatch by peek first
bytes from inbound that match to prior knowledge preface or not and
determine which handlers to set into pipeline.

Modifications:

Add ClearTextHttp2ServerUpgradeHandler to support start HTTP/2 via clear
text with two approach. And update example/http2-server to support
this functionality.

Result:

netty HTTP/2 and the example http2-server accept for two ways to start
HTTP/2 over clear text.

Fixed memory leak problem

Update fields to final

Rename ClearText to cleartext

Addressed comments for code improvement

- Always prefer static, final, and private if possible
- Add UnstableApi annotation
- Used EmbeddedChannel.readInbound instead of unhandled inbound handler
- More assertion

Update javadoc for CleartextHttp2ServerUpgradeHandler

Rename ClearTextHttp2ServerUpgradeHandler to CleartextHttp2ServerUpgradeHandler

Removed redundant code about configure pipeline

nit: PriorKnowledgeHandler

Removed Mockito.spy, investigate conn state instead

Add Http2UpgradeEvent

Check null of the constructor arguments

Rename Http2UpgradeEvent to PriorKnowledgeUpgradeEvent

Update unit test
2017-03-31 15:21:48 -07:00
Nathan Mittler
f4c635d30b Expose HTTP/2 HpackDecoder (#6589)
Motivation:

gRPC (and potentially other libraries) has an optimized header processor that requires direct access to the HpackDecoder.

Modifications:

Make the HpackDecoder and its constructors public.

Result:

Fixes #6579
2017-03-31 10:51:01 -07:00
Norman Maurer
40bead56c4 Revert "http2: Http2StreamChannel now shares options of its parent channel"
This reverts commit 7467106630.
2017-03-31 07:45:56 +02:00
Vladimir Kostyukov
7467106630 http2: Http2StreamChannel now shares options of its parent channel
Motivation

Http2StreamChannel ignores options of its parent channel when being created. That leads to surprising results when, for example, unpooled allocator could be silently replaced with pooled allocator (default setting).

Modification

Copy parent channel's options over to the Http2StreamChannel.

Result

Channel options are now consistent between Http2StreamChannel and its parent channel. Newly added test passes on this branch and fails on master. Fixes #6551.
2017-03-23 21:25:54 -07:00
Norman Maurer
2b8c8e0805 [maven-release-plugin] prepare for next development iteration 2017-03-10 07:46:17 +01:00
Norman Maurer
1db58ea980 [maven-release-plugin] prepare release netty-4.1.9.Final 2017-03-10 07:45:28 +01:00
Norman Maurer
e12f504ac1 Remove deprecated usage of Mockito methods
Motivation:

We used some deprecated Mockito methods.

Modifications:

- Replace deprecated method usage
- Some cleanup

Result:

No more usage of deprecated Mockito methods. Fixes [#6482].
2017-03-09 20:59:54 +01:00
Scott Mitchell
01012fc5b7 HTTP/2 SETTINGS ACK sequencing issue
Motivation:
DefaultHttp2ConnectionDecoder#onSettingsRead processes the settings, and then sends a SETTINGS ACK to the remote peer. Processing the settings may result in frames which violate the previous settings being send to the remote peer. The remote peer will not apply the new settings until it has received the SETTINGS ACK, and therefore we may violate the settings from the remote peer's perspective and the connection will be shutdown.

Modifications:
- We should send the SETTINGS ACK before we process the settings to ensure the peer receives the SETTINGS ACK before other frames which assume the settings have already been applied

Result:
Fixes https://github.com/netty/netty/issues/6520.
2017-03-09 10:21:55 -08:00
Fabian Lange
a94b23df7d Support SSL_ prefixed cipher suites in addition to TLS_ prefixed ones.
Motivation:
Http2SecurityUtil currently lists HTTP/2 ciphers as documented by
JSSE docs [1] and the IANA [2] using the TLS_ prefix.
In some IBM J9 implementations the SSL_ prefix is used, which is also
covered by the JSSE.

[1] http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
[2] http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml

Modifications:
Add both variants of the cipher names (prefixed with SSL_ in additon to TLS_)

Result:
HTTP/2 connections can now be created using the SslProvider.JDK on IBM J9
and potentially other JVMs which use the SSL_ prefix.
2017-03-09 09:26:33 +01:00
Nikolay Fedorovskikh
2993760e92 Fix misordered 'assertEquals' arguments in tests
Motivation:

Wrong argument order in some 'assertEquals' applying.

Modifications:

Flip compared arguments.

Result:

Correct `assertEquals` usage.
2017-03-08 22:48:37 -08:00
Scott Mitchell
e18c85b768 Add missing methods to Http2ConnectionHandlerBuilder
Motivation:
A previous commit added methods to AbstractHttp2ConnectionHandlerBuilder but forgot to expose them in Http2ConnectionHandlerBuilder.

Modifications:
- expose the new methods in Http2ConnectionHandlerBuilder

Result:
Http2ConnectionHandlerBuilder supports the new configuration options.
2017-03-07 20:34:16 -08:00
Scott Mitchell
f9001b9fc0 HTTP/2 move internal HPACK classes to the http2 package
Motivation:
The internal.hpack classes are no longer exposed in our public APIs and can be made package private in the http2 package.

Modifications:
- Make the hpack classes package private in the http2 package

Result:
Less APIs exposed as public.
2017-03-02 07:42:41 -08:00
Christopher Exell
52aecabe43 Allow GOAWAY to be sent from handlers after the Http2MultiplexCodec so that app developers can shed load by issuing GOAWAY 2017-03-01 19:22:35 -08:00
Norman Maurer
7aff6b0330 Increase timeouts in Http2ConnectionRoundtripTest
Motivation:

The timeouts used in the Http2ConnectionRoundtripTest seems to be too low when leak-detection is enabled so we sometimes get failed tests due timeout.

Modifications:

Increase timeouts.

Result:

Fixes [#6442].
2017-02-24 07:58:59 +01:00
Nikolay Fedorovskikh
0623c6c533 Fix javadoc issues
Motivation:

Invalid javadoc in project

Modifications:

Fix it

Result:

More correct javadoc
2017-02-22 07:31:07 +01:00
chhsiao90
e148b53c67 Add unit test for DefaultHttp2FrameWriter
Motivation:

DefaultHttp2FrameWriter contains important logic for how http2
frame encode into binary, currently, netty had no unit test
just for DefaultHttp2FrameWriter.

Modifications:

Write DefaultHttp2FrameWriterTest to test DefaultHttp2FrameWriter

Result:

The coverage of DefaultHttp2FrameWriter is increased, and the
class was more reliable
2017-02-21 09:57:16 +01:00
chhsiao90
b1436e80ef Cleanup DefaultHttp2FrameReader about verifyUnknownFrame
Motivation:

In previous PR about handling unknwon frame in the middle of header
block, I didn't notice and re-use about checking is processing header
. And I added a redundant method for same functionality.
I think that the redundant method would lead to some misleading
situation.

Modifications:

Removed redundant code on DefaultHttp2FrameReader

Result:

The code is more readable
2017-02-20 22:12:20 -08:00
Scott Mitchell
08e0c612cf HTTP/2 Unit Test LocalChannel Leaks
Motivation:
Some unit HTTP/2 unit tests use LocalChannel. LocalChannel's doClose method will ensure any pending items in the queue will be released, but it may execute a Runnable on the peer's EventLoop to ensure the peer's queue is also cleaned up. The HTTP/2 unit tests close the event loop groups with no wait time so that unit tests will execute quickly, but if the doClose Runnable is in the EventLoop's queue it will not run and thus the items in the queue will not be released.

Modifications:
- Ensure all HTTP/2 unit tests which use LocalChannel wait for both client and server channels to be closed before closing the EventLoop.

Result:
Related to https://github.com/netty/netty/issues/5850.
2017-02-20 13:54:30 -08:00
fenik17
0cf3f54a8d Adding 'final' keyword for private fields where possible
Motivation

Missing 'final' keyword for fields

Modifications

Add 'final' for fields where possible

Result

More safe and consistent code
2017-02-14 08:29:15 +01:00
chhsiao90
72916b9960 Add unit test on DefaultHttp2FrameReader
Motivation:

DefaultHttp2FrameReader contains the logic for how it parsed the network
traffic from http2 client,
it also validate the content is legal or not.
So keep high coverage rate on it will increase the stability of api.

Modifications:

Add unit test on DefaultHttp2FrameReader

Result:

Coverage rate increased
2017-02-06 23:57:03 -08:00
Norman Maurer
a7c0ff665c Only use Mockito for mocking.
Motivation:

We used various mocking frameworks. We should only use one...

Modifications:

Make usage of mocking framework consistent by only using Mockito.

Result:

Less dependencies and more consistent mocking usage.
2017-02-07 08:47:22 +01:00
Dmitriy Dumanskiy
b9abd3c9fc Cleanup : for loops for arrays to make code easier to read and removed unnecessary toLowerCase() 2017-02-06 07:47:59 +01:00
Scott Mitchell
3482651e0c HTTP/2 Non Active Stream RFC Corrections
Motivation:
codec-http2 couples the dependency tree state with the remainder of the stream state (Http2Stream). This makes implementing constraints where stream state and dependency tree state diverge in the RFC challenging. For example the RFC recommends retaining dependency tree state after a stream transitions to closed [1]. Dependency tree state can be exchanged on streams in IDLE. In practice clients may use stream IDs for the purpose of establishing QoS classes and therefore retaining this dependency tree state can be important to client perceived performance. It is difficult to limit the total amount of state we retain when stream state and dependency tree state is combined.

Modifications:
- Remove dependency tree, priority, and weight related items from public facing Http2Connection and Http2Stream APIs. This information is optional to track and depends on the flow controller implementation.
- Move all dependency tree, priority, and weight related code from DefaultHttp2Connection to WeightedFairQueueByteDistributor. This is currently the only place which cares about priority. We can pull out the dependency tree related code in the future if it is generally useful to expose for other implementations.
- DefaultHttp2Connection should explicitly limit the number of reserved streams now that IDLE streams are no longer created.

Result:
More compliant with the HTTP/2 RFC.
Fixes https://github.com/netty/netty/issues/6206.

[1] https://tools.ietf.org/html/rfc7540#section-5.3.4
2017-02-01 10:34:27 -08:00
Scott Mitchell
6e5b25733f HTTP/2 Connection Preface User Event
Motivation:
If an HTTP/2 client writes data before the connection preface the peer will shutdown the socket. Depending on what is in the pipeline (SslHandler) may require different evaluation criteria to infer when the codec-http2 has written the connection preface on behalf of the client. This can lead to unnecessarily complexity and error prone/racy application code.

Modifications:
- Introduce a user event that is fired up the pipeline when codec-http2 writes the connection preface

Result:
Reliable mechanism for applications to use to know when connection preface has been written (related to https://github.com/netty/netty/issues/6272).
2017-02-01 10:10:42 -08:00
Carl Mastrangelo
ead9938980 Include Http 1 request in error message
Motivation:

When An HTTP server is listening in plaintext mode, it doesn't have
a chance to negotiate "h2" in the tls handshake.  HTTP 1 clients
that are not expecting an HTTP2 server will accidentally a request
that isn't an upgrade, which the HTTP/2 decoder will not
understand.  The decoder treats the bytes as hex and adds them to
the error message.

These error messages are hard to understand by humans, and result
in extra, manual work to decode.

Modification:

If the first bytes of the request are not the preface, the decoder
will now see if they are an HTTP/1 request first.  If so, the error
message will include the method and path of the original request in
the error message.

In case the path is long, the decoder will check up to the first
1024 bytes to see if it matches.  This could be a DoS vector if
tons of bad requests or other garbage come in.  A future optimization
would be to treat the first few bytes as an AsciiString and not do
any Charset decoding.  ByteBuf.toCharSequence alludes to such an
optimization.

The code has been left simple for the time being.

Result:

Faster identification of errant HTTP requests.
2017-01-30 09:46:38 -08:00
Norman Maurer
735d6dd636 [maven-release-plugin] prepare for next development iteration 2017-01-30 15:14:02 +01:00
Norman Maurer
76e22e63f3 [maven-release-plugin] prepare release netty-4.1.8.Final 2017-01-30 15:12:36 +01:00
Scott Mitchell
e13da218e9 HTTP/2 revert Http2FrameWriter throws API change
Motivation:
2fd42cfc6b fixed a bug related to encoding headers but it also introduced a throws statement onto the Http2FrameWriter methods which write headers. This throws statement makes the API more verbose and is not necessary because we can communicate the failure in the ChannelFuture that is returned by these methods.

Modifications:
- Remove throws from all Http2FrameWriter methods.

Result:
Http2FrameWriter APIs do not propagate checked exceptions.
2017-01-26 23:26:17 -08:00
chhsiao90
f4c2c1926f Fixed rfc violation about sending extension frame in the middle of headers
Motivation:

At rfc7540 5.5, it said that it's not permitted to send extension
frame in the middle of header block and need be treated as
protocol error

Modifications:

When received a extension frame, in netty it's called unknown frame,
will verify that is there an headersContinuation exists

Result:

When received a extension frame in the middle of header block,
will throw connection error and closed the connection
2017-01-26 11:25:15 -08:00
Christopher Exell
d509365974 Move location of where oversized headers that don't exceed the go away limit is done
so that the check occurs after all headers have been read
2017-01-24 15:19:46 -08:00
Christopher Exell
907726988d Update hpack Decoder CTOR to allow for overflow in maxHeaderList size, as we do when we apply our ack'ed settings
This prevents us from having the first request, that hasn't ack'ed the setting causing a GOAWAY when we'd would
be under the maxHeaderListSizeGoAway that would have been set after the settings ack.
2017-01-21 10:20:49 -08:00
Scott Mitchell
2fd42cfc6b HTTP/2 Max Header List Size Bug
Motivation:
If the HPACK Decoder detects that SETTINGS_MAX_HEADER_LIST_SIZE has been violated it aborts immediately and sends a RST_STREAM frame for what ever stream caused the issue. Because HPACK is stateful this means that the HPACK state may become out of sync between peers, and the issue won't be detected until the next headers frame. We should make a best effort to keep processing to keep the HPACK state in sync with our peer, or completely close the connection.
If the HPACK Encoder is configured to verify SETTINGS_MAX_HEADER_LIST_SIZE it checks the limit and encodes at the same time. This may result in modifying the HPACK local state but not sending the headers to the peer if SETTINGS_MAX_HEADER_LIST_SIZE is violated. This will also lead to an inconsistency in HPACK state that will be flagged at some later time.

Modifications:
- HPACK Decoder now has 2 levels of limits related to SETTINGS_MAX_HEADER_LIST_SIZE. The first will attempt to keep processing data and send a RST_STREAM after all data is processed. The second will send a GO_AWAY and close the entire connection.
- When the HPACK Encoder enforces SETTINGS_MAX_HEADER_LIST_SIZE it should not modify the HPACK state until the size has been checked.
- https://tools.ietf.org/html/rfc7540#section-6.5.2 states that the initial value of SETTINGS_MAX_HEADER_LIST_SIZE is "unlimited". We currently use 8k as a limit. We should honor the specifications default value so we don't unintentionally close a connection before the remote peer is aware of the local settings.
- Remove unnecessary object allocation in DefaultHttp2HeadersDecoder and DefaultHttp2HeadersEncoder.

Result:
Fixes https://github.com/netty/netty/issues/6209.
2017-01-19 10:42:43 -08:00
Scott Mitchell
b701da8d1c HTTP/2 HPACK Integer Encoding Bugs
Motivation:
- Decoder#decodeULE128 has a bounds bug and cannot decode Integer.MAX_VALUE
- Decoder#decodeULE128 doesn't support values greater than can be represented with Java's int data type. This is a problem because there are cases that require at least unsigned 32 bits (max header table size).
- Decoder#decodeULE128 treats overflowing the data type and invalid input the same. This can be misleading when inspecting the error that is thrown.
- Encoder#encodeInteger doesn't support values greater than can be represented with Java's int data type. This is a problem because there are cases that require at least unsigned 32 bits (max header table size).

Modifications:
- Correct the above issues and add unit tests.

Result:
Fixes https://github.com/netty/netty/issues/6210.
2017-01-18 18:36:47 -08:00
Scott Mitchell
c590e3bd63 HTTP/2 relax test timeouts
Motivation:
Build failures have been observed with 2 second timeouts on the CI servers. We should make the timeouts longer to reduce false positive test failures due to tests timing out prematurely.

Modifications:
- Increase timeouts from 2 and 3 seconds to 5 seconds.

Result:
Less false positive test failures.
2017-01-18 08:02:15 +01:00
Norman Maurer
7f01da8d0f [maven-release-plugin] prepare for next development iteration 2017-01-12 11:36:51 +01:00
Norman Maurer
7a21eb1178 [maven-release-plugin] prepare release netty-4.1.7.Final 2017-01-12 11:35:58 +01:00
Scott Mitchell
ec3d077e0d DefaultHttp2Connection modifying child map while iterating
Motivation:
When DefaultHttp2Connection removes a stream it iterates over all children and adds them as children to the parent of the stream being removed. This process may remove elements from the child map while iterating without using the iterator's remove() method. This is generally unsafe and may result in an undefined iteration.

Modifications:
- We should use the Iterator's remove() method while iterating over the child map

Result:
Fixes https://github.com/netty/netty/issues/6163
2017-01-11 11:07:18 -08:00
Scott Mitchell
06e7627b5f Read Only Http2Headers
Motivation:
A read only implementation of Http2Headers can allow for a more efficient usage of memory and more performant combined construction and iteration during serialization.

Modifications:
- Add a new ReadOnlyHttp2Headers class

Result:
ReadOnlyHttp2Headers exists and can be used for performance reasons when appropriate.

```
Benchmark                                            (headerCount)  Mode  Cnt    Score   Error  Units
ReadOnlyHttp2HeadersBenchmark.defaultClientHeaders               1  avgt   20   96.156 ± 1.902  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultClientHeaders               5  avgt   20  157.925 ± 3.847  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultClientHeaders              10  avgt   20  236.257 ± 2.663  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultClientHeaders              20  avgt   20  392.861 ± 3.932  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultServerHeaders               1  avgt   20   48.759 ± 0.466  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultServerHeaders               5  avgt   20  113.122 ± 0.948  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultServerHeaders              10  avgt   20  192.698 ± 1.936  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultServerHeaders              20  avgt   20  348.974 ± 3.111  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultTrailers                    1  avgt   20   35.694 ± 0.271  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultTrailers                    5  avgt   20   98.993 ± 2.933  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultTrailers                   10  avgt   20  171.035 ± 5.068  ns/op
ReadOnlyHttp2HeadersBenchmark.defaultTrailers                   20  avgt   20  330.621 ± 3.381  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyClientHeaders              1  avgt   20   40.573 ± 0.474  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyClientHeaders              5  avgt   20   56.516 ± 0.660  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyClientHeaders             10  avgt   20   76.890 ± 0.776  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyClientHeaders             20  avgt   20  117.531 ± 1.393  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyServerHeaders              1  avgt   20   29.206 ± 0.264  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyServerHeaders              5  avgt   20   44.587 ± 0.312  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyServerHeaders             10  avgt   20   64.458 ± 1.169  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyServerHeaders             20  avgt   20  107.179 ± 0.881  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyTrailers                   1  avgt   20   21.563 ± 0.202  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyTrailers                   5  avgt   20   41.019 ± 0.440  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyTrailers                  10  avgt   20   64.053 ± 0.785  ns/op
ReadOnlyHttp2HeadersBenchmark.readOnlyTrailers                  20  avgt   20  113.737 ± 4.433  ns/op
```
2016-12-18 09:32:24 -08:00
Nikolaj Hald Nielsen
cd458f10bc Server returns status 431 on header size errors
Motivation:

Currently clients attempting to send headers that are too large recieve
a RST frame. This makes it harder than needed for implementations on top
of netty to handle this in a graceful way.

Modifications:

When the Decoder throws a StreamError of type FRAME_SIZE_ERROR, the
Http2ConnectionHandler will now attempt to send an Http2Header with
status 431 and endOfStream=true

Result:

Implementations now do not have to subclass parts of netty to handle
431s
2016-12-15 18:06:24 -08:00
Scott Mitchell
4639d56596 HttpToHttp2ConnectionHandlerTest increase setup timeout
Motivation:
The 2 second timeout to bootstrap the test can timeout on the build servers. We should increase the timeout so it is less likely under powered or over worked machines are less likely to generate false failures.

Modifications:
- HttpToHttp2ConnectionHandlerTest setup timeout changed from 2 to 5 seconds

Result:
Less false build failures.
2016-12-08 10:51:42 -08:00
Stephane Landelle
ba95c401a7 Misc clean up
Motivation:
IntelliJ issues several warnings.

Modifications:

* `ClientCookieDecoder` and `ServerCookieDecoder`:
  * `nameEnd`, `valueBegin` and `valueEnd` don't need to be initialized
  * `keyValLoop` loop doesn't been to be labelled, as it's the most inner one (same thing for labelled breaks)
  * Remove `if (i != headerLen)` as condition is always true
* `ClientCookieEncoder` javadoc still mention old logic
* `DefaultCookie`, `ServerCookieEncoder` and `DefaultHttpHeaders` use ternary ops that can be turned into simple boolean ones
* `DefaultHeaders` uses a for(int) loop over an array. It can be turned into a foreach one as javac doesn't allocate an iterator to iterate over arrays
* `DefaultHttp2Headers` and `AbstractByteBuf` `equal` can be turned into a single boolean statement
Result:

Cleaner code
2016-11-22 15:17:05 -08:00
Norman Maurer
e5070b7266 [#6023] Ensure LastInboundHandler correctly handle different events / data
Motivation:

LastInboundHandler maintains 2 queues which may contain the same data and tries to match these up when you read elements out of it. Because of this it can happen that you remove an element only out of one queue and so double free stuff later.

Modifications:

Just use one "queue" to store things.

Result:

Not possible to only remove things from one queue and so get into trouble later when release everything that sits in the handler.
2016-11-21 07:49:13 +01:00
Scott Mitchell
782b7bcf4a Fix HTTP/2 test cleanup with LocalChannel
Motivation:
When a LocalChannel is closed it is responsible to ensure all queued objects are released. When a LocalChannel is closed it will also close its peer channel. However in HTTP/2 unit tests we may not wait until all channels have completed the shutdown process before destroying the threads and exiting the test. This may mean buffers are GCed before they are released and be reported as a leak.

Modifications:
- In HTTP/2 tests when we use LocalChannel we should wait for all channels to close before exiting the test and cleaning up the associated EventLoopGroups.

Result:
More correct usage of LocalChannel in HTTP/2 tests.
2016-11-19 09:25:52 -08:00
Norman Maurer
0bc30a123e Eliminate usage of releaseLater(...) to reduce memory usage during tests
Motiviation:

We used ReferenceCountUtil.releaseLater(...) in our tests which simplifies a bit the releasing of ReferenceCounted objects. The problem with this is that while it simplifies stuff it increase memory usage a lot as memory may not be freed up in a timely manner.

Modifications:

- Deprecate releaseLater(...)
- Remove usage of releaseLater(...) in tests.

Result:

Less memory needed to build netty while running the tests.
2016-11-18 09:34:11 +01:00
Scott Mitchell
c4e96d010e HTTP/2 WeightedFairQueueByteDistributor Bug
Motivation:
If a stream is not able to send any data (flow control window for the stream is exhausted) but has descendants who can send data then WeightedFairQueueByteDistributor may incorrectly modify the pseudo time and also double add the associated state to the parent's priority queue. The pseudo time should only be modified if a node is moved in the priority tree, and not if there happens to be no active streams in its descendent tree and a descendent is moved (e.g. removed from the tree because it wrote all data and the last data frame was EOS). Also the state objects for WeightedFairQueueByteDistributor should only appear once in any queue. If this condition is violated the pseudo time accounting would be biased at and assumptions in WeightedFairQueueByteDistributor would be invalidated.

Modifications:
- WeightedFairQueueByteDistributor#isActiveCountChangeForTree should not allow re-adding to the priority queue if we are currently processing a node in the distribution algorithm. The distribution algorithm will re-evaluate if the node should be re-added on the tail end of the recursion.

Result:
Fixes https://github.com/netty/netty/issues/5980
2016-11-14 16:40:13 -08:00
Nikolaj Hald Nielsen
5b5f6a6031 Allow non-default Http/2 server initial settings
Motivation:

Currently it is not possible to have an Http/2 server send non default
initial settings to clients when doing the initial connection handshake

Modifications:

Add additional constructors to Http2Codec allowing users to specify the
initial settings to send to the client and apply locally

Result:

You can now specify non default initial settings
2016-11-09 08:31:26 +01:00
Scott Mitchell
efca9d180e HTTP/2 HPACK Encoder remove unsued import 2016-10-17 09:25:49 -07:00
buchgr
c9918de37b http2: Make MAX_HEADER_LIST_SIZE exceeded a stream error when encoding.
Motivation:

The SETTINGS_MAX_HEADER_LIST_SIZE limit, as enforced by the HPACK Encoder, should be a stream error and not apply to the whole connection.

Modifications:

Made the necessary changes for the exception to be of type StreamException.

Result:

A HEADERS frame exceeding the limit, only affects a specific stream.
2016-10-17 09:24:06 -07:00
Norman Maurer
5f533b7358 [maven-release-plugin] prepare for next development iteration 2016-10-14 13:20:41 +02:00
Norman Maurer
35fb0babe2 [maven-release-plugin] prepare release netty-4.1.6.Final 2016-10-14 12:47:19 +02:00
buchgr
af8ef3e40c Correctly release debugData and notify the promise when goAway was already sent
Motivation:

Commit 908464f161 also introduced a change to guard against re-entrance but failed to correctly handle the debugData and promise.

Modifications:

Release debugData and correctly notify the promise.

Result:

No more buffer leak and promise is always notified.
2016-10-14 10:57:18 +02:00
Norman Maurer
be8b6c6f24 Correctly add STREAM_WEIGHT header in InboundHttp2ToHttpAdapter.onPushPromiseRead(...)
Motivation:

The weight header with the default value is not set but it should be (rfc7540#5.3.5: …Pushed streams initially depend on their associated stream … are assigned a default weight of 16).

Modifications:

Add STREAM_WEIGHT header.

Result:

Correctly add headers.
2016-10-10 10:38:35 +02:00
Scott Mitchell
540c26bb56 HTTP/2 Ensure default settings are correctly enforced and interfaces clarified
Motivation:
The responsibility for retaining the settings values and enforcing the settings constraints is spread out in different areas of the code and may be initialized with different values than the default specified in the RFC. This should not be allowed by default and interfaces which are responsible for maintaining/enforcing settings state should clearly indicate the restrictions that they should only be set by the codec upon receipt of a SETTINGS ACK frame.

Modifications:
- Encoder, Decoder, and the Headers Encoder/Decoder no longer expose public constructors that allow the default settings to be changed.
- Http2HeadersDecoder#maxHeaderSize() exists to provide some bound when headers/continuation frames are being aggregated. However this is roughly the same as SETTINGS_MAX_HEADER_LIST_SIZE (besides the 32 byte octet for each header field) and can be used instead of attempting to keep the two independent values in sync.
- Encoding headers now enforces SETTINGS_MAX_HEADER_LIST_SIZE at the octect level. Previously the header encoder compared the number of header key/value pairs against SETTINGS_MAX_HEADER_LIST_SIZE instead of the number of octets (plus 32 bytes overhead).
- DefaultHttp2ConnectionDecoder#onData calls shouldIgnoreHeadersOrDataFrame but may swallow exceptions from this method. This means a STREAM_RST frame may not be sent when it should for an unknown stream and thus violate the RFC. The exception is no longer swallowed.

Result:
Default settings state is enforced and interfaces related to settings state are clarified.
2016-10-07 13:00:45 -07:00
buchgr
f010033590 h2childchan: Ability to open outbound/local streams. Fixes #4913
Motivation:

The HTTP/2 child channel API does not allow to create local/outbound HTTP/2 streams.

Modifications:

Add a Http2StreamChannelBootstrap that allows to create outbound streams.

Result:

The HTTP/2 child channel API now supports outbound streams.
2016-10-06 16:12:19 -07:00
Norman Maurer
d1d954da35 [#5866] HttpToHttp2ConnectionHandler / InboundHttp2ToHttpAdapter ignores STREAM_DEPENDENCY_ID and STREAM_WEIGHT.
Motivation:

HttpToHttp2ConnectionHandler.write(ctx, msg, promise) ignores HttpConversionUtil.ExtensionHeaderNames.STREAM_DEPENDENCY_ID header in outbound message. Beside this InboundHttp2ToHttpAdapter also not add the STREAM_DEPENDENCY_ID and STREAM_WEIGHT headers.

Modifications:

Respect STREAM_DEPENDENCY_ID and STREAM_WEIGHT

Result:

Correctly respect STREAM_DEPENDENCY_ID and STREAM_WEIGHT.
2016-10-03 18:05:50 +02:00
radai-rosenblatt
15ac6c4a1f Clean-up unused imports
Motivation:

the build doesnt seem to enforce this, so they piled up

Modifications:

removed unused import lines

Result:

less unused imports

Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
2016-09-30 09:08:50 +02:00
Norman Maurer
3240e97420 [#5862] Http2EventAdapter#onUnknownFrame(...) should throw Http2Exception
Motivation:

Http2EventAdapter implements the Http2FrameListener interface but implements the #onUnknownFrame(...) method without the interface's throws Http2Exception.

Modifications:

Add throws Http2Exception.

Result:

More correct method signature.
2016-09-29 14:22:13 +02:00
buchgr
6a6d422702 Complete documentation of StreamBufferingEncoder.
Motivation:

The StreamBufferingEncoder is missing documentation of what happens
to buffered frames when it's closed.

Modifications:

Added this missing piece of documentation.

Result:

Improved documentation.
2016-09-23 16:39:31 -07:00
Moses Nakamura
0d2baaf6ae codec-http2: Let users configure per-frame logs
Motivation:

codec-http2 is really loud!

Modification:

Allow users to select how to log in the Http2Codec.

Result:

We can run Http2Codec and log however we like.
2016-09-23 14:58:23 -07:00
Scott Mitchell
4145fae919 HTTP/2 HPACK Decoder VarInt Improvement
Motivation:
HTTP/2 Decoder#decodeULE128 current will tolerate more bytes than necessary when attempted to detect overflow. The usage of this method also currently requires an additional overflow conditional.

Modifications:
- Integrate the first byte into Decoder#decodeULE128 which allows us to detect overflow reliably and avoid overflow checks outside of this method.

Result:
Less conditionals and earlier overflow detection in Decoder#decodeULE128
2016-09-22 00:01:54 -07:00
Norman Maurer
3c62a20519 Revert "Ensure we only call debugData.release() if we called debugData.retain()"
This reverts commit 4beb075dd3.
2016-09-16 08:51:11 -07:00
Moses Nakamura
da8734a6f9 codec-http2: Mark requests as chunked in Http2ServerDowngrader
Motivation:

Http2ServerDowngrader doesn't mark chunked requests as chunked, even
though the natural conversion from http/2 requests to http/1.1 requests
is to chunked ones.

Modifications:

Mark requests that aren't already complete as chunked.

Result:

Requests will be chunked, and can later be aggregated if necessary.
2016-09-15 15:58:40 -07:00
buchgr
908464f161 make the http2 codec void promise ready.
Motivation:

Void promises need special treatment, as they don't behave like normal promises. One
cannot add a listener to a void promise for example.

Modifications:

Inspected the code for addListener() calls, and added extra logic for void promises
where necessary. Essentially, when writing a frame with a void promise, any errors
are reported via the channel pipeline's exceptionCaught event.

Result:

The HTTP/2 codec has better support for void promises.
2016-09-15 09:19:57 -07:00
Norman Maurer
4beb075dd3 Ensure we only call debugData.release() if we called debugData.retain()
Motivation:

We need to ensure we only call debugData.release() if we called debugData.retain(), otherwise we my see an IllegalReferenceCountException.

Modifications:

Only call release() if we called retain().

Result:

No more IllegalReferenceCountException possible.
2016-09-12 06:47:38 -07:00
Norman Maurer
51629245d0 Call debugData.retain() before we forward the frame to the pipeline
Motivation:

We need to call debugData.retain() before we forward the frame to the pipeline as ByteToMessageDecoder will call release() on the buffer.

Modifications:

Correctly retain debugData and fix the unit test to test for it.

Result:

No more IllegalReferenceCountException when using the Http2FrameCodec.
2016-09-12 06:45:13 -07:00
Norman Maurer
d2389a9339 [#5762] HTTP/2: SETTINGS_HEADER_TABLE_SIZE should be an unsigned int
Motivation:

he HTTP/2 spec demands that the max value for SETTINGS_HEADER_TABLE_SIZE should be an unsigned 32-bit integer.

Modifications:

Change the limit to unsigned 32-bit integer and add tests.

Result:

Complient to rfc.
2016-09-09 13:20:32 +02:00
Norman Maurer
54b1a100f4 [maven-release-plugin] prepare for next development iteration 2016-08-26 10:06:32 +02:00
buchgr
515f8964b4 HTTP/2: Fix some errors reported by h2spec.
Motivation:

h2spec [1] reported 32 issues [2] with Netty's HTTP/2 implementation.

Modifications:

Fixed 11 of those issues. Mostly wrong error codes or added missing validation
code in the frame reader.

Result:

11 fewer errors. h2spec now reports: 70 tests, 48 passed, 1 skipped, 21 failed

[1] https://github.com/summerwind/h2spec
[2] https://github.com/netty/netty/issues/5761
2016-09-01 08:28:16 +02:00
Norman Maurer
1208b90f57 [maven-release-plugin] prepare release netty-4.1.5.Final 2016-08-26 04:59:35 +02:00
Trustin Lee
5b46cf25c1 Fulfill the write promise of empty HTTP/2 DATA frames sooner
Motivation:

DefaultHttp2ConnectionEncoder.FlowControlledData.write() does not
complete the write promise of empty HTTP/2 DATA frames until either a
non-DATA frame, a non-empty DATA frame or a DATA frame with endOfStream
set. This makes the write promise of the empty DATA frame is notified
much later than it could be.

Modifications:

- Notify the write promise of the empty DATA frames immediately is the
  queue contains empty DATA frames only

Result:

The write promise of an empty DATA frame is notified sooner.
2016-08-26 08:45:09 +09:00
Scott Mitchell
208893aac9 HTTP/2 Hpack Encoder Cleanup
Motivation:
The HTTP/2 HPACK Encoder class has some code which is only used for test purposes. This code can be removed to reduce complexity and member variable count.

Modifications:
- Remove test code and update unit tests
- Other minor cleanup

Result:
Test code is removed from operational code.
2016-08-25 09:08:46 -07:00
Norman Maurer
5fd239c29c Ensure we not log missleading errors if the promise was already failed due errors
Motivation:

In DefaultHttp2ConnectionEncoder we fail the promise in in the FlowControlledData.error(...) method but also add it the CoalescingBufferQueue. Which can lead to have the promise failed by error(...) before it can be failed in CoalescingBufferQueue.

This can lead to confusing and missleading errors in the log like:
    2016-08-12 09:47:43,716       nettyIoExecutorGroup-1-9 [WARN ] PromiseNotifier                - Failed to mark a promise as failure because it's done already: DefaultChannelPromise@374225e0(failure: javax.net.ssl.SSLException: SSLEngine closed already)
    javax.net.ssl.SSLException: SSLEngine closed already
        at io.netty.handler.ssl.SslHandler.wrap(...)(Unknown Source) ~[netty-all-4.1.5.Final-SNAPSHOT.jar:?]

Modifications:

Ensure we only fail the queue (which will also fail the promise).

Result:

No more missleading logs.
2016-08-18 07:17:45 +02:00
buchgr
d568cfc14a HTTP/2: Treat MAX_CONCURRENT_STREAMS exceeded as a stream error.
Motivation:

As per the HTTP/2 spec, exceeding the MAX_CONCURRENT_STREAMS should be treated as a stream error as opposed to a connection error.

"An endpoint that receives a HEADERS frame that causes its advertised concurrent stream limit to be exceeded MUST treat this as a stream error (Section 5.4.2) of type PROTOCOL_ERROR or REFUSED_STREAM." http://httpwg.org/specs/rfc7540.html#rfc.section.5.1.2

Modifications:

Make the error a stream error.

Result:

It's a stream error.
2016-08-17 14:47:15 +02:00
Scott Mitchell
f73c4f24ee HTTP/2 HPACK Bounds Check Fix
Motivation:
21e8d84b79 changed the way bounds checking was done, but however a bounds check in the case of READ_LITERAL_HEADER_NAME_LENGTH_PREFIX was using an old value. This would delay when the bounds check would actually be done and potentially allow more allocation than necessary.

Modifications:
- Use the new length (index) in the bounds check instead of an old length (nameLength) which had not yet been assigned to the new value.

Result:
More correct bounds checking.
2016-08-13 11:04:26 -07:00
Scott Mitchell
21e8d84b79 HTTP/2 Simplify Headers Decode Bounds Checking
Motivation:
The HPACK decoder keeps state so that the decode method can be called multiple times with successive header fragments. This decoder also requires that a method is called to signify the decoding is complete. At this point status is returned to indicate if the max header size has been violated. Netty always accumulates the header fragments into a single buffer before attempting to HPACK decode process and so keeping state and delaying notification that bounds have been exceeded is not necessary.

Modifications:
- HPACK Decoder#decode(..) now must be called with a complete header block
- HPACK will terminate immediately if the maximum header length, or maximum number of headers is exceeded
- Reduce member variables in the HPACK Decoder class because they can now live in the decode(..) method

Result:
HPACK bounds checking is done earlier and less class state is needed.
2016-08-12 17:12:53 -07:00
Scott Mitchell
4d74bf3984 HTTP/2 MaxStreams cleanup
Motivation:
765e944d4d imposed a limit on the maximum number of stream in all states. However the default limit did not allow room for streams in addition to SETTINGS_MAX_CONCURRENT_STREAMS. This can mean streams in states outside the states which SETTINGS_MAX_CONCURRENT_STREAMS applies to may not be reliably created.

Modifications:
- The default limit should be larger than SETTINGS_MAX_CONCURRENT_STREAMS

Result:
More lenient limit is applied to maxStreams by default.
2016-08-11 09:38:58 -07:00
Scott Mitchell
765e944d4d HTTP/2 limit streams in all states
Motivation:
SETTINGS_MAX_CONCURRENT_STREAMS does not apply to idle streams and thus we do not apply any explicit limitations on how many idle streams can be created. This may allow a peer to consume an undesirable amount of resources.

Modifications:
- Each Endpoint should enforce a limit for streams in a any state. By default this limit will be the same as SETTINGS_MAX_CONCURRENT_STREAMS but can be overridden if necessary.

Result:
There is now a limit to how many IDLE streams can be created.
2016-08-11 09:01:37 -07:00
Scott Mitchell
a4ad68239e Http2ConnectionDecoder remove localSettings setter method
Motivation:
Http2ConnectionDecoder#localSettings(Http2Settings) is not used in codec-http2 and currently results in duplicated code.

Modifications:
- Remove Http2ConnectionDecoder#localSettings(Http2Settings)

Result:
Smaller interface and less duplicated code.
2016-08-10 12:23:03 -07:00
Norman Maurer
cb7cf4491c [maven-release-plugin] prepare for next development iteration 2016-07-27 13:29:56 +02:00
Norman Maurer
9466b32d05 [maven-release-plugin] prepare release netty-4.1.4.Final 2016-07-27 13:16:59 +02:00
buchgr
328510468c Complete ChannelPromise for Http2WindowUpdateFrames in Http2FrameCodec. Fixes #5530
Motivation:

The channel promise of a window update frame is not completed correctly,
depending on the failure or success of the operation.

Modification:

Succeed / Fail the promise if the window update succeeds / fails.

Result:

Correctly succeed / fail the promise.
2016-07-15 17:29:41 +02:00
Norman Maurer
047f6aed28 [maven-release-plugin] prepare for next development iteration 2016-07-15 09:09:13 +02:00
Norman Maurer
b2adea87a0 [maven-release-plugin] prepare release netty-4.1.3.Final 2016-07-15 09:08:53 +02:00
Oliver Gould
f2ce28bf18 Satisfy write promise when writing an Http2WindowUpdateFrame to Http2FrameCodec.
Motivation:

When writing an Http2WindowUpdateFrame to an Http2FrameCodec, the
ChannelPromise is never satisfied, so callers cannot generically rely on the
write future being satisfied on success.

Modifications:

When writing Http2WindowUpdateFrame, Http2FrameCodec now satisfies the
ChannelPromise immediately.

Result:

The write future is satisfied on successful writes.

Fixes netty/netty#5530.
2016-07-13 21:31:34 +02:00
Scott Mitchell
9de90d07a9 DefaultHttp2RemoteFlowController reentry infinite loop
Motivation:
DefaultHttp2RemoteFlowController.writePendingBytes does not support reentry but does not enforce this constraint. Reentry is possible if the channel transitions from Writable -> Not Writable -> Writable during the distribution phase. This can happen if the user flushes when notified of the channel transitioning to the not writable state, and may be done if the user wants to fill the channel outbound buffer before flushing.

Modifications:
- DefaultHttp2RemoteFlowController.writePendingBytes should protect against reentry

Result:
DefaultHttp2RemoteFlowController will not allocate unexpected amounts or enter an infinite loop.
2016-07-13 09:12:33 -07:00
Scott Mitchell
4baff691b4 DefaultPromise make listeners not volatile
Motivation:
DefaultPromise has a listeners member variable which is volatile to allow for an optimization which makes notification of listeners less expensive when there are no listeners to notify. However this change makes all other operations involving the listeners member variable more costly. This optimization which requires listeners to be volatile can be removed to avoid volatile writes/reads for every access on the listeners member variable.

Modifications:
- DefaultPromise listeners is made non-volatile and the null check optimization is removed

Result:
DefaultPromise.listeners is no longer volatile.
2016-07-07 12:53:03 -07:00
Norman Maurer
6492cb98b2 Revert "DefaultPromise make listeners not volatile"
This reverts commit 4d8132ff24 as I missed something I want to discuss first.
2016-07-07 08:37:41 +02:00
Scott Mitchell
4d8132ff24 DefaultPromise make listeners not volatile
Motivation:
DefaultPromise has a listeners member variable which is volatile to allow for an optimization which makes notification of listeners less expensive when there are no listeners to notify. However this change makes all other operations involving the listeners member variable more costly. This optimization which requires listeners to be volatile can be removed to avoid volatile writes/reads for every access on the listeners member variable.

Modifications:
- DefaultPromise listeners is made non-volatile and the null check optimization is removed

Result:
DefaultPromise.listeners is no longer volatile.
2016-07-07 08:01:25 +02:00
Norman Maurer
4676a2271c [maven-release-plugin] prepare for next development iteration 2016-07-01 10:33:32 +02:00
Norman Maurer
ad270c02b9 [maven-release-plugin] prepare release netty-4.1.2.Final 2016-07-01 09:07:40 +02:00
Scott Mitchell
6af56ffe76 HPACK Encoder headerFields improvements
Motivation:
HPACK Encoder has a data structure which is similar to a previous version of DefaultHeaders. Some of the same improvements can be made.

Motivation:
- Enforce the restriction that the Encoder's headerFields length must be a power of two so we can use masking instead of modulo
- Use AsciiString.hashCode which already has optimizations instead of having yet another hash code algorithm in Encoder

Result:
Fixes https://github.com/netty/netty/issues/5357
2016-06-30 09:00:12 -07:00
Scott Mitchell
a7f7d9c8e0 Remove unsafe char[] access in PlatformDependent
Motivation:
PlatformDependent attempts to use reflection to get the underlying char[] (or byte[]) from String objects. This is fragile as if the String implementation does not utilize the full array, and instead uses a subset of the array, this optimization is invalid. OpenJDK6 and some earlier versions of OpenJDK7 String have the capability to use a subsection of the underlying char[].

Modifications:
- PlatformDependent should not attempt to use the underlying array from String (or other data types) via reflection

Result:
PlatformDependent hash code generation for CharSequence does not depend upon specific JDK implementation details.
2016-06-30 08:58:28 -07:00
Scott Mitchell
df41be6fc8 HTTP/2 Decoder validate that GOAWAY lastStreamId doesn't increase
Motivation:
The HTTP/2 RFC states in https://tools.ietf.org/html/rfc7540#section-6.8 that Endpoints MUST NOT increase the value they send in the last stream identifier however we don't enforce this when decoding GOAWAY frames.

Modifications:
- Throw a connection error if the peer attempts to increase the lastStreamId in a GOAWAY frame

Result:
RFC is more strictly enforced.
2016-06-29 07:53:13 -07:00
buchgr
3613d15bca Split Http2MultiplexCodec into Frame- and MultiplexCodec + Tests. Fixes #4914
Motivation:

Quote from issue 4914:
"Http2MultiplexCodec currently does two things: mapping the existing h2 API to frames and managing the child channels.

It would be better if the two parts were separated. This would allow less-coupled development of the HTTP/2 handlers (flow control could be its own handler, for instance) and allow applications to insert themselves between all streams and the codec, which permits custom logic and could be used, in part, to implement custom frame types.

It would also greatly ease testing, as the child channel could be tested by itself without dealing with how frames are encoded on the wire."

Modifications:

- Split the Http2MultiplexCodec into Http2FrameCodec and Http2MultiplexCodec. The Http2FrameCodec interacts with the existing HTTP/2 callback-based API, while the Http2MulitplexCodec is completely independent of it and simply multiplexes Http2StreamFrames to the child channels. Additionally, the Http2Codec handler is introduced, which is a convenience class that simply sets up the Http2FrameCodec and Http2MultiplexCodec in the channel pipeline and removes itself.

- Improved test coverage quite a bit.

Result:

- The original Http2MultiplexCodec is split into Http2FrameCodec and Http2MultiplexCodec.
- More tests for higher confidence in the code.
2016-06-29 07:22:09 +02:00
Scott Mitchell
70651cc58d HpackUtil.equals performance improvement
Motivation:
PR #5355 modified interfaces to reduce GC related to the HPACK code. However this came with an anticipated performance regression related to HpackUtil.equals due to AsciiString's increase cost of charAt(..). We should mitigate this performance regression.

Modifications:
- Introduce an equals method in PlatformDependent which doesn't leak timing information and use this in HpcakUtil.equals

Result:
Fixes https://github.com/netty/netty/issues/5436
2016-06-27 14:37:39 -07:00
buchgr
73c4ad5f0a http2: count pad length field toward flow control. Fixes #5434
Motivation:
The HTTP/2 specification requires the pad length field of DATA, HEADERS and PUSH_PROMISE frames to be counted towards the flow control window. The current implementation doesn't do so (See #5434).

Furthermore, it's currently not possible to add one byte padding, as this would add the one byte pad length field as well as append one padding byte to the end of the frame.

Modifications:
Include the one byte pad length field in the padding parameter of the API. Thereby extending the allowed value range by one byte to 256 (inclusive). On the wire, a one byte padding is encoded with a pad length field with value zero and a 256 byte padding is encoded with a pad length field with value 255 and 255 bytes append to the end of the frame.

Result:
More correct padding.
2016-06-25 09:51:36 -07:00
Norman Maurer
b4d4c0034d Optimize HPACK usage to align more with Netty types and remove heavy object creations. Related to [#3597]
Motivations:

The HPACK code was not really optimized and written with Netty types in mind. Because of this a lot of garbage was created due heavy object creation.

This was first reported in [#3597] and https://github.com/grpc/grpc-java/issues/1872 .

Modifications:

- Directly use ByteBuf as input and output
- Make use of ByteProcessor where possible
- Use AsciiString as this is the only thing we need for our http2 usage

Result:

Less garbage and better usage of Netty apis.
2016-06-22 14:26:05 +02:00
Tim Brooks
d964bf6f18 Remove usages of deprecated methods group() and childGroup().
Motivation:

These methods were recently deprecated. However, they remained in use in several locations in Netty's codebase.

Modifications:

Netty's code will now access the bootstrap config to get the group or child group.

Result:

No impact on functionality.
2016-06-21 14:06:57 +02:00
Scott Mitchell
6aa5f76d42 HTTP/2 DelegatingDecompressorFrameListener return bytes to flow control
Motivation:
If a single DATA frame ends up being decompressed into multiple frames by DelegatingDecompressorFrameListener the flow control accounting is delayed until all frames have been decompressed. However it is possible the user may want to return bytes to the flow controller which were not included in the onDataRead return value. In this case the amount of processed bytes has not been incremented and will lead to negative value for processed bytes.

Modifications:
- Http2Decompressor.incrementProcessedBytes should be called each time onDataRead is called to ensure all bytes are accounted for at the correct time

Result:
Fixes https://github.com/netty/netty/issues/5375
2016-06-20 14:24:09 -07:00
Norman Maurer
e845670043 Set some StackTraceElement on pre-instantiated static exceptions
Motivation:

We use pre-instantiated exceptions in various places for performance reasons. These exceptions don't include a stacktrace which makes it hard to know where the exception was thrown. This is especially true as we use the same exception type (for example ChannelClosedException) in different places. Setting some StackTraceElements will provide more context as to where these exceptions original and make debugging easier.

Modifications:

Set a generated StackTraceElement on these pre-instantiated exceptions which at least contains the origin class and method name. The filename and linenumber are specified as unkown (as stated in the javadocs of StackTraceElement).

Result:

Easier to find the origin of a pre-instantiated exception.
2016-06-20 11:33:05 +02:00
Guido Medina
c3abb9146e Use shaded dependency on JCTools instead of copy and paste
Motivation:
JCTools supports both non-unsafe, unsafe versions of queues and JDK6 which allows us to shade the library in netty-common allowing it to stay "zero dependency".

Modifications:
- Remove copy paste JCTools code and shade the library (dependencies that are shaded should be removed from the <dependencies> section of the generated POM).
- Remove usage of OneTimeTask and remove it all together.

Result:
Less code to maintain and easier to update JCTools and less GC pressure as the queue implementation nt creates so much garbage
2016-06-10 13:19:45 +02:00
Norman Maurer
4dec7f11b7 [maven-release-plugin] prepare for next development iteration 2016-06-07 18:52:34 +02:00
Norman Maurer
cf670fab75 [maven-release-plugin] prepare release netty-4.1.1.Final 2016-06-07 18:52:22 +02:00
Scott Mitchell
79f2e3604e HTTP/2 close only send GO_AWAY if one has not already been sent
Motivation:
Http2ConnectionHandler will always send a GO_AWAY when the channel is closed. This may cause problems if the user is attempting to control when GO_AWAY is sent and the content of the GO_AWAY.

Modifications:
- When the channel is closed Http2ConnectionHandler should only send a GO_AWAY if one has not already been sent

Result:
The user has more control over when GO_AWAY is sent
Fixes https://github.com/netty/netty/issues/5307
2016-06-06 11:18:30 -07:00
Norman Maurer
844976a0a2 Ensure the same ByteBufAllocator is used in the EmbeddedChannel when compress / decompress. Related to [#5294]
Motivation:

The user may specify to use a different allocator then the default. In this case we need to ensure it is shared when creating the EmbeddedChannel inside of a ChannelHandler

Modifications:

Use the config of the "original" Channel in the EmbeddedChannel and so share the same allocator etc.

Result:

Same type of buffers are used.
2016-05-31 09:08:33 +02:00
Norman Maurer
6ca49d1336 [maven-release-plugin] prepare for next development iteration 2016-05-25 19:16:44 +02:00
Norman Maurer
446b38db52 [maven-release-plugin] prepare release netty-4.1.0.Final 2016-05-25 19:14:15 +02:00
nmittler
79a06eaede Fix NPE in Http2ConnectionHandler.onHttpServerUpgrade
Motivation:

Performing a server upgrade with a new initial flow control window will cause an NPE in the DefaultHttp2RemoteFlowController. This is due to the fact that the monitor does not check whether or not the channel is writable.

Modifications:

Added a check for channel writability before calling `writePendingBytes`. Also fixed a unit test that was supposed to be testing this :).

Result:

Fixes #5301
2016-05-25 09:13:06 -07:00
Norman Maurer
7b25402e80 Add CompositeByteBuf.addComponent(boolean ...) method to simplify usage
Motivation:

At the moment the user is responsible to increase the writer index of the composite buffer when a new component is added. We should add some methods that handle this for the user as this is the most popular usage of the composite buffer.

Modifications:

Add new methods that autoamtically increase the writerIndex when buffers are added.

Result:

Easier usage of CompositeByteBuf.
2016-05-21 19:52:16 +02:00
Scott Mitchell
1cb706ac93 HTTP/2 HPACK Header Name Validation and Trailing Padding
Motivation:
The HPACK code currently disallows empty header names. This is not explicitly forbidden by the HPACK RFC https://tools.ietf.org/html/rfc7541. However the HTTP/1.x RFC https://tools.ietf.org/html/rfc7230#section-3.2 and thus HTTP/2 both disallow empty header names, and so this precondition check should be moved from the HPACK code to the protocol level.
HPACK also requires that string literals which are huffman encoded must be treated as an encoding error if the string has more than 7 trailing padding bits https://tools.ietf.org/html/rfc7541#section-5.2, but this is currently not enforced.

Result:
- HPACK to allow empty header names
- HTTP/1.x and HTTP/2 header validation should not allow empty header names
- Enforce max of 7 trailing padding bits

Result:
Code is more compliant with the above mentioned RFCs
Fixes https://github.com/netty/netty/issues/5228
2016-05-17 13:42:16 -07:00
Trustin Lee
3a9f472161 Make retained derived buffers recyclable
Related: #4333 #4421 #5128

Motivation:

slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.

Modifications:

- Add the following methods which creates a non-recyclable derived buffer
  - retainedSlice()
  - retainedDuplicate()
  - readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
  own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
  - a user can replace the content of the holder in a consistent way
  - copy/duplicate/retainedDuplicate() can delegate the holder
    construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
  - Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
  - Make ReplayingDecoderByteBuf.reject() return an exception instead of
    throwing it so that its callers don't need to add dummy return
    statement

Result:

Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
2016-05-17 11:16:13 +02:00
Norman Maurer
68cd670eb9 Remove ChannelHandlerInvoker
Motivation:

We tried to provide the ability for the user to change the semantics of the threading-model by delegate the invoking of the ChannelHandler to the ChannelHandlerInvoker. Unfortunually this not really worked out quite well and resulted in just more complexity and splitting of code that belongs together. We should remove the ChannelHandlerInvoker again and just do the same as in 4.0

Modifications:

Remove ChannelHandlerInvoker again and replace its usage in Http2MultiplexCodec

Result:

Easier code and less bad abstractions.
2016-05-17 11:14:00 +02:00
Norman Maurer
979dc3e3e4 Fix one more possible leak as a follow up of 341a235fea 2016-05-13 18:38:37 +02:00
Norman Maurer
341a235fea Fix possible leaks in Http2ServerDowngraderTest
Motivation:

We need to ensure we release all ReferenceCounted objects during tests to not leak.

Modifications:

Fix possible leaks by calling release()

Result:

No more leaks in tests.
2016-05-13 17:38:26 +02:00
Norman Maurer
4b24b0aac8 Annotate Http2ServerDowngrader with @UnstableApi
Motivation:

Everything in the http2 package should be considered unstable for now

Modifications:

Add missing annotation on Http2ServerDowngrader

Result:

Clearly mark class as unstable.
2016-05-13 08:41:46 +02:00
Moses Nakamura
4d2e91a10d codec-http2: Stop leaking in header downgrader test
Motivation:

We're leaking requests in our Http2ServerDowngrader tests when we
allocate a buffer using the local allocator.

Modification:

Release the request later when the request is constructed with the local
allocator.

Result:

Less leaky tests.
2016-05-10 09:04:52 +02:00
Scott Mitchell
d580245afc DefaultHttp2Connection.close Reentrant Modification
Motivation:
The DefaultHttp2Conneciton.close method accounts for active streams being iterated and attempts to avoid reentrant modifications of the underlying stream map by using iterators to remove from the stream map. However there are a few issues:

- While iterating over the stream map we don't prevent iterations over the active stream collection
- Removing a single stream may actually remove > 1 streams due to closed non-leaf streams being preserved in the priority tree which may result in NPE

Preserving closed non-leaf streams in the priority tree is no longer necessary with our current allocation algorithms, and so this feature (and related complexity) can be removed.

Modifications:
- DefaultHttp2Connection.close should prevent others from iterating over the active streams and reentrant modification scenarios which may result from this
- DefaultHttp2Connection should not keep closed stream in the priority tree
  - Remove all associated code in DefaultHttp2RemoteFlowController which accounts for this case including the ReducedState object
  - This includes fixing writability changes which depended on ReducedState
- Update unit tests

Result:
Fixes https://github.com/netty/netty/issues/5198
2016-05-09 14:16:30 -07:00
Moses Nakamura
9ce84dcb21 Http2 to Http1.1 converter on new Http2 server API
Motivation:

http/2 and http/1.1 have similar protocols, and it's useful to be able
to implement a single server against a single interface. There's an
injection from http/1.1 messages to http/2 ones, so it makes sense to
make folks program against http/1.1 and upgrade them under the hood.

Modifications:

added a MessageToMessageCodec<Http2StreamFrame, HttpObject> which turns
every kind of Http2StreamFrame domain object into an HttpObject domain
object, and then back again on the way out.  This one is specialized for
servers, but it should be straightforward to make a symmetric one for
clients, or else extend this one.

Result:

fixes #5199, and it's now simple to make your Http2MultiplexCodec speak
Http1.1
2016-05-09 13:24:46 -07:00
Norman Maurer
9229ed98e2 [#5088] Add annotation which marks packages/interfaces/classes as unstable
Motivation:

Some codecs should be considered unstable as these are relative new. For this purpose we should introduce an annotation which these codecs should us to be marked as unstable in terms of API.

Modifications:

- Add UnstableApi annotation and use it on codecs that are not stable
- Move http2.hpack to http2.internal.hpack as it is internal.

Result:

Better document unstable APIs.
2016-05-09 15:16:35 +02:00
Scott Mitchell
b17ee6066f HTTP/2 Log Cleanup
Motivation:
The format of some log lines used the printf style formatting instead of the logger {} formatting for variables. This would lead to printing out the literal printf tokens instead of substituting the value.

Modifications:
- Fix the format string for log statements which use printf style formatting

Result:
Logs actually capture the value of the variables instead of fixed string tokens.
2016-05-06 10:52:00 -07:00
Scott Mitchell
467e5a1019 DefaultHttp2FrameReader stops reading if stream error
Motivation:
DefaultHttp2FrameReader will stop reading data if any exception is thrown. However some exceptions are recoverable and we will lose data if we don't continue reading. For example some stream errors are recoverable.

Modifications:
- DefaultHttp2FrameReader should attempt to continue reading if a stream error is encountered.

Result:
Fixes https://github.com/netty/netty/issues/5186
2016-04-30 10:41:06 -07:00
Trustin Lee
38d05abd84 Reorganize imports 2016-04-14 18:58:47 +09:00
Norman Maurer
d698746609 Add ByteBuf.asReadOnly()
Motivation:

We lately added ByteBuf.isReadOnly() which allows to detect if a buffer is read-only or not. We should add ByteBuf.asReadOnly() to allow easily access a read-only version of a buffer.

Modifications:

- Add ByteBuf.asReadOnly()
- Deprecate Unpooled.unmodifiableBuffer(Bytebuf)

Result:

More consistent api.
2016-04-14 10:51:20 +02:00
Norman Maurer
7c734fcf73 Fix resource leak in tests introduced by 69070c37ba. 2016-04-14 10:13:55 +02:00
Trustin Lee
0b078314b2 Add ByteBuf.isReadOnly()
Motivation:

It is sometimes useful to determins if a buffer is read-only.

Modifications:

Add ByteBuf.isReadOnly()

Result:

One more feature
2016-04-13 21:41:27 +09:00
Norman Maurer
572bdfb494 [maven-release-plugin] prepare for next development iteration 2016-04-10 08:37:18 +02:00
Norman Maurer
c6121a6f49 [maven-release-plugin] prepare release netty-4.1.0.CR7 2016-04-10 08:36:56 +02:00
Norman Maurer
6e919f70f8 [maven-release-plugin] rollback the release of netty-4.1.0.CR7 2016-04-09 22:13:44 +02:00
Norman Maurer
4cdd51509a [maven-release-plugin] prepare release netty-4.1.0.CR7 2016-04-09 22:05:34 +02:00
Trustin Lee
3b941c2a7c [maven-release-plugin] prepare for next development iteration 2016-04-02 01:25:05 -04:00
Trustin Lee
7368ccc539 [maven-release-plugin] prepare release netty-4.1.0.CR6 2016-04-02 01:24:55 -04:00
Norman Maurer
cee38ed2b6 [maven-release-plugin] prepare for next development iteration 2016-03-29 16:45:13 +02:00
Norman Maurer
9cd9e7daeb [maven-release-plugin] prepare release netty-4.1.0.CR5 2016-03-29 16:44:33 +02:00
Scott Mitchell
61cfdd7671 e24a5d8 compile error
Motivation:
e24a5d8 was cherry-picked but had a compile error.

Modifications:
- Fix the compile error in e24a5d8

Result:
Build now compiles.
2016-03-25 12:51:13 -07:00
Eric Anderson
e24a5d8839 Map HTTP/2 Streams to Channels
Motivation:

This allows using handlers for Streams in normal Netty-style. Frames are
read/written to the channel as messages, not directly as a
callback/method call. Handlers allow mixing and can ease HTTP/1 and
HTTP/2 interoperability by eventually supporting HTTP/1 handlers in
HTTP/2 and vise versa.

Modifications:

New handler Http2MultiplexCodec that converts from the current HTTP/2
API to a message-based API and child channels for streams.

Result:

The basics are done for server-side: new streams trigger creation of new
channels in much the same appearance to how new connections trigger new
channel creation. The basic frames HEADERS and DATA are handled, but
also GOAWAY and RST_STREAM.

Inbound flow control is implemented, but outbound is not. That will be
done later, along with not completing write promises on the child
channel until the write actually completes on the parent.

There is not yet support for outbound priority/weight, push promises,
and many other features.

There is a generic Object that may be set on stream frames. This also
paves the way for client-side support which needs a way to refer to
yet-to-be-created streams (due to how HEADERS allocates a stream id, and
the allocation order must be the same as transmission order).
2016-03-25 12:14:44 -07:00
Carsten Varming
d6bf388343 Prevent nepotism with generational GCs.
Motivation:

If a single Encoder object is promoted to the old generation then every object
reachable from the promoted object will eventually be promoted as well. A queue
illustrates the problem very well. Say a sequence of inserts and deletions
generate an object graph:

   A -> B -> C -> D -> E -> F -> G -> H,

the head of the queue is E, the tail of the queue is H, and A, B, C, D are
dead. If all queue nodes are in the young generation, then a young gc will
clean up the object graph and leave us with:

   E -> F -> G -> H

on the other hand, if B and C were previously promoted to the old generation,
then a young collection assumes the refernece from C to D is from a live object
(this is a key result of generational gc, no need to mark the old generation).
Hence the young collection assumes the refence to D is a gc root and leave us
with the object graph:

   B-> C -> D -> E -> F -> G -> H.

Eventually D, E, F, G, H, and all queue nodes ever seen from this point on will
be promoted, regardless of their global live or dead status. It is generally
trivial to fix nepotism issues by simply breaking the reference chain after
dequeuing a node.

Currently Encoder objects do not null their references when removed from the
hash map. We have observed a 20X increase in promoted Encoder objects due to
nepotism.

Modifications:

Null before, after, and next fields when removing Encoder objects from maps.

Result:

Fewer promoted Encoder objects, fewer Encoder objects in the old generation,
shorter young collection times, old collections spaced further apart (nepotism
is just really bad). Enjoy.
2016-03-23 17:27:00 +01:00
Norman Maurer
28d03adbfe [maven-release-plugin] prepare for next development iteration 2016-03-21 11:51:50 +01:00
Norman Maurer
4653dc1d05 [maven-release-plugin] prepare release netty-4.1.0.CR4 2016-03-21 11:51:12 +01:00
Scott Mitchell
fc099292fd HTTP/2 DefaultHttp2ConnectionEncoder data frame size incorrect if error
Motivation:
If an error occurs during a write operation then DefaultHttp2ConnectionEncoder.FlowControlledData will clear the CoalescingBufferQueue which will reset the queue's readable bytes to 0. To recover from an error the DefaultHttp2RemoteFlowController will attempt to return bytes to the flow control window, but since the frame has reset its own size this will lead to invalid flow control accounting.

Modifications:
- DefaultHttp2ConnectionEncoder.FlowControlledData should not reset its size if an error occurs

Result:
No more flow controller errors due to DefaultHttp2ConnectionEncoder.FlowControlledData setting its size to 0 if an error occurs.
2016-03-18 11:38:01 -07:00
Norman Maurer
8ec594c6eb Change HttpServerUpgradeHandler.UpgradeCodec to allow aborting upgrade
Motivation:

HttpServerUpgradeHandler.UpgradeCodec.prepareUpgradeResponse should allow to abort the upgrade and so just continue with using HTTP. Beside this we should only pass in the response HttpHeaders as this is inline with the docs.

Modifications:

- UpgradeCodec.prepareUpgradeResponse now allows to return a boolean and so allows to specifiy if the upgrade should take place.
- Change the param from FullHttpResponse to HttpHeaders to be inline with the javadocs.

Result:

More flexible and correct handling of upgrades.
2016-03-18 17:01:59 +01:00
Norman Maurer
ed9d6c79bc [#4972] Remove misleading argument from HttpServerUpgradeHandler.UpgradeCodec.upgradeTo
Motivation:

upgradeTo(...) takes the response as paramater, but the respone itself was already written to the Channel. This gives the user the impression the response can be changed or even act on it which may not be safe anymore once it was written and has been released.

Modifications:

Remove the response param from the method.

Result:

Less confusion and safer usage.
2016-03-17 10:50:07 +01:00
Scott Mitchell
6a2425b846 HTTP/2 SimpleChannelPromiseAggregator don't fail fast
Motivation:
Http2Codec.SimpleChannelPromiseAggregator currently fails fast if as soon as a tryFailure or setFailure method is called. This can lead to write operations which pass the result of SimpleChannelPromiseAggregator.newPromise to multiple channel.write calls throwing exceptions due to the promise being already done. This behavior is not expected by most of the Netty codecs (SslHandler) and can also create unexpected leaks in the http2 codec (DefaultHttp2FrameWriter).

Modifications:
- Http2Codec.SimpleChannelPromiseAggregator shouldn't complete the promise until doneAllocatingPromises is called
- Usages of Http2Codec.SimpleChannelPromiseAggregator should be adjusted to handle the change in behavior
- What were leaks in DefaultHttp2FrameWriter should be fixed to catch any other cases where ctx.write may throw

Result:
SimpleChannelPromiseAggregator won't generate promises which are done when newPromise is called.
2016-03-14 11:00:21 -07:00
Scott Mitchell
45849b2fa8 Deprecate PromiseAggregator
Motivation:
PromiseAggregator's API allows for the aggregate promise to complete before the user is done adding promises. In order to support this use case the API structure would need to change in a breaking manner.

Modifications:
- Deprecate PromiseAggregator and subclasses
- Introduce PromiseCombiner which corrects these issues

Result:
PromiseCombiner corrects the deficiencies in PromiseAggregator.
2016-03-14 10:53:30 -07:00
Scott Mitchell
404666d247 HTTP/2 ByteBufUtil.writeUtf8 cleanup
Motiviation:
691bc1690e made writeUtf8 consistent with String.getBytes() so that it never throws.
94f27be59b provided a writeUtf8 method which takes a ByteBufAllocator to do an appropriately sized buffer allocation.

Result:
- Assume writeUtf8 will not throw in HTTP/2 codec
- Use the new writeUtf8 method

Result:
Cleaner code in codec-http2.
2016-03-11 14:24:45 -08:00
Scott Mitchell
bd6040a36e HTTP/2 DefaultHttp2Connection NPE
Motivation:
If while iterating the active streams a close operation occurs this will be queued and process after the iteration has completed to avoid a concurrent modification exception. However it is possible that during the iteration the stream which was closed could have been removed from the priority tree and its parent would be set to null. Then after the iteration completes the close operation will attempt to dereference the parent and results in a NPE.

Modifications:
- pending close operations should verify the stream's parent is not null before processing the event

Result:
No More NPE.
2016-03-11 07:41:04 -08:00
Scott Mitchell
900353af52 HTTP/2 Reduce Log Level
Motivation:
83c4aa6ad8 changed the log level to warn, but should have changed to debug.

Modifications:
- Change the log level to debug in Http2ConnectionHandler if the GO_AWAY fails to send. The write failure could be the result of the channel already being closed.

Result:
Fixes https://github.com/netty/netty/issues/4930.
2016-03-04 17:25:04 -08:00
Scott Mitchell
4b5b230802 HTTP/2 DefaultHttp2HeadersDecoder weighted average error
Motiviation:
cfcee5798d introduced code to resize the headers based upon a weighted average. The weight used for new entries was initialized using integer arithmetic when it should have been floating point arithmetic and so new values contribute 0 weight.

Modifications:
- Cast to float when initializing

Result:
Weighted average does not give 0 weight to new headers in DefaultHttp2HeadersDecoder.
2016-03-02 15:07:38 -08:00
Scott Mitchell
c4fbc0642d HTTP/2 stream removed from map before onStreamClosed called
Motivation:
The interface contract of Http2Connection.Listener.onStreamClosed says that the stream will be removed from the active stream map, and not necessarily the stream map. If the channel becomes inactive we may remove from the stream map before calling onStreamClosed.

Modifications:
- Don't remove from the stream map during iteration until after onStreamClosed is called

Result:
Expectations of onStreamClosed interface are not violated
2016-02-29 08:46:22 -08:00
Norman Maurer
ca443e42e0 [maven-release-plugin] prepare for next development iteration 2016-02-19 23:00:11 +01:00
Norman Maurer
f39eb9a6b2 [maven-release-plugin] prepare release netty-4.1.0.CR3 2016-02-19 22:59:52 +01:00
Scott Mitchell
83c4aa6ad8 HTTP/2 Writes GO_AWAY on channelInactive
Motivation:
Http2ConnectionHandler inherits from ByteToMessageDecoder. ByteToMessageDecoder.channelInactive will attempt to decode any remaining data by calling the abstract decode method. If the Http2ConnectionHandler is in server mode, and no data has been exchanged yet, it will try to treat this data as an invalid connection preface and write a GO_AWAY. This is noisy in the logs and creates an illusion that there is a protocol violation when there has not been.

Modifications:
- If the channel is inactive the connection preface decode should not be executed.

Result:
Log files don't include misleading error messages related to connection preface errors.
2016-02-18 19:47:42 -08:00
Brendt Lucas
41d0a81691 Use ByteBufAllocator to allocate ByteBuf for FullHttpMessage Motivation: When converting SPDY or HTTP/2 frames to HTTP/1.x, netty always used an unpooled heap ByteBuf.
Modifications:
When constructing the FullHttpMessage pass in the ByteBuf to use via the ByteBufAllocator assigned via the context.

Result:
The ByteBuf assigned to the FullHttpMessage can now be configured as a pooled/unpooled, direct/heap based ByteBuf via the ByteBufAllocator used.
2016-02-17 19:55:52 -08:00
Moses Nakamura
f0f0b69d90 fixed "sensative" typo to read "sensitive" 2016-02-17 08:18:11 -08:00
Scott Mitchell
06e29e0d1b HTTP/2 codec may not always call Http2Connection.onStreamRemoved
Motivation:
Http2Connection.onStreamRemoved is not always called if Http2Connection.onStreamAdded is called. This is problematic as users may rely on the onStreamRemoved method to be called to release ByteBuf objects and do other cleanup.

Modifications:
- Http2Connection.close will remove all streams existing streams and prevent new ones from being created
- Http2ConnectionHandler will call the new close method in channelInactive

Result:
Http2Connection.onStreamRemoved is always called when Http2Connection.onStreamRemoved is called to preserve the Http2Connection guarantees.
Fixes https://github.com/netty/netty/issues/4838
2016-02-12 16:01:37 -08:00
Scott Mitchell
56e6e07b25 HTTP/2 RST_STREAM Regression f990f99
Motivation:
Commit f990f99 introduced a bug into the RST_STREAM processing that would prevent a RST_STREAM from being sent when it should have been. The promise would be marked as successful even though the RST_STREAM frame would never be sent.

Modifications:
- Fix conditional in Http2ConnectionHandler.resetStream to allow reset streams to be sent in all stream states besides IDLE.

Result:
RST_STREAM frames are now sent when they are supposed to be sent
Fixes https://github.com/netty/netty/issues/4856
2016-02-10 13:47:53 -08:00
Norman Maurer
75a2ddd61c [maven-release-plugin] prepare for next development iteration 2016-02-04 16:51:44 +01:00
Norman Maurer
7eb3a60dba [maven-release-plugin] prepare release netty-4.1.0.CR2 2016-02-04 16:37:06 +01:00
Scott Mitchell
7a7160f176 HTTP/2 Buffer Leak if UTF8 Conversion Fails
Motivation:
Http2CodecUtil uses ByteBufUtil.writeUtf8 but does not account for it
throwing an exception. If an exception is thrown because the format is
not valid UTF16 encoded UTF8 then the buffer will leak.

Modifications:
- Make sure the buffer is released if an exception is thrown
- Ensure call sites of the Http2CodecUtil.toByteBuf can tolerate and
  exception being thrown

Result:
No leak if exception data can not be converted to UTF8.
2016-02-02 11:22:17 -08:00
Scott Mitchell
f990f9983d HTTP/2 Don't Flow Control Iniital Headers
Motivation:
Currently the initial headers for every stream is queued in the flow controller. Since the initial header frame may create streams the peer must receive these frames in the order in which they were created, or else this will be a protocol error and the connection will be closed. Tolerating the initial headers being queued would increase the complexity of the WeightedFairQueueByteDistributor and there is benefit of doing so is not clear.

Modifications:
- The initial headers will no longer be queued in the flow controllers

Result:
Fixes https://github.com/netty/netty/issues/4758
2016-02-01 13:37:43 -08:00
Scott Mitchell
5fb18e3415 InboundHttp2ToHttpAdapter leak and logic improvements
Motivation:
In HttpConversionUtil's toHttpRequest and toHttpResponse methods can
allocate FullHttpMessage objects, and if an exeception is thrown during
the header conversion then this object will not be released. If a
FullHttpMessage is not fired up the pipeline, and the stream is closed
then we remove from the map, but do not release the object. This leads
to a ByteBuf leak. Some of the logic related to stream lifetime management
and FullHttpMessage also predates the RFC being finalized and is not correct.

Modifications:
- Fix leaks in HttpConversionUtil
- Ensure the objects are released when they are removed from the map.
- Correct logic and unit tests where they are found to be incorrect.

Result:
Fixes https://github.com/netty/netty/issues/4780
Fixes https://github.com/netty/netty/issues/3619
2016-02-01 12:38:40 -08:00
Trustin Lee
4d6ab1d30d Fix missing trailing data on HTTP client upgrade
Motivation:

When HttpClientUpgradeHandler upgrades from HTTP/1 to another protocol,
it performs a two-step opertion:

1. Remove the SourceCodec (HttpClientCodec)
2. Add the UpgradeCodec

When HttpClientCodec is removed from the pipeline, the decoder being
removed triggers channelRead() event with the data left in its
cumulation buffer. However, this is not received by the UpgradeCodec
becuase it's not added yet. e.g. HTTP/2 SETTINGS frame sent by the
server can be missed out.

To fix the problem, we need to reverse the steps:

1. Add the UpgradeCodec
2. Remove the SourceCodec

However, this does not work as expected either, because UpgradeCodec can
send a greeting message such as HTTP/2 Preface. Such a greeting message
will be handled by the SourceCodec and will trigger an 'unsupported
message type' exception.

To fix the problem really, we need to make the upgrade process 3-step:

1. Remove/disable the encoder of SourceCodec
2. Add the UpgradeCodec
3. Remove the SourceCodec

Modifications:

- Add SourceCodec.prepareUpgradeFrom() so that SourceCodec can remove or
  disable its encoder
- Implement HttpClientCodec.prepareUpgradeFrom() properly
- Miscellaneous:
  - Log the related channel as well When logging the failure to send a
    GOAWAY

Result:

Cleartext HTTP/1-to-HTTP/2 upgrade works again.
2016-02-01 15:52:37 +01:00
Scott Mitchell
11bcb8790c Http2Connection stream id generation to support queueing
Motivation:
StreamBufferingEncoder provides queueing so that MAX_CONCURRENT_STREAMS is not violated. However the stream id generation provided by Http2Connection.nextStreamId() only returns the next stream id that is expected on the connection and does not account for queueing. The codec should provide a way to generate the next stream id for a given endpoint that functions with or without queueing.

Modifications:
- Change Http2Connection.nextStreamId to Http2Connection.incrementAndGetNextStreamId

Result:
Http2Connection can generate the next stream id in queued and non-queued scenarios.
Fixes https://github.com/netty/netty/issues/4704
2016-01-29 11:37:17 -08:00
Scott Mitchell
78b508a7eb AbstractHttp2ConnectionHandlerBuilder validateHeaders cannot be set with encoder/decoder
Motivation:
If validateHeaders is set in combination with the encoder/decoder it will be silently ignored. We should enforce the constraint that validateHeaders and encoder/decoder are mutually exclusive.

Modifications:
- Make sure either validateHeaders can be set or encoder/decoder.

Result:
AbstractHttp2ConnectionHandlerBuilder does not allow conflicting options to be set.
2016-01-29 00:33:45 -08:00
Scott Mitchell
e8850072e2 HTTP/2 DefaultHttp2RemoteFlowController frame merging with padding bug
Motivation:
DefaultHttp2RemoteFlowController does not correctly account for the padding in the event frames are merged. This causes the internal stat of DefaultHttp2RemoteFlowController to become corrupt and can result in attempting to write frames when there are none.

Modifications:
- Update DefaultHttp2RemoteFlowController to account for frame sizes not necessarily adding together.

Result:
DefaultHttp2RemoteFlowController internal state does not become corrupt when padding is present.
Fixes https://github.com/netty/netty/issues/4573
2016-01-27 14:52:00 -08:00
Norman Maurer
1c417e5f82 [maven-release-plugin] prepare for next development iteration 2016-01-21 15:35:55 +01:00
Norman Maurer
c681a40a78 [maven-release-plugin] prepare release netty-4.1.0.CR1 2016-01-21 15:28:21 +01:00
Alex Petrov
7bcae8919d Log the current channel in Http2FrameLogger
Motivation:
Currently it's impossible to distinguish which
connection the corresponding logged message	is
related to.

Modifications:
Http2FrameLogger is extended to support channel
id logging, usages in Inbound/Outbound Frame
Loggers are adjusted accordingly.

Result:
Logger outputs the channel id.
2016-01-18 10:52:08 +01:00
Scott Mitchell
7dba13f276 HttpConversionUtil remove throws from method signature
Motivation:
HttpConversionUtil.toHttp2Headers currently has a throws Exception as part of the signature. This comes from the signature of ByteProcessor.process, but is not necessary because the ByteProcessor used does not throw.

Modifications:
- Remove throws Exception from the signature of HttpConversionUtil.toHttp2Headers.

Result:
HttpConversionUtil.toHttp2Headers interface does not propagate a throws Exception when it is used.
2016-01-15 10:53:34 +01:00
Norman Maurer
8716b9d4bd Revert "Fix unnecessary boxing and incorrect Serializable"
This reverts commit 0ae6f17285.
2015-12-31 14:48:10 +01:00
Xiaoyan Lin
0ae6f17285 Fix unnecessary boxing and incorrect Serializable
Motivation:

- AbstractHttp2ConnectionHandlerBuilder.encoderEnforceMaxConcurrentStreams can be the primitive boolean
- SpdySession.StreamComparator should not be Serializable since SpdySession is not Serializable

Modifications:

Use boolean instead and remove Serializable

Result:

- Minor improvement for AbstractHttp2ConnectionHandlerBuilder
- StreamComparator is not Serializable any more
2015-12-31 10:45:24 +01:00
Xiaoyan Lin
475d901131 Fix errors reported by javadoc
Motivation:

Javadoc reports errors about invalid docs.

Modifications:

Fix some errors reported by javadoc.

Result:

A lot of javadoc errors are fixed by this patch.
2015-12-27 08:36:45 +01:00
Xiaoyan Lin
a96d52fe66 Fix javadoc links and tags
Motivation:

There are some wrong links and tags in javadoc.

Modifications:

Fix the wrong links and tags in javadoc.

Result:

These links will work correctly in javadoc.
2015-12-26 08:34:31 +01:00
Scott Mitchell
80cff236e4 HTTP/2 UniformStreamByteDistributor negative window shouldn't write
Motivation:
If the stream window is negative UniformStreamByteDistributor may write data. This is prohibited by the RFC https://tools.ietf.org/html/rfc7540#section-6.9.2.

Modifications:
- UniformStreamByteDistributor should use StreamState.isWriteAllowed()

Result:
UniformStreamByteDistributor is more complaint with HTTP/2 RFC.
Fixes https://github.com/netty/netty/issues/4545
2015-12-23 10:14:24 -08:00
Scott Mitchell
7b2f55ec2f HTTP/2 Remove RemoteFlowController.streamWritten
Motivation:
RemoteFlowController.streamWritten is not currently required. We should remove it to keep interfaces minimal.

Modifications:
- Remove RemoteFlowController.streamWritten

Result:
1 Less method in RemoteFlowController interface.
Fixes https://github.com/netty/netty/issues/4600
2015-12-22 16:59:40 -08:00
Scott Mitchell
72accceeac HTTP/2 remove PriorityStreamByteDistributor
Motivation:
PriorityStreamByteDistributor is now obsolete and can be replaced by WeightedFairQueueByteDistributor.

Modifications:
- Remove PriorityStreamByteDistributor and use WeightedFairQueueByteDistributor by default.

Result:
PriorityStreamByteDistributor no longer has to be maintained and is replaced by a better algorithm.
2015-12-21 08:56:50 -08:00
Scott Mitchell
9ac430f16f HTTP/2 DefaultHttp2RemoteFlowController Stream writability notification broken
Motivation:
DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor no longer reliably detects when a stream's writability change occurs.

Modifications:
- Ensure writiability is reliabily reported by DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor
- Fix infinite loop issue (https://github.com/netty/netty/issues/4588) detected when consolidating unit tests

Result:
Reliable stream writability change notification, and 1 less infinite loop in UniformStreamByteDistributor.
Fixes https://github.com/netty/netty/issues/4587
2015-12-21 10:01:33 +01:00
Trustin Lee
b39380ad83 Revamp InboundHttp2ToHttpAdapter builder API
Related: #4572 #4574

Motivation:

Consistency in our builder API design

Modifications:

- Add AbstractInboundHttp2ToHttpAdapterBuilder
- Replace the old 'Builder's with InboundHttp2ToHttpAdapterBuilder and
  InboundHttp2ToHttpPriorityAdapterBuilder

Result:

Builder API consistency
2015-12-18 12:44:46 +09:00
Scott Mitchell
904e70a4d4 HTTP/2 Weighted Fair Queue Byte Distributor
Motivation:
PriorityStreamByteDistributor uses a homegrown algorithm which distributes bytes to nodes in the priority tree. PriorityStreamByteDistributor has no concept of goodput which may result in poor utilization of network resources. PriorityStreamByteDistributor also has performance issues related to the tree traversal approach and number of nodes that must be visited. There also exists some more proven algorithms from the resource scheduling domain which PriorityStreamByteDistributor does not employ.

Modifications:
- Introduce a new ByteDistributor which uses elements from weighted fair queue schedulers

Result:
StreamByteDistributor which is sensitive to priority and uses a more familiar distribution concept.
Fixes https://github.com/netty/netty/issues/4462
2015-12-17 11:17:02 -08:00
Trustin Lee
2202e8f967 Revamp the Http2ConnectionHandler builder API
Related: #4572

Motivation:

- A user might want to extend Http2ConnectionHandler and define his/her
  own static inner Builder class that extends
  Http2ConnectionHandler.BuilderBase. This introduces potential
  confusion because there's already Http2ConnectionHandler.Builder. Your
  IDE will warn about this name duplication as well.
- BuilderBase exposes all setters with public modifier. A user's Builder
  might not want to expose them to enforce it to certain configuration.
  There's no way to hide them because it's public already and they are
  final.
- BuilderBase.build(Http2ConnectionDecoder, Http2ConnectionEncoder)
  ignores most properties exposed by BuilderBase, such as
  validateHeaders, frameLogger and encoderEnforceMaxConcurrentStreams.
  If any build() method ignores the properties exposed by the builder,
  there's something wrong.
- A user's Builder that extends BuilderBase might want to require more
  parameters in build(). There's no way to do that cleanly because
  build() is public and final already.

Modifications:

- Make BuilderBase and Builder top-level so that there's no duplicate
  name issue anymore.
  - Add AbstractHttp2ConnectionHandlerBuilder
  - Add Http2ConnectionHandlerBuilder
  - Add HttpToHttp2ConnectionHandlerBuilder
- Make all builder methods in AbstractHttp2ConnectionHandlerBuilder
  protected so that a subclass can choose which methods to expose
- Provide only a single build() method
  - Add connection() and codec() so that a user can still specify
    Http2Connection or Http2Connection(En|De)coder explicitly
  - Implement proper state validation mechanism so that it is prevented
    to invoke conflicting setters

Result:

Less confusing yet flexible builder API
2015-12-17 14:08:13 +09:00
Norman Maurer
f4386fb8e9 Allow to set Http2HeaderEncoder.SensitivityDetector in the Http2ConnectionHandler
Motivation:

Some times the user wants to set a Http2HeaderEncoder.SensitivityDetector when building a Http2ConnectionHandler.

Modifications:

Allow to set Http2HeaderEncoder.SensitivityDetector via builder.

Result:

More flexible building of Http2ConnectionHandler possible.
2015-12-10 14:34:04 +01:00
Norman Maurer
c6d43667d4 Add Http2HeadersEncoder.ALWAYS_SENSITIVE instance
Motivation:

We already provide a NEVER_SENSITIVE instance,we should add ALWAYS_SENSITIVE as well.

Modifications:

Add ALWAYS_SENSITIVE instance which will always return true when check for sesitive.

Result:

User can reuse code.
2015-12-10 12:59:04 +01:00
Scott Mitchell
6257091d12 HttpConversionUtil does not account for COOKIE compression
Motivation:
The HTTP/2 RFC allows for COOKIE values to be split into individual header elements to get more benefit from compression (https://tools.ietf.org/html/rfc7540#section-8.1.2.5). HttpConversionUtil was not accounting for this behavior.

Modifications:
- Modify HttpConversionUtil to support compressing and decompressing the COOKIE values

Result:
HttpConversionUtil is compatible with https://tools.ietf.org/html/rfc7540#section-8.1.2.5)
Fixes https://github.com/netty/netty/issues/4457
2015-12-08 20:00:31 -08:00
nmittler
8cd259896e No HTTP/2 RST_STREAM if no prior HEADERS were sent
Motivation:

Because we flow control HEADERS frames, it's possible that an intermediate error can result in a RST_STREAM frame being sent for a frame that the other endpoint is not yet aware of. This is a violation of the spec and will either result in spammy logs at the other endpoint or broken connections.

Modifications:

Modified the HTTP/2 handler so that it only sends RST_STREAM if it has sent at least one HEADERS frame to the remote endpoint for the stream.

Result:

Fixes #4465
2015-11-25 13:46:32 -08:00
nmittler
dbaeb3314e Allow HTTP2 frame writer to accept arbitrarily large frames
Motivation:

The encoder is currently responsible for chunking frames when writing in order to conform to max frame size. The frame writer would be a better place for this since it could perform a reuse the same promise aggregator for all the write and could also perform a single allocation for all of the frame headers.

Modifications:

Modified `DefaultHttp2FrameWriter` to perform the chunking and modified the contract in the `Http2FrameWriter` interface. Modified `DefaultHttp2ConnectionEncoder` to send give all allocated bytes to the writer.

Result:

Fixes #3966
2015-11-24 11:44:06 -08:00
nmittler
79ab756fa3 Use a single queue in UniformStreamByteDistributor
Motivation:

The UniformStreamByteDistributor currently processes all zero-length frames, regardless of add order. This means that we would always send HEADERS for all streams, possibly taking away bandwidth for streams that actually have data.

Modifications:

Empty frames are now treated the same as any other frame except that the algorithm will pop off the any empty frames at the head of the queue.

Result:

Empty frames require no extra processing.
2015-11-24 08:11:23 -08:00
Scott Mitchell
cfcee5798d Adjustable size of DefaultHeaders array
Motivation:
DefaultHeaders creates an array of size 16 for all headers. This may waste a good deal of memory if applications only have a small number of headers. This memory may be critical when the number of connections grows large.

Modifications:
- Make the size of the array for DefaultHeaders configurable

Result:
Applications can control the size of the DefaultHeaders array and save memory.
2015-11-23 15:38:08 -08:00
nmittler
2a2059d976 Adding UniformStreamByteDistributor
Motivation:

The current priority algorithm can yield poor per-stream goodput when either the number of streams is high or the connection window is small. When all priorities are the same (i.e. priority is disabled), we should be able to do better.

Modifications:

Added a new UniformStreamByteDistributor that ignores priority entirely and manages a queue of streams.  Each stream is allocated a minimum of 1KiB on each iteration.

Result:

Improved goodput when priority is not used.
2015-11-19 16:49:12 -08:00
nmittler
96f9b0b91b Remote flow controller incorrectly updates stream state
Motivation:

The `DefaultHttp2RemoteFlowController` does not correctly determine `hasFrame` when updating the stream state for the distributor. Adding a check to enforce `hasFrame` when `streamableBytes > 0` causes several test failures.

Modifications:

Modified `DefaultHttp2RemoteFlowController` to simplify the writing logic and to correct the bookkeeping for `hasFrame`.

Result:

The distributors are always called with valid arguments.
2015-11-18 11:32:18 -08:00
nmittler
8accc52b03 Forking Twitter's hpack
Motivation:

The twitter hpack project does not have the support that it used to have.  See discussion here: https://github.com/netty/netty/issues/4403.

Modifications:

Created a new module in Netty and copied the latest from twitter hpack master.

Result:

Netty no longer depends on twitter hpack.
2015-11-14 10:13:32 -08:00
Norman Maurer
2ecce8fa56 [maven-release-plugin] prepare for next development iteration 2015-11-10 22:59:33 +01:00
Norman Maurer
6a93f331d3 [maven-release-plugin] prepare release netty-4.1.0.Beta8 2015-11-10 22:50:57 +01:00
Scott Mitchell
2f81364522 DefaultHttp2HeadersTest updates
Motivation:
Recently a bug was found in DefaultHttp2Headers where the state of the headers could be corrupted due to the extra tracking to make pseudo headers first during iteration. Unit tests did not catch this bug.

Modifications:
- Update unit tests to cover more methods

Result:
Unit tests for DefaultHttp2Headers have better code coverage.
2015-11-07 10:14:00 -08:00
Louis Ryan
7cc320ce47 Fix memory leak in DefaultHttp2Headers
Motivation:

Memory leak makes headers non-reusable.

Modifications:

Correctly reset firstNonPseudo header reference

Result:

No leak
2015-11-06 07:08:57 -08:00
nmittler
6504d52b94 Add HTTP/2 local flow control option for auto refill
Motivation:

For many HTTP/2 applications (such as gRPC) it is necessary to autorefill the connection window in order to prevent application-level deadlocking.

Consider an application with 2 streams, A and B.  A receives a stream of messages and the application pops off one message at a time and makes a request on stream B. However, if receiving of data on A has caused the connection window to collapse, B will not be able to receive any data and the application will deadlock.  The only way (currently) to get around this is 1) use multiple connections, or 2) manually refill the connection window.  Both are undesirable and could needlessly complicate the application code.

Modifications:

Add a configuration option to DefaultHttp2LocalFlowController, allowing it to autorefill the connection window.

Result:

Applications can configure HTTP/2 to avoid inter-stream deadlocking.
2015-11-05 15:47:10 -08:00
Scott Mitchell
91b8ef3d10 HTTP/2 PriorityStreamByteDistributor exceptions and reentry
Motivation:
PriorityStreamByteDistributor saves exception state and attempts to reset state. This could be simplified by just throwing a connection error and closing the connection. PriorityStreamByteDistributor also does not handle or detect re-entry in the distribute method.

Motivation:
- PriorityStreamByteDistributor propagate an INTERNAL_ERROR if an exception occurs during writing
- PriorityStreamByteDistributor to handle re-entry on the write method

Result:
PriorityStreamByteDistributor exception code state simplified, and re-entry is detected.
2015-11-03 13:22:11 -08:00
Trustin Lee
8f334885ef Reject the first SETTINGS ack on HTTP/2 Preface
Motivation:

Http2ConnectionHandler verifies if the first frame after the preface is
a SETTINGS frame.  However, it does not reject the SETTING ack frame
which is not expected actually.

Modifications:

Reject a SETTINGS-ack frame as well

Result:

When the first frame is a SETTINGS-ack frame, connection does not
proceed to further frame handling. (simplicity)
2015-11-03 11:23:54 +09:00
Scott Mitchell
19658e9cd8 HTTP/2 Headers Type Updates
Motivation:
The HTTP/2 RFC (https://tools.ietf.org/html/rfc7540#section-8.1.2) indicates that header names consist of ASCII characters. We currently use ByteString to represent HTTP/2 header names. The HTTP/2 RFC (https://tools.ietf.org/html/rfc7540#section-10.3) also eludes to header values inheriting the same validity characteristics as HTTP/1.x. Using AsciiString for the value type of HTTP/2 headers would allow for re-use of predefined HTTP/1.x values, and make comparisons more intuitive. The Headers<T> interface could also be expanded to allow for easier use of header types which do not have the same Key and Value type.

Motivation:
- Change Headers<T> to Headers<K, V>
- Change Http2Headers<ByteString> to Http2Headers<CharSequence, CharSequence>
- Remove ByteString. Having AsciiString extend ByteString complicates equality comparisons when the hash code algorithm is no longer shared.

Result:
Http2Header types are more representative of the HTTP/2 RFC, and relationship between HTTP/2 header name/values more directly relates to HTTP/1.x header names/values.
2015-10-30 15:29:44 -07:00
Scott Mitchell
d66520db1b Http2ConnectionHandler.BaseBuilder exception cleanup
Motivation:
Http2ConnectionHandler.BaseBuilder is constructing objects which have 'close' methods, but is not calling these methods in the event of an exception.

Modifications:
- Objects which implement 'close' should have this method called if an exception is thrown and the build operation can not complete normally.

Result:
Objects are closed even if the build process encounters an error.
2015-10-29 11:09:51 -07:00
Norman Maurer
077af8c019 Remove encoderMaxConcurrentStreams
Motivation:

Remove encoderMaxConcurrentStreams(...) and use the default settings. Also throw an exception if server mode is used.

Modifications:

- Remove encoderMaxConcurrentStreams(...) method
- Throw exception if server mode is used and trying to enforce conncurrent streams.

Result:

Correctly support settings stuff via builder
2015-10-15 10:11:39 +02:00
Norman Maurer
e419533498 Remove unused parameter from method declaration.
Motivation:

We had an unused paramter on a method, we should just remove it to keep code clean.

Modifications:

- Remove parameter
- Fix typo in javadoc

Result:

Cleanup done.
2015-10-07 09:18:22 +02:00
Norman Maurer
2ff2806ada [maven-release-plugin] prepare for next development iteration 2015-10-02 09:03:29 +02:00
Norman Maurer
5a43de10f7 [maven-release-plugin] prepare release netty-4.1.0.Beta7 2015-10-02 09:02:58 +02:00
Scott Mitchell
06c3ae07a0 DefaultHttp2ConnectionDecoder write ping buffer
Motivation:
DefaultHttp2ConnectionDecoder writes a ACK when receiving a ping frame and sends the same data buffer it received. The data buffer is also passed to the listener, but the indexes are shared between the send and the listener. We should ensure the indexes are independent for these two operations.

Modifications:
- Call slice on the buffer that is being sent

Result:
Listener now has access to a buffer that will not appear to be already consumed.
2015-10-02 11:40:21 -07:00
Scott Mitchell
284e3702d8 Http2ConnectionHandler Builder instead of constructors
Motivation:
Using the builder pattern for Http2ConnectionHandler (and subclasses) would be advantageous for the following reasons:
1. Provides the consistent construction afforded by the builder pattern for 'optional' arguments. Users can specify these options 1 time in the builder and then re-use the builder after this.
2. Enforces that the Http2ConnectionHandler's internals (decoder Http2FrameListener) are initialized after construction.

Modifications:
- Add an extensible builder which can be used to build Http2ConnectionHandler objects
- Update classes which inherit from Http2ConnectionHandler

Result:
It is easier to specify options and construct Http2ConnectionHandler objects.
2015-10-01 13:51:03 -07:00
Scott Mitchell
1485a87e25 Http2ConnectionHandler and Http2FrameListener cyclic dependency
Motivation:
It is often the case that implementations of Http2FrameListener will want to send responses when data is read. The Http2FrameListener needs access to the Http2ConnectionHandler (or the encoder contained within) to be able to send responses. However the Http2ConnectionHandler requires a Http2FrameListener instance to be passed in during construction time. This creates a cyclic dependency which can make it difficult to cleanly accomplish this relationship.

Modifications:
- Add Http2ConnectionDecoder.frameListener(..) method to set the frame listener. This will allow the listener to be set after construction.

Result:
Classes which inherit from Http2ConnectionHandler can more cleanly set the Http2FrameListener.
2015-09-30 15:41:15 -07:00
Scott Mitchell
0e9545e94d Http2RemoteFlowController stream writibility listener
Motivation:
For implementations that want to manage flow control down to the stream level it is useful to be notified when stream writability changes.

Modifications:
- Add writabilityChanged to Http2RemoteFlowController.Listener
- Add isWritable to Http2RemoteFlowController

Result:
The Http2RemoteFlowController provides notification when writability of a stream changes.
2015-09-28 13:47:24 -07:00
nmittler
7ab132f28a Making HTTP/2 stream byte assignment pluggable
Motivation:

The DefaultHttp2RemoteFlowController has become very large and is getting difficult to understand and maintain. It is also desirable for some applications to be able to disable the priority algorithm altogether for performance reasons.

Modifications:

Abstract the stream byte assignment logic (renamed allocation->assignment for clarity) behind an interface `StreamByteAssigner` with a single implementation `PriorityStreamByteAssigner`.

Result:

Goes some way towards supporting #4246
2015-09-25 14:00:12 -07:00
Scott Mitchell
93011dd315 DefaultHttp2RemoteFlowController not allocating all available bytes
Motivation:
DefaultHttp2RemoteFlowController's allocation algorithm may not allocate all bytes that are available in the connection window. If the 'fair share' based upon weight is not fully used by sibling nodes it was not correctly re-distributed to other sibilings which may be able to utilize part / all of that share.

Modifications:
- Add a unit test which demonstrates the issue.
- Modify the allocation algorithm to ensure all available bytes are allocated.

Result:
Fixes https://github.com/netty/netty/issues/4266
2015-09-25 11:12:04 -07:00
Scott Mitchell
c372f69118 http2.HttpConversionUtil :authority conversion error
Motiviation:
The http2 spec https://tools.ietf.org/html/rfc7540#section-8.1.2.3 states that the :authority header should be copied into the HOST header when converting from HTTP/2 to HTTP/1.x. We currently have an extension header to preserve the authority.

Modifications:
- Remove AUTHORITY extension header
- HTTP/2 :authority should map to HOST header when converting to HTTP/1.x.

Result:
More spec compliant.
2015-09-23 17:06:52 -07:00
Scott Mitchell
ed4928f62a StreamBufferingEncoderTest leak
Motivation:
Buffer leak in StreamBufferingEncoderTest

Modifications:
- Make sure buffers are released in StreamBufferingEncoderTest

Result:
Fixes https://github.com/netty/netty/issues/4230
2015-09-23 16:49:39 -07:00
Scott Mitchell
edb91afcd6 Http2LifecycleManager.onException rename
Motivation:
Http2LifecycleManager.onException takes a Throwable as a paramter and not an Exception. There are also onConnectionError and onStreamError methods in the codec. We should rename this method to onError for consistency and clarity.

Modifications:
- Rename Http2LifecycleManager.onException to Http2LifecycleManager.onError

Result:
More consistent and clarified interface.
2015-09-23 16:48:36 -07:00
Scott Mitchell
24c9407080 DefaultHttp2RemoteFlowController may not write all pending bytes
Motivation:
DefaultHttp2RemoteFlowController attempts to write as many bytes as possible to transition the channel to not writable, and then relies on notification of channelWritabilityChange to continue writing. However the amount of bytes written by DefaultHttp2RemoteFlowController may not be the same number of bytes that is actually written to the channel due to other ChannelHandlers (SslHandler, compression, etc...) in the pipeline. This means there is a potential for the DefaultHttp2RemoteFlowController to be waiting for a channel writaiblity change event that will never come, and thus not write all queued data.

Modifications:
- DefaultHttp2RemoteFlowController should write pending bytes until there are no more, or until the channel is not writable.

Result:
DefaultHttp2RemoteFlowController will write all pending data.
Fixes https://github.com/netty/netty/issues/4242
2015-09-23 16:39:24 -07:00
nmittler
ec20902613 Don't set HTTP/2 flow controller ctx to null
Motivation:

We currently set the flow controller ChannelHandlerContexts to null when the channel becomes inactive. This is bad :)

Modifications:

Just remove that code in Http2ConnectionHandler

Result:

Fixes #4240
2015-09-22 07:25:42 -07:00
fratboy
6241bb059c [#4244] Convert urlencoded uri to http2 path correctly
Motivation:

HttpConversionUtil.toHttp2Headers does not convert urlencoded uri to http2 path properly.

Modifications:

Use getRawPath(), getRawQuery(), getRawFragment() in java.net.URI when converts to http2 path

Result:

HttpConversionUtil.toHttp2Headers does not urldecode uri unproperly.
2015-09-21 16:30:59 -07:00
Scott Mitchell
c7e3f6c6fd HTTP/2 defines using String instead of CharSequence
Motivation:
Http2CodecUtils has some static variables which are defined as Strings instead of CharSequence. One of these defines is used as a header name and should be AsciiString.

Modifications:
- Change the String defines in Http2CodecUtils to CharSequence

Result:
Types are more consistently using CharSequence and adding the upgrade header will require less work.
2015-09-16 14:55:33 -07:00
Scott Mitchell
1d4d5fe312 DefaultHttp2Headers should throw exception of type Http2Exception
Motivation:
The DefaultHttp2Headers code is throwing a IllegalArgumentException if an invalid character is detected. This is being ignored by the HTTP/2 codec instead of generating a GOAWAY.

Modifications:
- Throw a Http2Exception of type PROTOCOL_ERROR in accordance with https://tools.ietf.org/html/rfc7540#section-8.1.2.6
- Update examples which were building invalid headers

Result:
More compliant with https://tools.ietf.org/html/rfc7540#section-8.1.2.6
2015-09-16 13:47:05 -07:00
Scott Mitchell
15450af2e7 DefaultHttp2FrameWriter ping payload size check
Motivation:
The HTTP/2 spec states that the ping frame length must be 8 and is otherwise an error https://tools.ietf.org/html/rfc7540#section-6.7. The DefaultHttp2FrameReader enforces this, but the DefaultHttp2FrameWriter allows invalid frames to be written. We should not allow invalid ping frames to be written to the network.

Modifications:
- DefaultHttp2FrameWriter checks the frame size to be 8, or throws an exception

Result:
Fixes https://github.com/netty/netty/issues/3721
2015-09-16 10:25:07 -07:00
Scott Mitchell
59600f1812 HTTP/2 to HTTP/1.x headers conversion more accessible
Motivation:
Currently there is a HttpConversionUtil.addHttp2ToHttpHeaders which requires a FullHttpMessage, but this may not always be available. There is no interface that can be used with just Http2Headers and HttpHeaders.

Modifications:
- add an overload for HttpConversionUtil.addHttp2ToHttpHeaders which does not take FullHttpMessage

Result:
An overload for HttpConversionUtil.addHttp2ToHttpHeaders exists which does not require FullHttpMessage.
2015-09-16 10:02:48 -07:00
Scott Mitchell
ba11879c9f HTTP/2 codec heap buffer usage
Motivation:
The HTTP/2 codec has a few static buffers sent over the network which are allocated on the heap. This results in a copy operation when the buffer is sent out on the network.

Modifications:
- Ensure these static buffers are allocated using direct memory.

Result:
No copy operation necessary when writing static buffers to network.
2015-09-14 13:12:02 -07:00
Scott Mitchell
f89dfb0bd5 Deprecation cleanup for HTTP headers
Motivaion:
The HttpHeaders and DefaultHttpHeaders have methods deprecated due to being removed in future releases, but no replacement method to use in the current release. The deprecation policy should not be so aggressive as to not provide any non-deprecated method to use.

Modifications:
- Remove deprecated annotations and javadocs from methods which are the best we can do in terms of matching the master's api for 4.1

Result:
There should be non-deprecated methods available for HttpHeaders in 4.1.
2015-09-09 14:30:21 -07:00