Motivation:
If a single Encoder object is promoted to the old generation then every object
reachable from the promoted object will eventually be promoted as well. A queue
illustrates the problem very well. Say a sequence of inserts and deletions
generate an object graph:
A -> B -> C -> D -> E -> F -> G -> H,
the head of the queue is E, the tail of the queue is H, and A, B, C, D are
dead. If all queue nodes are in the young generation, then a young gc will
clean up the object graph and leave us with:
E -> F -> G -> H
on the other hand, if B and C were previously promoted to the old generation,
then a young collection assumes the refernece from C to D is from a live object
(this is a key result of generational gc, no need to mark the old generation).
Hence the young collection assumes the refence to D is a gc root and leave us
with the object graph:
B-> C -> D -> E -> F -> G -> H.
Eventually D, E, F, G, H, and all queue nodes ever seen from this point on will
be promoted, regardless of their global live or dead status. It is generally
trivial to fix nepotism issues by simply breaking the reference chain after
dequeuing a node.
Currently Encoder objects do not null their references when removed from the
hash map. We have observed a 20X increase in promoted Encoder objects due to
nepotism.
Modifications:
Null before, after, and next fields when removing Encoder objects from maps.
Result:
Fewer promoted Encoder objects, fewer Encoder objects in the old generation,
shorter young collection times, old collections spaced further apart (nepotism
is just really bad). Enjoy.
Motivation:
If an error occurs during a write operation then DefaultHttp2ConnectionEncoder.FlowControlledData will clear the CoalescingBufferQueue which will reset the queue's readable bytes to 0. To recover from an error the DefaultHttp2RemoteFlowController will attempt to return bytes to the flow control window, but since the frame has reset its own size this will lead to invalid flow control accounting.
Modifications:
- DefaultHttp2ConnectionEncoder.FlowControlledData should not reset its size if an error occurs
Result:
No more flow controller errors due to DefaultHttp2ConnectionEncoder.FlowControlledData setting its size to 0 if an error occurs.
Motivation:
HttpServerUpgradeHandler.UpgradeCodec.prepareUpgradeResponse should allow to abort the upgrade and so just continue with using HTTP. Beside this we should only pass in the response HttpHeaders as this is inline with the docs.
Modifications:
- UpgradeCodec.prepareUpgradeResponse now allows to return a boolean and so allows to specifiy if the upgrade should take place.
- Change the param from FullHttpResponse to HttpHeaders to be inline with the javadocs.
Result:
More flexible and correct handling of upgrades.
Motivation:
upgradeTo(...) takes the response as paramater, but the respone itself was already written to the Channel. This gives the user the impression the response can be changed or even act on it which may not be safe anymore once it was written and has been released.
Modifications:
Remove the response param from the method.
Result:
Less confusion and safer usage.
Motivation:
Http2Codec.SimpleChannelPromiseAggregator currently fails fast if as soon as a tryFailure or setFailure method is called. This can lead to write operations which pass the result of SimpleChannelPromiseAggregator.newPromise to multiple channel.write calls throwing exceptions due to the promise being already done. This behavior is not expected by most of the Netty codecs (SslHandler) and can also create unexpected leaks in the http2 codec (DefaultHttp2FrameWriter).
Modifications:
- Http2Codec.SimpleChannelPromiseAggregator shouldn't complete the promise until doneAllocatingPromises is called
- Usages of Http2Codec.SimpleChannelPromiseAggregator should be adjusted to handle the change in behavior
- What were leaks in DefaultHttp2FrameWriter should be fixed to catch any other cases where ctx.write may throw
Result:
SimpleChannelPromiseAggregator won't generate promises which are done when newPromise is called.
Motivation:
PromiseAggregator's API allows for the aggregate promise to complete before the user is done adding promises. In order to support this use case the API structure would need to change in a breaking manner.
Modifications:
- Deprecate PromiseAggregator and subclasses
- Introduce PromiseCombiner which corrects these issues
Result:
PromiseCombiner corrects the deficiencies in PromiseAggregator.
Motiviation:
691bc1690e made writeUtf8 consistent with String.getBytes() so that it never throws.
94f27be59b provided a writeUtf8 method which takes a ByteBufAllocator to do an appropriately sized buffer allocation.
Result:
- Assume writeUtf8 will not throw in HTTP/2 codec
- Use the new writeUtf8 method
Result:
Cleaner code in codec-http2.
Motivation:
If while iterating the active streams a close operation occurs this will be queued and process after the iteration has completed to avoid a concurrent modification exception. However it is possible that during the iteration the stream which was closed could have been removed from the priority tree and its parent would be set to null. Then after the iteration completes the close operation will attempt to dereference the parent and results in a NPE.
Modifications:
- pending close operations should verify the stream's parent is not null before processing the event
Result:
No More NPE.
Motivation:
83c4aa6ad8 changed the log level to warn, but should have changed to debug.
Modifications:
- Change the log level to debug in Http2ConnectionHandler if the GO_AWAY fails to send. The write failure could be the result of the channel already being closed.
Result:
Fixes https://github.com/netty/netty/issues/4930.
Motiviation:
cfcee5798d introduced code to resize the headers based upon a weighted average. The weight used for new entries was initialized using integer arithmetic when it should have been floating point arithmetic and so new values contribute 0 weight.
Modifications:
- Cast to float when initializing
Result:
Weighted average does not give 0 weight to new headers in DefaultHttp2HeadersDecoder.
Motivation:
The interface contract of Http2Connection.Listener.onStreamClosed says that the stream will be removed from the active stream map, and not necessarily the stream map. If the channel becomes inactive we may remove from the stream map before calling onStreamClosed.
Modifications:
- Don't remove from the stream map during iteration until after onStreamClosed is called
Result:
Expectations of onStreamClosed interface are not violated
Motivation:
Http2ConnectionHandler inherits from ByteToMessageDecoder. ByteToMessageDecoder.channelInactive will attempt to decode any remaining data by calling the abstract decode method. If the Http2ConnectionHandler is in server mode, and no data has been exchanged yet, it will try to treat this data as an invalid connection preface and write a GO_AWAY. This is noisy in the logs and creates an illusion that there is a protocol violation when there has not been.
Modifications:
- If the channel is inactive the connection preface decode should not be executed.
Result:
Log files don't include misleading error messages related to connection preface errors.
Modifications:
When constructing the FullHttpMessage pass in the ByteBuf to use via the ByteBufAllocator assigned via the context.
Result:
The ByteBuf assigned to the FullHttpMessage can now be configured as a pooled/unpooled, direct/heap based ByteBuf via the ByteBufAllocator used.
Motivation:
Http2Connection.onStreamRemoved is not always called if Http2Connection.onStreamAdded is called. This is problematic as users may rely on the onStreamRemoved method to be called to release ByteBuf objects and do other cleanup.
Modifications:
- Http2Connection.close will remove all streams existing streams and prevent new ones from being created
- Http2ConnectionHandler will call the new close method in channelInactive
Result:
Http2Connection.onStreamRemoved is always called when Http2Connection.onStreamRemoved is called to preserve the Http2Connection guarantees.
Fixes https://github.com/netty/netty/issues/4838
Motivation:
Commit f990f99 introduced a bug into the RST_STREAM processing that would prevent a RST_STREAM from being sent when it should have been. The promise would be marked as successful even though the RST_STREAM frame would never be sent.
Modifications:
- Fix conditional in Http2ConnectionHandler.resetStream to allow reset streams to be sent in all stream states besides IDLE.
Result:
RST_STREAM frames are now sent when they are supposed to be sent
Fixes https://github.com/netty/netty/issues/4856
Motivation:
Http2CodecUtil uses ByteBufUtil.writeUtf8 but does not account for it
throwing an exception. If an exception is thrown because the format is
not valid UTF16 encoded UTF8 then the buffer will leak.
Modifications:
- Make sure the buffer is released if an exception is thrown
- Ensure call sites of the Http2CodecUtil.toByteBuf can tolerate and
exception being thrown
Result:
No leak if exception data can not be converted to UTF8.
Motivation:
Currently the initial headers for every stream is queued in the flow controller. Since the initial header frame may create streams the peer must receive these frames in the order in which they were created, or else this will be a protocol error and the connection will be closed. Tolerating the initial headers being queued would increase the complexity of the WeightedFairQueueByteDistributor and there is benefit of doing so is not clear.
Modifications:
- The initial headers will no longer be queued in the flow controllers
Result:
Fixes https://github.com/netty/netty/issues/4758
Motivation:
In HttpConversionUtil's toHttpRequest and toHttpResponse methods can
allocate FullHttpMessage objects, and if an exeception is thrown during
the header conversion then this object will not be released. If a
FullHttpMessage is not fired up the pipeline, and the stream is closed
then we remove from the map, but do not release the object. This leads
to a ByteBuf leak. Some of the logic related to stream lifetime management
and FullHttpMessage also predates the RFC being finalized and is not correct.
Modifications:
- Fix leaks in HttpConversionUtil
- Ensure the objects are released when they are removed from the map.
- Correct logic and unit tests where they are found to be incorrect.
Result:
Fixes https://github.com/netty/netty/issues/4780
Fixes https://github.com/netty/netty/issues/3619
Motivation:
When HttpClientUpgradeHandler upgrades from HTTP/1 to another protocol,
it performs a two-step opertion:
1. Remove the SourceCodec (HttpClientCodec)
2. Add the UpgradeCodec
When HttpClientCodec is removed from the pipeline, the decoder being
removed triggers channelRead() event with the data left in its
cumulation buffer. However, this is not received by the UpgradeCodec
becuase it's not added yet. e.g. HTTP/2 SETTINGS frame sent by the
server can be missed out.
To fix the problem, we need to reverse the steps:
1. Add the UpgradeCodec
2. Remove the SourceCodec
However, this does not work as expected either, because UpgradeCodec can
send a greeting message such as HTTP/2 Preface. Such a greeting message
will be handled by the SourceCodec and will trigger an 'unsupported
message type' exception.
To fix the problem really, we need to make the upgrade process 3-step:
1. Remove/disable the encoder of SourceCodec
2. Add the UpgradeCodec
3. Remove the SourceCodec
Modifications:
- Add SourceCodec.prepareUpgradeFrom() so that SourceCodec can remove or
disable its encoder
- Implement HttpClientCodec.prepareUpgradeFrom() properly
- Miscellaneous:
- Log the related channel as well When logging the failure to send a
GOAWAY
Result:
Cleartext HTTP/1-to-HTTP/2 upgrade works again.
Motivation:
StreamBufferingEncoder provides queueing so that MAX_CONCURRENT_STREAMS is not violated. However the stream id generation provided by Http2Connection.nextStreamId() only returns the next stream id that is expected on the connection and does not account for queueing. The codec should provide a way to generate the next stream id for a given endpoint that functions with or without queueing.
Modifications:
- Change Http2Connection.nextStreamId to Http2Connection.incrementAndGetNextStreamId
Result:
Http2Connection can generate the next stream id in queued and non-queued scenarios.
Fixes https://github.com/netty/netty/issues/4704
Motivation:
If validateHeaders is set in combination with the encoder/decoder it will be silently ignored. We should enforce the constraint that validateHeaders and encoder/decoder are mutually exclusive.
Modifications:
- Make sure either validateHeaders can be set or encoder/decoder.
Result:
AbstractHttp2ConnectionHandlerBuilder does not allow conflicting options to be set.
Motivation:
DefaultHttp2RemoteFlowController does not correctly account for the padding in the event frames are merged. This causes the internal stat of DefaultHttp2RemoteFlowController to become corrupt and can result in attempting to write frames when there are none.
Modifications:
- Update DefaultHttp2RemoteFlowController to account for frame sizes not necessarily adding together.
Result:
DefaultHttp2RemoteFlowController internal state does not become corrupt when padding is present.
Fixes https://github.com/netty/netty/issues/4573
Motivation:
Currently it's impossible to distinguish which
connection the corresponding logged message is
related to.
Modifications:
Http2FrameLogger is extended to support channel
id logging, usages in Inbound/Outbound Frame
Loggers are adjusted accordingly.
Result:
Logger outputs the channel id.
Motivation:
HttpConversionUtil.toHttp2Headers currently has a throws Exception as part of the signature. This comes from the signature of ByteProcessor.process, but is not necessary because the ByteProcessor used does not throw.
Modifications:
- Remove throws Exception from the signature of HttpConversionUtil.toHttp2Headers.
Result:
HttpConversionUtil.toHttp2Headers interface does not propagate a throws Exception when it is used.
Motivation:
- AbstractHttp2ConnectionHandlerBuilder.encoderEnforceMaxConcurrentStreams can be the primitive boolean
- SpdySession.StreamComparator should not be Serializable since SpdySession is not Serializable
Modifications:
Use boolean instead and remove Serializable
Result:
- Minor improvement for AbstractHttp2ConnectionHandlerBuilder
- StreamComparator is not Serializable any more
Motivation:
Javadoc reports errors about invalid docs.
Modifications:
Fix some errors reported by javadoc.
Result:
A lot of javadoc errors are fixed by this patch.
Motivation:
There are some wrong links and tags in javadoc.
Modifications:
Fix the wrong links and tags in javadoc.
Result:
These links will work correctly in javadoc.
Motivation:
If the stream window is negative UniformStreamByteDistributor may write data. This is prohibited by the RFC https://tools.ietf.org/html/rfc7540#section-6.9.2.
Modifications:
- UniformStreamByteDistributor should use StreamState.isWriteAllowed()
Result:
UniformStreamByteDistributor is more complaint with HTTP/2 RFC.
Fixes https://github.com/netty/netty/issues/4545
Motivation:
RemoteFlowController.streamWritten is not currently required. We should remove it to keep interfaces minimal.
Modifications:
- Remove RemoteFlowController.streamWritten
Result:
1 Less method in RemoteFlowController interface.
Fixes https://github.com/netty/netty/issues/4600
Motivation:
PriorityStreamByteDistributor is now obsolete and can be replaced by WeightedFairQueueByteDistributor.
Modifications:
- Remove PriorityStreamByteDistributor and use WeightedFairQueueByteDistributor by default.
Result:
PriorityStreamByteDistributor no longer has to be maintained and is replaced by a better algorithm.
Motivation:
DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor no longer reliably detects when a stream's writability change occurs.
Modifications:
- Ensure writiability is reliabily reported by DefaultHttp2RemoteFlowController.ListenerWritabilityMonitor
- Fix infinite loop issue (https://github.com/netty/netty/issues/4588) detected when consolidating unit tests
Result:
Reliable stream writability change notification, and 1 less infinite loop in UniformStreamByteDistributor.
Fixes https://github.com/netty/netty/issues/4587
Related: #4572#4574
Motivation:
Consistency in our builder API design
Modifications:
- Add AbstractInboundHttp2ToHttpAdapterBuilder
- Replace the old 'Builder's with InboundHttp2ToHttpAdapterBuilder and
InboundHttp2ToHttpPriorityAdapterBuilder
Result:
Builder API consistency
Motivation:
PriorityStreamByteDistributor uses a homegrown algorithm which distributes bytes to nodes in the priority tree. PriorityStreamByteDistributor has no concept of goodput which may result in poor utilization of network resources. PriorityStreamByteDistributor also has performance issues related to the tree traversal approach and number of nodes that must be visited. There also exists some more proven algorithms from the resource scheduling domain which PriorityStreamByteDistributor does not employ.
Modifications:
- Introduce a new ByteDistributor which uses elements from weighted fair queue schedulers
Result:
StreamByteDistributor which is sensitive to priority and uses a more familiar distribution concept.
Fixes https://github.com/netty/netty/issues/4462
Related: #4572
Motivation:
- A user might want to extend Http2ConnectionHandler and define his/her
own static inner Builder class that extends
Http2ConnectionHandler.BuilderBase. This introduces potential
confusion because there's already Http2ConnectionHandler.Builder. Your
IDE will warn about this name duplication as well.
- BuilderBase exposes all setters with public modifier. A user's Builder
might not want to expose them to enforce it to certain configuration.
There's no way to hide them because it's public already and they are
final.
- BuilderBase.build(Http2ConnectionDecoder, Http2ConnectionEncoder)
ignores most properties exposed by BuilderBase, such as
validateHeaders, frameLogger and encoderEnforceMaxConcurrentStreams.
If any build() method ignores the properties exposed by the builder,
there's something wrong.
- A user's Builder that extends BuilderBase might want to require more
parameters in build(). There's no way to do that cleanly because
build() is public and final already.
Modifications:
- Make BuilderBase and Builder top-level so that there's no duplicate
name issue anymore.
- Add AbstractHttp2ConnectionHandlerBuilder
- Add Http2ConnectionHandlerBuilder
- Add HttpToHttp2ConnectionHandlerBuilder
- Make all builder methods in AbstractHttp2ConnectionHandlerBuilder
protected so that a subclass can choose which methods to expose
- Provide only a single build() method
- Add connection() and codec() so that a user can still specify
Http2Connection or Http2Connection(En|De)coder explicitly
- Implement proper state validation mechanism so that it is prevented
to invoke conflicting setters
Result:
Less confusing yet flexible builder API
Motivation:
Some times the user wants to set a Http2HeaderEncoder.SensitivityDetector when building a Http2ConnectionHandler.
Modifications:
Allow to set Http2HeaderEncoder.SensitivityDetector via builder.
Result:
More flexible building of Http2ConnectionHandler possible.
Motivation:
We already provide a NEVER_SENSITIVE instance,we should add ALWAYS_SENSITIVE as well.
Modifications:
Add ALWAYS_SENSITIVE instance which will always return true when check for sesitive.
Result:
User can reuse code.
Motivation:
Because we flow control HEADERS frames, it's possible that an intermediate error can result in a RST_STREAM frame being sent for a frame that the other endpoint is not yet aware of. This is a violation of the spec and will either result in spammy logs at the other endpoint or broken connections.
Modifications:
Modified the HTTP/2 handler so that it only sends RST_STREAM if it has sent at least one HEADERS frame to the remote endpoint for the stream.
Result:
Fixes#4465
Motivation:
The encoder is currently responsible for chunking frames when writing in order to conform to max frame size. The frame writer would be a better place for this since it could perform a reuse the same promise aggregator for all the write and could also perform a single allocation for all of the frame headers.
Modifications:
Modified `DefaultHttp2FrameWriter` to perform the chunking and modified the contract in the `Http2FrameWriter` interface. Modified `DefaultHttp2ConnectionEncoder` to send give all allocated bytes to the writer.
Result:
Fixes#3966
Motivation:
The UniformStreamByteDistributor currently processes all zero-length frames, regardless of add order. This means that we would always send HEADERS for all streams, possibly taking away bandwidth for streams that actually have data.
Modifications:
Empty frames are now treated the same as any other frame except that the algorithm will pop off the any empty frames at the head of the queue.
Result:
Empty frames require no extra processing.
