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.
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
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
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.
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.
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
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
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.
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.
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.
Motivation:
The HTTP/2 header name validation was removed, and does not currently exist.
Modifications:
- Header name validation for HTTP/2 should be restored and set to the default mode of operation.
Result:
HTTP/2 header names are validated according to https://tools.ietf.org/html/rfc7540
Motivation:
The javadoc comments on Http2Headers.iterator() are incorrect.
Modifications:
- Correct and clarify the javadoc for Http2Headers.iterator()
Result:
Javadoc for Http2Headers.iterator() is more correct.
Motivation:
InboundHttp2ToHttpAdapterTest.bootstrapEnv does not wait for the serverConnectedChannel to be initialized before returning. Some methods rely only this behavior and throw a NPE because it may not be set.
Modifications:
- Add a CountDownLatch to ensure the serverConnectedChannel is initialized
Result:
No more NPE.
Motivation:
The SimplePromiseAggregator.setFailure allows a failure to occur before newPromise is called, but tryFailure doesn't. These methods should be consistent.
Modifications:
- tryFailure should use the same logic as setFailure
Result:
Consistent failure routines.
Motivation:
DefaultPropertyKey.index is currently private and accessed outside the class's scope.
Modifications:
- Change access level to package private
Result:
No chance of synthetic method generation for accessing this field
Motivation:
The latches in InboundHttp2ToHttpAdapterTest were volatile and reset during the tests. This resulted in race conditions and sometimes the tests would be waiting on old latches that were not the same latches being counted down when messages were received.
Modifications:
- Remove volatile latches from tests
Result:
More reliable tests with less race conditions.
Motivation:
There currently exists http.HttpUtil, http2.HttpUtil, and http.HttpHeaderUtil. Having 2 HttpUtil methods can be confusing and the utilty methods in the http package could be consolidated.
Modifications:
- Rename http2.HttpUtil to http2.HttpConversionUtil
- Move http.HttpHeaderUtil methods into http.HttpUtil
Result:
Consolidated utilities whose names don't overlap.
Fixes https://github.com/netty/netty/issues/4120
Motivation:
ByteToMessageDecoder may call decode after channelInactive is called. This will lead to a NPE.
Modifications:
- Call super.channelInactive() before we process the event in Http2ConnectionHandler
Result:
No more NPE in decode.
Motivation:
Commit 0d8ce23c83 failed to fix the Host header processing. Host is not a URI but is instead defined in https://tools.ietf.org/html/rfc3986#section-3.2.2 as host = IP-literal / IPv4address / reg-name
Modifications:
- Host should not be treated as a URI.
- We should be more explicit about required fields, and unexpected input by throwing exceptions.
Result:
Translation from HTTP/1.x to HTTP/2 is more correct.
Motivation:
If a SimpleChannelPromiseAggregator is failed before any new promises are generated, the failure is not propegated through to the aggregated promise.
Modifications:
- Failures should be allowed to occur even if no new promises have been generated
Result:
Failures are always allowed.
Motivation:
If any streams are still active the graceful shutdown code will wait until they are all closed before the connection is closed. In some situations this event may never occur, and thus a timeout should be supported so the socket can be closed even if all streams haven't been closed.
Modifications:
- Add a configurable timeout for when the graceful shutdown process is attempted.
- Update unit tests to be faster, and use this graceful timeout
Result:
Local endpoint can protect from local or remote issues which prevent the channel from being closed during the graceful shutdown process.
Motivation:
DefaultHttp2ConnectionEncoder.FlowControlledHeaders and DefaultHttp2ConnectionEncoder.FlowControlledData have private constructors which may result in static factory methods being generated to construct instances of these classes.
Modifications:
- Make constructors public for these private classes
Result:
Accessor for inner class constructor more correct and no possibiliy of synthetic method generation.
Motivation:
A degradation in performance has been observed from the 4.0 branch as documented in https://github.com/netty/netty/issues/3962.
Modifications:
- Simplify Headers class hierarchy.
- Restore the DefaultHeaders to be based upon DefaultHttpHeaders from 4.0.
- Make various other modifications that are causing hot spots.
Result:
Performance is now on par with 4.0.
Motivation:
The Http2ConnectionHandler was writing pending bytes, but was not flushing. This may result in deadlock.
Modifications:
- Http2ConnectionHandler must writePendingBytes and also flush.
Result:
Data is now flushed after writabilityChange writes more data to underlying layers.
Motivation:
We noticed that the headers implementation in Netty for HTTP/2 uses quite a lot of memory
and that also at least the performance of randomly accessing a header is quite poor. The main
concern however was memory usage, as profiling has shown that a DefaultHttp2Headers
not only use a lot of memory it also wastes a lot due to the underlying hashmaps having
to be resized potentially several times as new headers are being inserted.
This is tracked as issue #3600.
Modifications:
We redesigned the DefaultHeaders to simply take a Map object in its constructor and
reimplemented the class using only the Map primitives. That way the implementation
is very concise and hopefully easy to understand and it allows each concrete headers
implementation to provide its own map or to even use a different headers implementation
for processing requests and writing responses i.e. incoming headers need to provide
fast random access while outgoing headers need fast insertion and fast iteration. The
new implementation can support this with hardly any code changes. It also comes
with the advantage that if the Netty project decides to add a third party collections library
as a dependency, one can simply plug in one of those very fast and memory efficient map
implementations and get faster and smaller headers for free.
For now, we are using the JDK's TreeMap for HTTP and HTTP/2 default headers.
Result:
- Significantly fewer lines of code in the implementation. While the total commit is still
roughly 400 lines less, the actual implementation is a lot less. I just added some more
tests and microbenchmarks.
- Overall performance is up. The current implementation should be significantly faster
for insertion and retrieval. However, it is slower when it comes to iteration. There is simply
no way a TreeMap can have the same iteration performance as a linked list (as used in the
current headers implementation). That's totally fine though, because when looking at the
benchmark results @ejona86 pointed out that the performance of the headers is completely
dominated by insertion, that is insertion is so significantly faster in the new implementation
that it does make up for several times the iteration speed. You can't iterate what you haven't
inserted. I am demonstrating that in this spreadsheet [1]. (Actually, iteration performance is
only down for HTTP, it's significantly improved for HTTP/2).
- Memory is down. The implementation with TreeMap uses on avg ~30% less memory. It also does not
produce any garbage while being resized. In load tests for GRPC we have seen a memory reduction
of up to 1.2KB per RPC. I summarized the memory improvements in this spreadsheet [1]. The data
was generated by [2] using JOL.
- While it was my original intend to only improve the memory usage for HTTP/2, it should be similarly
improved for HTTP, SPDY and STOMP as they all share a common implementation.
[1] https://docs.google.com/spreadsheets/d/1ck3RQklyzEcCLlyJoqDXPCWRGVUuS-ArZf0etSXLVDQ/edit#gid=0
[2] https://gist.github.com/buchgr/4458a8bdb51dd58c82b4
Motivation:
The DataCompressionHttp2Test was exiting prematurely leading to unit test failures.
Modifications:
- Fix the race condition so the test does not evaluate final conditions until all expected events occur
Result:
Unit test no longer fails
Motivation:
When looking through the logs for entries pertaining to a specific stream, it's difficult because header entries use the syntax "streamId:<id>" but all other entries use "streamId=<id>". We should make all of the entries consistent.
Modifications:
Changed header entries to use "streamId=<id>" to match the other entries.
Result:
Easier HTTP/2 log navigation.
Motivation:
We should support XXXCollections methods for all primitive map types.
Modifications:
Removed PrimitiveCollections and added a template for XXXCollections.
Result:
Fixes#4001
Motivation:
It would be useful to support the Java `Map` interface in our primitive maps.
Modifications:
Renamed current methods to "pXXX", where p is short for "primitive". Made the template for all primitive maps extend the appropriate Map interface.
Result:
Fixes#3970
Motivation:
HttpToHttp2ConnectionHandler only converts FullHttpMessage to HTTP/2 Frames. This does not support other use cases such as adding a HttpContentCompressor to the pipeline, which writes HttpMessage and HttpContent.
Additionally HttpToHttp2ConnectionHandler ignores converting and sending HTTP trailing headers, which is a bug as the HTTP/2 spec states that they should be sent.
Modifications:
Update HttpToHttp2ConnectionHandler to support converting HttpMessage and HttpContent to HTTP/2 Frames.
Additionally, include an extra call to writeHeaders if the message includes trailing headers
Result:
One can now write HttpMessage and HttpContent (http chunking) down the pipeline and they will be converted to HTTP/2 Frames. If any trailing headers exist, they will be converted and sent as well.
Motivation:
The bufferingNewStreamFailsAfterGoAwayReceived method currently causes an NPE.
Modifications:
Fixed the test so that a valid ByteBuf is passed in.
Result:
The test no longer throws an NPE.
Motivation:
It is currently assumed that all usages of the HTTP/2 codec will be from the same event loop context. If the methods are used outside of the assumed thread context then unexpected behavior is observed. This assumption should be more clearly communicated and enforced in key areas.
Modifications:
- The flow controller interfaces have assert statements and updated javadocs indicating the assumptions.
Result:
Interfaces more clearly indicate thread context limitations.
Motivation:
The CompressorHttp2ConnectionEncoder is attempting to attach a property to streams before the exist.
Modifications:
- Allow the super class to create the streams before attempting to attach a property to the stream.
Result:
CompressorHttp2ConnectionEncoder is able to set the property and access the compressor.
Motivation:
See #3783
Modifications:
- The DefaultHttp2RemoteFlowController should use Channel.isWritable() before attempting to do any write operations.
- The Flow controller methods should no longer take ChannelHandlerContext. The concept of flow control is tied to a connection and we do not support 1 flow controller keeping track of multiple ChannelHandlerContext.
Result:
Writes are delayed until isWritable() is true. Flow controller interface methods are more clear as to ChannelHandlerContext restrictions.
Motivation:
The MAX_HEADER_LIST_SIZE of SETTINGS is represented by
unsigned 32-bit value and this value isn't limited in RFC7540.
But in current implementation, its stored to int variable so
over 2^31-1 value is recognized as minus and handled as
PROTOCOL_ERROR.
Modifications:
If a value of MAX_HEADER_LIST_SIZE is larger than 2^31-1, its
handled as 2^31-1
Result:
Over 2^31-1 MAX_HEADER_LIST_SIZE is became acceptable
Motivation:
Slicing a mutable CompositeByteBuf is not the appropriate mechanism to use to track and release buffers that have been written to a channel.
In particular buffers passed over an Embedded or LocalChannel are retained after the ChannelPromise is completed and listening to the
promise to consolidate a CompositeBuffer breaks slices taken from the composite as the offset indices have changed.
In addition CoalescingBufferQueue handles taking arbitrarily sized slices of a sequence of buffers more efficiently.
Modifications:
Convert FlowControlledData to use a CoalescingBufferQueue to handle merging data writes.
Result:
HTTP2 works over LocalChannel and code is considerably simpler.
Motivation:
Sometimes people use a data frame with length 0 to end a stream(such as jetty http2-server). So it is possible that data.readableBytes and padding are all 0 for a data frame, and cause an IllegalArgumentException when calling flowController.consumeBytes.
Modifications:
Return false when numBytes == 0 instead of throwing IllegalArgumentException.
Result:
Fix IllegalArgumentException.
Motivation:
The problem is described in https://github.com/grpc/grpc-java/issues/605. Basically, when using `StreamBufferingEncoder` there is a chance of creating zombie streams that never get closed.
Modifications:
Change `Http2ConnectionHandler`'s `channelInactive` handling logic to shutdown the encoder/decoder before shutting down the active streams.
Result:
Fixes https://github.com/grpc/grpc-java/issues/605
Motivation:
Http2ConnectionHandler missed to forward channelReadComplete(...) events.
Modifications:
Ensure we notify the next handler in the pipeline via ctx.fireChannelReadComplete().
Result:
Correctly forwarding of event.
Motivation:
The Http2FrameListener requires that the Http2FrameReader accumulate ByteBuf objects. There is currently no way to limit the amount of bytes that is accumulated.
Motiviation:
- DefaultHttp2FrameReader supports maxHeaderSize which will fail fast as soon as the maximum size is exceeded.
- DefaultHttp2HeadersDecoder will respect the return value of endHeaderBlock() and fail if the max size is exceeded.
Result:
Frames which carry header data now respect a maximum number of bytes that can be accumulated.
Motivation:
If server sends SETTINGS with ENABLE_PUSH, its handled as
PROTOCOL_ERROR in spite of the value. But the value specified to
0 may be allowed in RFC7540.
Modifications:
Check whether ENABLE_PUSH sent from a server is 0 or not.
Result:
When server specifies ENABLE_PUSH to 0 explicitly, client doesn't
handle it as PROTOCOL_ERROR.
Motivation:
NPE doesn't cause the tests to fail but should get fixed.
Modifications:
Modified the StreamBufferingEncoderTest to mock the ctx to return a promise.
Result:
Fixes#3905
Motivation:
Coalescing many small writes into a larger DATA frame reduces framing overheads on the wire and reduces the number of calls to Http2FrameListeners on the remote side.
Delaying the write of WINDOW_UPDATE until flush allows for more consumed bytes to be returned as the aggregate of consumed bytes is returned and not the amount consumed when the threshold was crossed.
Modifications:
- Remote flow controller no longer immediately writes bytes when a flow-controlled payload is enqueued. Sequential data payloads are now merged into a single CompositeByteBuf which are written when 'writePendingBytes' is called.
- Listener added to remote flow-controller which observes written bytes per stream.
- Local flow-controller no longer immediately writes WINDOW_UPDATE when the ratio threshold is crossed. Now an explicit call to 'writeWindowUpdates' triggers the WINDOW_UPDATE for all streams who's ratio is exceeded at that time. This results in
fewer window updates being sent and more bytes being returned.
- Http2ConnectionHandler.flush triggers 'writeWindowUpdates' on the local flow-controller followed by 'writePendingBytes' on the remote flow-controller so WINDOW_UPDATES preceed DATA frames on the wire.
Result:
- Better throughput for writing many small DATA chunks followed by a flush, saving 9-bytes per coalesced frame.
- Fewer WINDOW_UPDATES being written and more flow-control bytes returned to remote side more quickly, thereby improving throughput.
Motivation:
Bootstrap of the HTTP/2 can take a lot of paths and a lot of things can go wrong in the initial handshakes leading up to establishment of HTTP/2 between client and server. There have been many times where handshakes have failed silently, leading to very cryptic errors that are hard to debug.
Modifications:
Changed the HTTP/2 handler and decoder to ensure that the very first data on the wire (WRT HTTP/2) is SETTINGS/preface+SETTINGS. When this is not the case, a connection error is thrown with the bytes that were found instead.
Result:
Fixes#3880
Related: #3814
Motivation:
To implement the support for an upgrade from cleartext HTTP/1.1
connection to cleartext HTTP/2 (h2c) connection, a user usually uses
HttpServerUpgradeHandler.
It does its job, but it requires a user to instantiate the UpgradeCodecs
for all supported protocols upfront. It means redundancy for the
connections that are not upgraded.
Modifications:
- Change the constructor of HttpServerUpgradeHandler
- Accept UpgraceCodecFactory instead of UpgradeCodecs
- The default constructor of HttpServerUpgradeHandler sets the
maxContentLength to 0 now, which shouldn't be a problem because a
usual upgrade request is a GET.
- Update the examples accordingly
Result:
A user can instantiate Http2ServerUpgradeCodec and its related objects
(Http2Connection, Http2FrameReader/Writer, Http2FrameListener, etc) only
when necessary.
Motivation:
Our HTTP/2 implementation sometimes uses hard-coded handler names when
adding/removing a handler to/from a pipeline. It's not really a good
idea because it can easily result in name clashes. Unless there is a
good reason, we need to use the reference to the handlers
Modifications:
- Allow null as a handler name for Http2Client/ServerUpgradeCodec
- Use null as the default upgrade handler name
- Do not use handler name strings in some test cases and examples
Result:
Fixes#3815
Related: #3871
Motivation:
StreamBufferingEncoderTest does not release when writeGoAway() is
called.
Modifications:
Release the buffer in mock object arguments
Result:
No buffer leak
Motiviation:
https://github.com/netty/netty/pull/3865 was merged from a machine with old code. A test case that was updates was not merged.
Modifications:
- Merge the missing test case updates
Result:
Test case no longer fails.
Motiviation:
The connection handler stream close operation is unconditionally adding a listener object to a future. We may not have to add a listener at all because the future has already been completed.
Modifications:
- If the future is done, directly invoke the logic without creating/adding a new listener.
Result:
No need to create/add listener if the future is already done in close logic.
Motivation:
gRPC's BufferingHttp2ConnectionEncoder is a generic utility that simplifies client-side applications that want to allow stream creation without worrying about violating the SETTINGS_MAX_CONCURRENT_STREAMS limit. Since it's not gRPC-specific it makes sense to move it into Netty proper.
Modifications:
Adding the BufferingHttp2ConnectionEncoder and it's unit test.
Result:
Netty now supports buffering stream creation.
Motivation:
SpdyOrHttpChooser and Http2OrHttpChooser duplicate fair amount code with each other.
Modification:
- Replace SpdyOrHttpChooser and Http2OrHttpChooser with ApplicationProtocolNegotiationHandler
- Add ApplicationProtocolNames to define the known application-level protocol names
Result:
- Less code duplication
- A user can perform dynamic pipeline configuration that follows ALPN/NPN for any protocols.
Related: #3641 and #3813
Motivation:
When setting up an HTTP/1 or HTTP/2 (or SPDY) pipeline, a user usually
ends up with adding arbitrary set of handlers.
Http2OrHttpChooser and SpdyOrHttpChooser have two abstract methods
(create*Handler()) that expect a user to return a single handler, and
also have add*Handlers() methods that add the handler returned by
create*Handler() to the pipeline as well as the pre-defined set of
handlers.
The problem is, some users (read: I) don't need all of them or the
user wants to add more than one handler. For example, take a look at
io.netty.example.http2.tiles.Http2OrHttpHandler, which works around
this issue by overriding addHttp2Handlers() and making
createHttp2RequestHandler() a no-op.
Modifications:
- Replace add*Handlers() and create*Handler() with configure*()
- Rename getProtocol() to selectProtocol() to make what it does clear
- Provide the default implementation of selectProtocol()
- Remove SelectedProtocol.UNKNOWN and use null instead, because
'UNKNOWN' is not a protocol
- Proper exception handling in the *OrHttpChooser so that the
exception is logged and the connection is closed when failed to
select a protocol
- Make SpdyClient example always use SSL. It was always using SSL
anyway.
- Implement SslHandshakeCompletionEvent.toString() for debuggability
- Remove an orphaned class: JettyNpnSslSession
- Add SslHandler.applicationProtocol() to get the name of the
application protocol
- SSLSession.getProtocol() now returns transport-layer protocol name
only, so that it conforms to its contract.
Result:
- *OrHttpChooser have better API.
- *OrHttpChooser handle protocol selection failure properly.
- SSLSession.getProtocol() now conforms to its contract.
- SpdyClient example works with SpdyServer example out of the box
Motivation:
There is currently no good way to configure the initial SETTINGS frame. The individual settings can be configured on the various components, but doing this bypasses the proper setting update logic in the encoder.
Modifications:
Updated Http2ConnectionHandler to optionally take initial settings in the constructor. If not provided, it will default to current behavior.
Result:
Easy manual configuration of initial settings.
Motivation:
The Http2OutboundFrameLogger logs all PING frames as not acks.
Modifications:
Changed the logger to correctly log PING acks.
Result:
PING acks are logged correctly.
Motiviation:
The Http2ConnectionHandler is incrementing the reference count in the goAway method for the debugData buffer after it has already been sent and maybe consumed. This may result in an IllegalRefCountException to be thrown. The unit tests also encounter buffer leaks because they have not been updated to invoke the listener which releases the buffer in the goAway method.
Modifications:
- The retain() call should be before the frameWriter().writeGoAway(...) call
- The unit tests which call goAway must also invoke the operationComplete(..) method for the listener.
Result:
No IllegalRefCountException. Less buffer leaks in tests.
Motivation:
If headers are sent on a stream that does not yet exist and the END_STREAM flag is set we will send a RST_STREAM frame. We should send the HEADERS frame and no RST_STREAM.
Modifications:
DefaultHttp2RemoteFlowController should allow frames to be sent if stream is created in the 'half closed (local)' state.
Result:
We can send HEADERS frame with the END_STREAM flag sent without sending a RST_STREAM frame.
Motivation:
There were a few outstanding comments that were left unaddressed after committing the changes for #3749.
Modifications:
Changes to Http2ConnectionHandler.goAway():
- Retaining the debugData buffer, rather than always converting it to a string immediately.
- Changing log level for sending a GOAWAY with error to debug.
Result:
Remaining comments from #3749 are addressed.
Motivation:
Currently the graceful shutdown of the HTTP/2 connection waits until there are no active streams. There may be use cases that buffer stream creation (due to limits imposed by MAX_CONCURRENT_STREAMS), in which case they may still want those streams to complete before closing.
Modifications:
Added a isGracefulShutdownComplete method to Http2ConnectionHandler, which can be overridden by a subclass.
Result:
Graceful shutdown logic can be overridden.
Motivation:
If the client closes, a GOWAY is sent with a lastKnownStream of zero (since the remote side never created a stream). If there is still an exchange in progress, inbound frames for streams created by the client will be ignored because our ignore logic doesn't check to see if the stream was created by the remote endpoint. Frames for streams created by the local endpoint should continue to come through after sending GOAWAY.
Modifications:
Changed the decoder's streamCreatedAfterGoAwaySent logic to properly ensure that the stream was created remotely.
Result:
We now propertly process frames received after sending GOAWAY.
Motivation:
The isDone method is currently broken in the aggregator because the doneAllocatingPromises accidentally calls the overridden version of setSuccess, rather than calling the base class version. This causes the base class's version to never be called since allowNotificationEvent will evaluate to false. This means that setSuccess0 will never be set, resulting in isDone always returning false.
Modifications:
Changed setSuccess() to call the base class when appropriate, regardless of the result of allowNotificationEvent.
Result:
isDone now behaves properly for the promise aggregator.
Motivation:
Allow users of HTTP2 to control when flushes occur so they can optimize network writes.
Modifications:
Removed explicit calls to flush in encoder, decoder & flow-controller
Connection handler now calls flush on read-complete to enable batching writes in response to reads
Result:
Much less flushing occurs for normal HTTP2 request and response patterns.
Motiviation:
There are a few spots in the HTTP/2 codec where warnings were generated and can be avoided.
Modifications:
Clean up the cause of the warnings.
Result:
Less warnings.
Motivation:
The Http2ConnectionHandler incorrectly doesn't propagate channelActive and channelInactive events and thus breaks the pipeline
for other ChannelHandler.
Modification:
- Add calls to super.channelActive() and super.channelInactive().
- Remove unused methods.
Result:
- Http2ConnectionHandler can be used with other ChannelHandlers.
Motivation:
The ByteString class currently assumes the underlying array will be a complete representation of data. This is limiting as it does not allow a subsection of another array to be used. The forces copy operations to take place to compensate for the lack of API support.
Modifications:
- add arrayOffset method to ByteString
- modify all ByteString and AsciiString methods that loop over or index into the underlying array to use this offset
- update all code that uses ByteString.array to ensure it accounts for the offset
- add unit tests to test the implementation respects the offset
Result:
ByteString and AsciiString can represent a sub region of a byte[].
Motivation:
Streams currently maintain a hash map of user-defined properties, which has been shown to add significant memory overhead as well as being a performance bottleneck for lookup of frequently used properties.
Modifications:
Modifying the connection/stream to use an array as the storage of user-defined properties, indexed by the class that identifies the index into the array where the property is stored.
Result:
Stream processing performance should be improved.
Motivation:
Currently we allocate the full amount of state for each stream as soon as the stream is created, and keep that state until the stream is GC. The full set of state is only needed when the stream can support flow controlled frames. There is an opportunity to reduce the required amount of memory, and make memory eligible for GC sooner by only allocating what is necessary for flow control stream state.
Modifications:
Introduce objects which require 'less' state for local/remote flow control stream state.
Use these new objects when streams have been created but will not transition out of idle AND when streams are no longer eligible for flow controlled frame transfer but still must persist in the priority tree.
Result:
Memory allocations are reduced to what is actually needed, and memory is made eligible for GC potentially sooner.
Motivation:
The recent PR that discarded the Http2StreamRemovalPolicy causes connection errors when receiving a frame for a stream that no longer exists. We should ignore these frames if we think there's a chance that the stream has existed previously
Modifications:
Modified the Http2Connection interface to provide a `streamMayHaveExisted` method. Also removed the requireStream() method to identify all of the places in the code that need to be updated.
Modified the encoder and decoder to properly handle cases where a stream may have existed but no longer does.
Result:
Fixes#3643
Motivation:
The current local flow controller does not guarantee that unconsumed bytes for a closed stream will be restored to the connection window. This may lead to degradation of the connection window over time.
Modifications:
Modified DefaultHttp2LocalFlowController to guarantee that any unconsumed bytes are returned to the connection window as soon as the stream is closed. We also immediately consume any bytes when receiving DATA for a closed stream.
Result:
Fixes#3668
Motivation:
Flow control is a required part of the HTTP/2 specification but it is currently structured more like an optional item. It must be accessed through the property map which is time consuming and does not represent its required nature. This access pattern does not give any insight into flow control outside of the codec (or flow controller implementation).
Modifications:
1. Create a read only public interface for LocalFlowState and RemoteFlowState.
2. Add a LocalFlowState localFlowState(); and RemoteFlowState remoteFlowState(); to Http2Stream.
Result:
Flow control is not part of the Http2Stream interface. This clarifies its responsibility and logical relationship to other interfaces. The flow controller no longer must be acquired though a map lookup.
Motivation:
If an exclusive dependency change stream B should be an exclusive dependency of stream A is requested and stream B is already a child of stream A...then we will add B to B's own children map and create a circular link in the priority tree. This leads to an infinite recursive loop and a stack overflow exception.
Modifications:
-when removeAllChildren is called it should not remove the exclusive dependency.
-unit test to ensure this case is covered.
Result:
No more circular link in the priority tree.
Motivation:
While forward porting https://github.com/netty/netty/pull/3579 there were a few areas that had not been previously back ported.
Modifications:
Backport the missed areas to ensure consistency.
Result:
More consistent 4.1 and master branches.
Motivation:
The usage and code within AsciiString has exceeded the original design scope for this class. Its usage as a binary string is confusing and on the verge of violating interface assumptions in some spots.
Modifications:
- ByteString will be created as a base class to AsciiString. All of the generic byte handling processing will live in ByteString and all the special character encoding will live in AsciiString.
Results:
The AsciiString interface will be clarified. Users of AsciiString can now be clear of the limitations the class imposes while users of the ByteString class don't have to live with those limitations.
Motivation:
Due to a recent flurry of cleanup and fixes, we no longer need the stream removal policy to protect against recently removed streams. We should get rid of it.
Modifications:
Removed Http2StreamRemovalPolicy and everywhere it's used.
Result:
Fixes#3448
Motivation:
The DefaultHttp2Connection is not checking for RuntimeExceptions when invoking Http2Connection.Listener methods. This is a problem for a few reasons: 1. The state of DefaultHttp2Connection will be corrupted if a listener throws a RuntimeException. 2. If the first listener throws then no other listeners will be notified, which may further corrupt state that is updated as a result of listeners being notified.
Modifications:
- Document that RuntimeExceptions are not supported for Http2Connection.Listener methods, and will be logged as an error.
- Update DefaultHttp2Connection to handle and exception for each listener that is notified, and be sure that 1 listener throwing an exception does not prevent others from being notified.
Result:
More robust DefaultHttp2Connection.
Motivation:
Now that we have a CharObjectHashMap, we should change Http2Settings to use it.
Modifications:
Changed Http2Settings to extend CharObjectHashMap rather than IntObjectHashMap.
Result:
Http2Settings uses less memory to store keys.
Motivation:
We've removed access to the activeStreams collection, we should do the same for the children of a stream to provide a consistent interface.
Modifications:
Moved Http2StreamVisitor to a top-level interface. Removed unnecessary child operations from the Http2Stream interface so that we no longer require a map structure.
Result:
Cleaner and more consistent interface for iterating over child streams.
Motivation:
The Http2Connection interface exposes an activeStreams() method which allows direct iteration over the underlying collection. There are a few places that make copies of this collection to avoid modification while iterating, and a few places that do not make copies. The copy operation can be expensive on hot code paths and also we are not consistently iterating over the activeStreams collection.
Modifications:
- The Http2Connection interface should reduce the exposure of the underlying collection and just expose what is necessary for the interface to function. This is just a means to iterate over the collection.
- The DefaultHttp2Connection should use this new interface and protect it's internal state while iteration is occurring.
Result:
Reduction in surface area of the Http2Connection interface. Consistent iteration of the set of active streams. Concurrent modification exceptions are handled in 1 encapsulated spot.
Motivation:
1) The current implementation doesn't allow for HEADERS, DATA, PING, PRIORITY and SETTINGS
frames to be sent after GOAWAY.
2) When receiving or sending a GOAWAY frame, all streams with ids greater than the lastStreamId
of the GOAWAY frame should be closed. That's not happening.
Modifications:
1) Allow sending of HEADERS and DATA frames after GOAWAY for streams with ids < lastStreamId.
2) Always allow sending PING, PRIORITY AND SETTINGS frames.
3) Allow sending multiple GOAWAY frames with decreasing lastStreamIds.
4) After receiving or sending a GOAWAY frame, close all streams with ids > lastStreamId.
Result:
The GOAWAY handling is more correct.
Motivation:
There are methods to manipulate the prioritzable count for streams which have the '0' postfix which are designed to be used during recursion. However these methods are calling out to an external method without the '0' during the recursive process. This is doing uneccessary conditional checks during recursion.
Modifications:
Change the decrementPrioritizableForTree to decrementPrioritizableForTree0 while in recursive method.
Change the incrementPrioritizableForTree to incrementPrioritizableForTree0 while in recursive method.
Result:
Less overhead during recursive calls.
Motiviation:
The interface provided by Http2LifecycleManager is not clear as to how the writeXXX methods should behave. The implementation of this interface from the Http2ConnectionHandler's perspecitve is unclear what writeXXX means in this context.
Modifications:
- Method names in Http2LifecycleManager and Http2ConnectionHandler should be renamed and comments should clarify the interfaces.
Results:
Http2LifecycleManager is more clear and Http2ConnectionHandler's implementation makes sense w.r.t to return values.
Motivation:
The HTTP/2 headers code should be using binary string (currently AsciiString) objects instead of String objects. The DefaultHttp2HeadersEncoder was still using String for sensitiveHeaders.
Modifications:
- Remove the usage of String from DefaultHttp2HeadersEncoder.
- Introduce an interface to determine if a header name/value is sensitive or not to 1. prevent necessarily creating/copying sets. 2. Allow the name/value to be considered when checking if sensitive.
Result:
No more String in DefaultHttp2HeadersEncoder and less required set creation/operations.
Motivation:
The spec requires that a RST_STREAM received on an IDLE stream results in a connection error. This is not happening.
Modifications:
Check for this condition when a RST_STREAM is received in DefaultHttp2ConnectionDecoder.
Result:
More spec compliant. Fixes https://github.com/netty/netty/issues/3573.
Motivation:
The DefaultHttp2ConnectionDecoder has the setPriority call after the Http2FrameListener is notified of the change. The setPriority call has additional verification logic and may even create the dependency stream and so it must be before the Http2FrameListener is notified.
Modifications:
The DefaultHttp2ConnectionDecoder should treat the setPriority call in the same for the HEADERS and PRIORITY frame (call it before notifying the listener).
Result:
Http2FrameListener should see correct state when a HEADERS frame has a stream dependency that has not yet been created yet. Fixes https://github.com/netty/netty/issues/3572.
Motivation:
We are allocating a hash map for every HTTP2 Stream to store it's children.
Most streams are leafs in the priority tree and don't have children.
Modification:
- Only allocate children when we actually use them.
- Make EmptyIntObjectMap not throw a UnsupportedOperationException on remove, but return null instead (as is stated in it's javadoc).
Result:
Fewer unnecessary allocations.
Motivation:
In a simple load test that creates and closes several 10k streams per second
I have seen Iterator objects using roughly 1.6% of the total committed heap.
Modifications:
Use an ArrayList instead of a LinkedHashSet to store the connection listeners.
That way we can iterate over the list without creating an iterator every time.
Result:
Zero Iterator allocations due to notifying connection listeners.
Motivation:
The Http2Settings class currently disallows setting non-standard settings, which violates the spec.
Modifications:
Updated Http2Settings to permit arbitrary settings. Also adjusting the default initial capacity to allow setting all of the standard settings without reallocation.
Result:
Fixes#3560
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
