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.
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
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.
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.
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.
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.
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
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
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.
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
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.
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.
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.
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
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
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.
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.
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.
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.
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.
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.
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>
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.
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.
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.
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
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
