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