Motivation:
The HTTP/2 specification allows for closed (and streams in any state) to exist in the priority tree. The current code removes streams from the priority tree as soon as they are closed (subject to the removal policy). This may lead to undesired distribution of resources from the peer's perspective.
Modifications:
- We should only remove streams from the priority tree when they have no descendant streams in a viable state.
- We should track when tree edges change or nodes are removed if inviable nodes can then be removed.
Result:
Priority tree doesn't remove closed streams until descendant are all closed, or there are no descendants.
Motivation:
We're currently using Math.ceil which isn't necessary. We should exchange for a lighter weight operation.
Modifications:
Changing the logic to just ensure that we allocate at least one byte to the child rather than always performing a ceil.
Result:
Slight performance improvement in the priority algorithm.
Motivation:
The Connection.Listener GOAWAY event handler currently provides no additional information, requiring applications to hack in other ways to get at the error code and debug message.
Modifications:
Modified the Connection.Listener interface to pass on the error code and message that triggered the GOAWAY.
Result:
Application can now use Connection.Listener for all GOAWAY processing.
Motivation:
It currently takes a builder for the encoder and decoder, which makes it difficult to decorate them.
Modifications:
Removed the builders from the interfaces entirely. Left the builder for the decoder impl but removed it from the encoder since it's constructor only takes 2 parameters. Also added decorator base classes for the encoder and decoder and made the CompressorHttp2ConnectionEncoder extend the decorator.
Result:
Fixes#3530
Motivation:
The DefaultHttp2RemoteFlowController's priority algorithm doesn't really need to sort the children by weight since it already fairly distributes data based on weight.
Modifications:
Removing the sorting in the priority algorithm and updating one test to allow a small bit of variability in the results.
Result:
Slight improvement on the performance of the priority algorithm.
Motivation:
The encoder/decoder currently do not handle streams which have previously existed but no longer exist because they were closed. The specification requires supporting this.
Modifications:
- encoder/decoder should tolerate the frame or the dependent frame not existing in the streams map due to the fact that it may have previously existed.
Result:
encoder/decoder are more compliant with the specification.
Motivation:
The DefaultHttp2ConnectionDecoder class is calling verifyPrefaceReceived() for almost every frame event at all times.
The Http2ConnectionHandler class is calling readClientPrefaceString() on every decode event.
Modifications:
- DefaultHttp2ConnectionDecoder should not have to continuously call verifyPrefaceReceived() because it transitions boolean state 1 time for each connection.
- Http2ConnectionHandler should not have to continuously call readClientPrefaceString() because it transitions boolean state 1 time for each connection.
Result:
- Less conditional checks for the mainstream usage of the connection.
Motivation:
The current DefaultHttp2RemoteFlowController's writePendingBytes currently operates in 2 passes. The first allocates bytes and optionally writes some frames. The second pass just loops across all active streams and writes all remaining bytes.
If streams can be removed/added as a side effect of writing (EOS or error) then we need to take more care when the write actually occurs. Moving all of the writes to the second loop (across active streams) is simpler since we can just make a copy of the list and not worry about any restructuring of the priority tree that may result.
Modifications:
Modified DefaultHttp2RemoteFlowController.writePendingBytes to only allocate bytes on the first pass and then write any allocated bytes on the second pass.
Result:
Side effects resulting from writing should no longer impact the flow control algorithm.
Motivation:
A microbenchmark will be useful to get a baseline for performance.
Modifications:
- Introduce a new microbenchmark which tests the Http2DefaultFrameWriter.
- Allow benchmarks to run without thread context switching between JMH and Netty.
Result:
Microbenchmark exists to test performance.
Motivation:
The Http2ConnectionHandler writeRstStream method allows RST_STREAM frames to be sent when we do not know about the stream and after a RST_STREAM frame has already been sent. This may lead to sending frames when we should not according to the HTTP/2 spec. There is also the potential to notify the closeListener multiple times if the closeStream method is called multiple times.
Modifications:
- Prevent RST_STREAM from being sent if we don't know about the stream, or if we already sent the RST_STREAM.
- Prevent the closeListener from being notified multiple times.
Result:
More robust writeRstStream logic in boundary conditions.
Motivation:
There was a new draft for HTTP/2. We should support the new draft.
Modifications:
- Review the HTTP/2 draft 17 specification, and update code to reflect changes.
Result:
Support for HTTP/2 draft 17.
Motivation:
- In FlowState.write(...) we are currently swalloing an exception.
- In my previous commit I introduced a compiler warning by not making
a local variabe final.
Modifications:
- Have FlowState.cancel() take a Throwable.
- Make the variable final.
Result:
No more swallowed exceptions and warnings.
Motivation:
- The encoder and decoder should be closed right after the handler releases its resources.
- The clientPrefaceString is allocated in the constructor but releases in handlerRemoved.
If the handler is never added to the pipeline, the clientPrefaceString will never be
released.
Modifications:
- Call encoder.close() and decoder.close() on channelInactive.
- Release the clientPrefaceString on handlerRemoved.
Result:
- The encoder and decoder get closed right after the handler's resources are freed.
- It's easier to verify that the clientPrefaceString will also get released.
Motivation:
The Http2FrameLogger is currently using the internal logging classes. We should change this so that it's using the public classes and then converts internally.
Modifications:
Modified Http2FrameLogger and the examples to use the public LogLevel class.
Result:
Fixes#2512
Motivation:
The current documentation for Endpoint methods referring to concurrent streams and the SETTINGS_MAX_CONCURRENT_STREAMS setting are a bit confusing.
Modifications:
Renamed a few of the methods and added more clear documentation.
Result:
Fixes#3451
Motivation:
There are two writeRstStream methods in the DefaultHttp2ConnectionEncoder.
One of the two is neither used nor part of the Http2FrameWriter interface.
Modifications:
Delete the method.
Result:
Fewer lines of dead code.
Motivation:
For every write of a flow controlled frame (HEADERS, DATA) we are allocating
a Frame object that is not necessary anymore as it does not maintain any
state, besides the payload.
Modifications:
Remove the Frame class and directly add the payload to the pending write queue.
Result:
One few object allocation per write of a flow controlled frame.
Motivation:
The logger was always performing a hex dump of the ByteBufs regarless whether or not the log would take place.
Modifications:
Fixed the logger to avoid serializing the ByteBufs and calling the varargs method if logging is not enabled.
Result:
The loggers should run MUCH faster when disabled.
Motivation:
If HEADERS or DATA frames are pending due to a too small flow control
window, a frame with the END_STREAM flag set will wrongfully cancel
all pending frames (including itself).
Also see grpc/grpc-java#145
Modifications:
The transition of the stream state to CLOSE / HALF_CLOSE due to a
set END_STREAM flag is delayed until the frame with the flag is
actually written to the Channel.
Result:
Flow control works correctly. Frames with END_STREAM flag will no
longer cancel their preceding frames.
Motivation:
HttpToHttp2ConnectionHandlerTest was accidentally modified with a
debugging value for WAIT_TIME_SECONDS.
Modifications:
Reverted the change.
Result:
original wait time restored.
Motivation:
The Http2DefaultFrameWriter copies all contents into a buffer (or uses a CompositeBuffer in 1 case) and then writes that buffer to the socket. There is an opportunity to avoid the copy operations and write directly to the socket.
Modifications:
- Http2DefaultFrameWriter should avoid copy operations where possible.
- The Http2FrameWriter interface should be clarified to indicate that ByteBuf objects will be released.
Result:
Hopefully less allocation/copy leads to memory and throughput performance benefit.
Motivation:
Http2Stream has several methods that provide state information. We need
to simplify how state is used and consolidate as many of these fields as
possible.
Modifications:
Since we already have a concept of a stream being active or inactive,
I'm now separating the deactivation of a stream from the act of closing
it. The reason for this is the case of sending a frame with
endOfStream=true. In this case we want to close the stream immediately
in order to disallow further writing, but we don't want to mark the
stream as inactive until the write has completed since the inactive
event triggers the flow controller to cancel any pending writes on the
stream.
With deactivation separated out, we are able to eliminate most of the
additional state methods with the exception of `isResetSent`. This is
still required because we need to ignore inbound frames in this case (as
per the spec), since the remote endpoint may not yet know that the
stream has been closed.
Result:
Fixes#3382
Motivation:
Previously flow-controller had to know the implementation details of each frame type in order to write it correctly. That concern is more correctly handled by the encoder. By encapsulating the payload types to be flow-controlled it will be easier to add support for extension types later. This change also fixes#3353.
Modifications:
Add interface FlowControlled which is now delivered to flow-controller.
Implement this interface for HEADERS and DATA
Refactor and improve tests for flow-control.
Result:
Flow control semantics are more cleanly separated for data encoding and implementation is simpler overall.
Motivation:
A downstream consumer of Netty failed as emitting zero-length http2 data frames in a unit test resulted in assertion errors in Http2LocalFlowController. Since zero-length frames are valid, an assertion that http2 data frame length must be positive is invalid.
Modifications:
Assertions of data length in Http2LocalFlowController now permit zero.
Result:
Those running netty with assertions on can now emit zero length http2 data frames.
Motivation:
Commit eb0e127ee9 was designed for the 4.1 branch. The 5.0 branch had a few additional classes which required updates to get the new timeMillis methods for all the header classes.
Modifications:
Update the *Headers interfaces/classes that were not updated in the 4.1 branch.
Result:
All *Headers interfaces support set/add timeMillis methods.
Motivation:
HttpToHttp2ConnectionHandler is awaiting on a future and a latch that may be competed before the buffers actually get released. This test is attempting to validate that the buffer's refCnt() is 0 but there is no mechanism to wait on for a buffer's release() method to be called.
Modifications:
Remove the buffer refCnt() check. The leak profile is designed to pick these up.
Result:
Unit tests that no longer have a race condition.
Motivation:
The stress tests have been observed to fail on the CI server. The average run time of the stress tests has recently been 26+ seconds. Our timeout is currently set to 30 seconds.
Modifications:
Increase the timeout for the stress test so when the leak profile is active we will have more time to complete the test (with the additional overhead).
Result:
Stress tests fail less frequently (hopefully not at all).
Motivation:
Clinker provides a Sonar tool which detects potential bugs or problems in the code. These problems were reported here http://clinker.netty.io/sonar/drilldown/issues/io.netty:netty-parent:master?
Modifications:
Make the recommended changes as reported by Sonar
Result:
Better or more standard code. Less Sonar problem reports for HTTP/2 codec.
Motivation:
The Http2ConnectionRoundtripTest.noMoreStreamIdsShouldSendGoAway unit test had a race condition where it would sometimes receive a SETINGS_ACK message that was not anticipated. This caused the test to fail because of bad test code.
Modifications:
The bad unit test should be updated to handle the message exchange for a good connection setup, and then the GO_AWAY frame.
Result:
Http2ConnectionRoundtripTest.noMoreStreamIdsShouldSendGoAway should no longer sporadically fail.
Motivation:
The terminology used with inbound/outbound is a little confusing since
it's not discussed in the spec. We should switch to using local/remote
instead. Also there is some asymmetry between the inbound/outbound
interfaces which could probably be cleaned up.
Modifications:
Changing the interface names and making a common Http2FlowController
interface for most of the methods.
Result:
The HTTP/2 flow control interfaces should be more clear.
Motivation:
The inbound flow control code was returning too many bytes to the connection window. This was resulting in GO_AWAYs being generated by peers with the error code indicating a flow control issue. Bytes were being returned to the connection window before the call to returnProcessedBytes. All of the state representing the connection window was not updated when a local settings event occurred.
Modifications:
The DefaultHttp2InboundFlowController will be updated to correct the above defects.
The unit tests will be updated to reflect the changes.
Result:
Inbound flow control algorithm does not cause peers to send flow control errors for the above mentioned cases.
Motivation:
We use 3 (!) libraries to build mock objects - easymock, mockito, jmock.
Mockito and jMock pulls in the different versions of Hamcrest, and it
conflicts with the version pulled by jUnit.
Modifications:
- Replace mockito-all with mockito-core to avoid pulling in outdated
jUnit and Hamcrest
- Exclude junit-dep when pulling in jmock-junit4, because it pulls an
outdated Hamcrest version
- Pull in the hamcrest-library version used by jUnit explicitly
Result:
No more dependency hell that results in NoSuchMethodError during the
tests
Motivation:
The Http2SecurityUtil class lists a few ciphers that are explicitly prohibited by the HTTP/2 specification because of their characteristics.
Modifications:
Remove the ciphers that are prohibited.
Results:
Cipher suite used for HTTP/2 codec is compatible with HTTP/2 spec.
Motivation:
The DefaultHttp2FrameWriter has an exception generated but is missing the throw keyword.
Modifications:
Insert the missing throw keyword.
Result:
Exception thrown when it was intended to be thrown.
Motivation:
The DefaultHttp2InboundFlowController uses processedBytes to determine
when to send the WINDOW_UPDATE, but uses window to determine the delta
to send in the request. This is incorrect since we shouldn't be
requesting bytes that haven't been processed.
Modifications:
Changed DefaultHttp2InboundFlowController to use processedBytes in the
calculation of the delta to send in the WINDOW_UPDATE request.
Result:
Inbound flow control only asks for bytes that have been processed in
WINDOW_UPDATE.
Related: #3157
Motivation:
It should be convenient to have an easy way to classify an
HttpResponseStatus based on the first digit of the HTTP status code, as
defined in the RFC 2616:
- Information 1xx
- Success 2xx
- Redirection 3xx
- Client Error 4xx
- Server Error 5xx
Modification:
- Add HttpStatusClass
- Add HttpResponseStatus.codeClass() that returns the class of the HTTP
status code
- Remove HttpResponseStatus.isInformational()
Result:
It's easier to determine the class of an HTTP status
Motivation:
The HTTP/2 compressor does not release the input buffer when compression is done. This results in buffer leaks.
Modifications:
- Release the buffer in the HTTP/2 compressor
- Update tests to reflect the correct state
Result:
1 less buffer leak.
Motivation:
The interface for HTTP/2 onDataRead states that buffers will be released by the codec. The decompressor and compressor methods are not releasing buffers created during the decompression/compression process.
Modifications:
After onDataRead calls the decompressor and compressor classes will release the data buffer.
Result:
HTTP/2 compressor/decompressors are consistent with onDataRead interface assumptions.
Motivation:
Some of the comments in HTTP/2 Frame Listener interface are misleading.
Modifications:
Clarify comments in Http2FrameListener.
Result:
Http2FrameListener onDataRead comments are clarified.
Motivation:
When DefaultHttp2FrameReader has read a settings frame, the settings
will be passed along the pipeline. This allows a client to hold off
sending data until it has received a settings frame. But for a server it
will always have received a settings frame and the usefulness of this
forwarding of settings is less useful. This also causes a debug message
to be logged on the server side if there is no channel handler to handle
the settings:
[nioEventLoopGroup-1-1] DEBUG io.netty.channel.DefaultChannelPipeline -
Discarded inbound message {INITIAL_WINDOW_SIZE=131072,
MAX_FRAME_SIZE=16384} that reached at the tail of the pipeline. Please
check your pipeline configuration.
Modifications:
Added a builder for the InboundHttp2ToHttpAdapter and
InboundHttp2PriortyAdapter and a new parameter named 'propagateSettings'
to their constructors.
Result:
It is now possible to control whether settings should be passed along
the pipeline or not.
Motivation:
Found performance issues via FindBugs and PMD.
Modifications:
- Removed unnecessary boxing/unboxing operations in DefaultTextHeaders.convertToInt(CharSequence) and DefaultTextHeaders.convertToLong(CharSequence). A boxed primitive is created from a string, just to extract the unboxed primitive value.
- Added a static modifier for DefaultHttp2Connection.ParentChangedEvent class. This class is an inner class, but does not use its embedded reference to the object which created it. This reference makes the instances of the class larger, and may keep the reference to the creator object alive longer than necessary.
- Added a static compiled Pattern to avoid compile it each time it is used when we need to replace some part of authority.
- Improved using of StringBuilders.
Result:
Performance improvements.
Motivation:
The HTTP/2 codec currently does not expose the boundaries of the initial settings. This could be useful for applications to define their own initial settings.
Modifications:
Add new static final variables to Http2CodecUtil and make Http2Settings use these in the bounds checks.
Result:
Applications can use the max (or min) variables to initialize their settings.
Motivation:
When the inbound flow controller recognizes that the flow control window
has been violated on a stream (not connection-wide), it throws a
connection error.
Modifications:
Changed the DefaultHttp2InboundFlowController to properly throw
connection error if the connection window is violated and stream error
if a stream window is violated.
Result:
inbound flow control throws the correct error for window violations.
Motivation:
The current name of the class which converts from HTTP objects to HTTP/2 frames contains the text Http2ToHttp. This is misleading and opposite of what is being done.
Modifications:
Rename this class name to be HttpToHttp2.
Result:
Class names that more clearly identify what they do.
Motivation:
The current decompression frame listener currently opts-out of application level flow control. The application should still be able to control flow control even if decompression is in use.
Modifications:
- DecompressorFrameListener will maintain how many compressed bytes, decompressed bytes, and processed by the listener bytes. A ratio will be used to translate these values into application level flow control amount.
Result:
HTTP/2 decompressor delegates the application level flow control to the listener processing the decompressed data.
