Motivation:
At the moment it’s possible to have a Channel in Netty that is not registered / assigned to an EventLoop until register(...) is called. This is suboptimal as if the Channel is not registered it is also not possible to do anything useful with a ChannelFuture that belongs to the Channel. We should think about if we should have the EventLoop as a constructor argument of a Channel and have the register / deregister method only have the effect of add a Channel to KQueue/Epoll/... It is also currently possible to deregister a Channel from one EventLoop and register it with another EventLoop. This operation defeats the threading model assumptions that are wide spread in Netty, and requires careful user level coordination to pull off without any concurrency issues. It is not a commonly used feature in practice, may be better handled by other means (e.g. client side load balancing), and therefore we propose removing this feature.
Modifications:
- Change all Channel implementations to require an EventLoop for construction ( + an EventLoopGroup for all ServerChannel implementations)
- Remove all register(...) methods from EventLoopGroup
- Add ChannelOutboundInvoker.register(...) which now basically means we want to register on the EventLoop for IO.
- Change ChannelUnsafe.register(...) to not take an EventLoop as parameter (as the EventLoop is supplied on custruction).
- Change ChannelFactory to take an EventLoop to create new Channels and introduce ServerChannelFactory which takes an EventLoop and one EventLoopGroup to create new ServerChannel instances.
- Add ServerChannel.childEventLoopGroup()
- Ensure all operations on the accepted Channel is done in the EventLoop of the Channel in ServerBootstrap
- Change unit tests for new behaviour
Result:
A Channel always has an EventLoop assigned which will never change during its life-time. This ensures we are always be able to call any operation on the Channel once constructed (unit the EventLoop is shutdown). This also simplifies the logic in DefaultChannelPipeline a lot as we can always call handlerAdded / handlerRemoved directly without the need to wait for register() to happen.
Also note that its still possible to deregister a Channel and register it again. It's just not possible anymore to move from one EventLoop to another (which was not really safe anyway).
Fixes https://github.com/netty/netty/issues/8513.
Motivation:
In Http2FrameCodec we made the incorrect assumption that we can only have 1 buffered outboundstream as maximum. This is not correct and we need to account for multiple buffered streams.
Modifications:
- Use a map to allow buffer multiple streams
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8692.
* Handling AUTO_READ should not be the responsibility of DefaultChannelPipeline but the Channel itself.
Motivation:
At the moment we do automatically call read() in the DefaultChannelPipeline when fireChannelReadComplete() / fireChannelActive() is called and the Channel is using auto read. This is nice in terms of sharing code but imho is not the responsibility of the ChannelPipeline implementation but the responsibility of the Channel implementation.
Modifications:
Move handing of auto read from DefaultChannelPipeline to Channel implementations.
Result:
More clear responsibiliy and not depending on implemention details of the ChannelPipeline.
Motiviation:
Http2FrameCodecTest and Http2MultiplexCodecTest were quite fragile and often not went through the whole pipeline which made testing sometimes hard and error-prone.
Modification:
- Refactor tests to have data flow through the whole pipeline and so made the test more robust (by testing the while implementation).
Result:
Easier to write tests for the codecs in the future and more robust testing in general.
Beside this it also fixes https://github.com/netty/netty/issues/6036.
Motivation:
We should always call ctx.read() even when AUTO_READ is false as flow-control is enforced by the HTTP/2 protocol.
See also https://tools.ietf.org/html/rfc7540#section-5.2.2.
We already did this before but not explicit and only did so because of some implementation details of ByteToMessageDecoder. It's better to be explicit here to not risk of breakage later on.
Modifications:
- Ensure we always call ctx.read() when AUTO_READ is false
- Add unit test.
Result:
No risk of staling the connection when HTTP/2 is used.
Motivation:
In windows if the project is in a path that contains whitespace,
resources cannot be accessed and tests fail.
Modifications:
Adds ResourcesUtil.java in netty-common. Tests use ResourcesUtil.java to access a resource.
Result:
Being able to build netty in a path containing whitespace
Motivation:
ByteBuf supports “marker indexes”. The intended use case for these is if a speculative operation (e.g. decode) is in process the user can “mark” and interface and refer to it later if the operation isn’t successful (e.g. not enough data). However this is rarely used in practice,
requires extra memory to maintain, and introduces complexity in the state management for derived/pooled buffer initialization, resizing, and other operations which may modify reader/writer indexes.
Modifications:
Remove support for marking and adjust testcases / code.
Result:
Fixes https://github.com/netty/netty/issues/8535.
Motivation:
9f9aa1a did some changes related to fixing how we handle ctx.read() in child channel but did incorrectly change some assert.
Modifications:
Fix assert to be correct.
Result:
Code does not throw an AssertionError due incorrect assert check.
Motivation:
Most of the maven modules do not explicitly declare their
dependencies and rely on transitivity, which is not always correct.
Modifications:
For all maven modules, add all of their dependencies to pom.xml
Result:
All of the (essentially non-transitive) depepdencies of the modules are explicitly declared in pom.xml
Motivation:
We did not correct respect ctx.read() calls while processing a read for a child Channel. This could lead to read stales when auto read is disabled and no other read was requested.
Modifications:
- Keep track of extra read() calls while processing reads
- Add unit tests that verify that read() is respected when triggered either in channelRead(...) or channelReadComplete(...)
Result:
Fixes https://github.com/netty/netty/issues/8209.
Motivation
DefaultHttp2FrameReader currently does a fair amount of "intermediate"
slicing which can be avoided.
Modifications
Avoid slicing the input buffer in DefaultHttp2FrameReader until
necessary. In one instance this also means retainedSlice can be used
instead (which may also avoid allocating).
Results
Less allocations when using http2.
Motivation:
When the DefaultHttp2ConnectionEncoder writes the initial headers for a new
locally created stream we create the stream in the half-closed state if the
end-stream flag is set which signals to the life cycle manager that the headers
have been sent. However, if we synchronously fail to write the headers the
life cycle manager then sends a RST_STREAM on our behalf which is a connection
level PROTOCOL_ERROR because the peer sees the stream in an IDLE state.
Modification:
Don't open the stream in the half-closed state if the end-stream flag is
set and let the life cycle manager take care of it.
Result:
Cleaner state management in the DefaultHttp2ConnectionEncoder.
Fixes#8434.
Motivation:
The `Http2StreamFrameToHttpObjectCodec` is marked `@Sharable` but mutates
an internal `HttpScheme` field every time it is added to a pipeline.
Modifications:
Instead of storing the `HttpScheme` in the handler we store it as an
attribute on the parent channel.
Result:
Fixes#8480.
Motivation:
There are log messages emitted from Http2ConnectionDecoder of the form
```
INF i.n.h.c.h.DefaultHttp2ConnectionDecoder ignoring HEADERS frame for stream RST_STREAM sent. {}
```
Modifications:
Remove the trailing `{}` in the log message that doesn't have a value.
Result:
Log messages no longer have a trailing `{}`.
Motivation:
When writing an HTTP/2 HEADERS with END_STREAM=1, the application expects
the stream to be closed afterward. However, the write can fail locally
due to HPACK encoder and similar. When that happens we need to make sure
to issue a RST_STREAM otherwise the stream can be closed locally but
orphaned remotely. The RST_STREAM is typically handled by
Http2ConnectionHandler.onStreamError, which will only send a RST_STREAM
if that stream still exists locally.
There are two possible flows for trailers, one handled immediately and
one going through the flow controller. Previously they behaved
differently, with the immedate code calling the error handler after
closing the stream. The immediate code also used a listener for calling
closeStreamLocal while the flow controlled code did so immediately after
the write.
The two code paths also differed in their VoidChannelPromise handling,
but both were broken. The immediate code path called unvoid() only if
END_STREAM=1, however it could always potentially add a listener via
notifyLifecycleManagerOnError(). And the flow controlled code path
unvoided incorrectly, changing the promise completion behavior. It also
passed the wrong promise to closeStreamLocal() in FlowControlledBase.
Modifications:
Move closeStreamLocal handling after calls to onError. This is the
primary change.
Now call closeStreamLocal immediately instead of when the future
completes. This is the more likely correct behavior as it matches that
of DATA frames.
Fix all the VoidChannelPromise handling.
Result:
Http2ConnectionHandler.onStreamError sees the same state as the remote
and issues a RST_STREAM, properly cleaning up the stream.
Motivation:
Http2MultiplexCodec queues data internally if data is delivered from the
parent channel but the child channel did not request data. If the parent
channel notifies of a stream closure it is possible data in the queue
will be discarded before closing the channel.
Http2MultiplexCodec interacts with RecvByteBufAllocator to control the
child channel's demand for read. However it currently only ever reads a
maximum of one time per loop. This can thrash the read loop and bloat
the call stack if auto read is on, because channelReadComplete will
re-enter the read loop synchronously, and also neglect to deliver data
during the parent's read loop (if it is active). This also meant the
readPendingQueue was not utilized as originally intended (to extend the
child channel's read loop during the parent channel's read loop if
demand for data still existed).
Modifications:
- Modify the child channel's read loop to respect the
RecvByteBufAllocator, and append to the parents readPendingQueue if
appropriate.
- Stream closure notification behaves like EPOLL and KQUEUE transports
and reads all queued data, because the data is already queued in memory
and it is known there will be no more data. This will also replenish the
connection flow control window which may otherwise be constrained by a
closed stream.
Result:
More correct read loop and less risk of dropping data.
Motivation:
When a Http2MultiplexCodec stream channel fails to write the first
HEADERS it will forcibly close, and that will trigger sending a
RST_STREAM, which is commonly a connection level protocol error. This is
because it has what looks like a valid stream id, but didn't check with
the connection as to whether the stream may have actually existed.
Modifications:
Instead of checking if the stream was just a valid looking id ( > 0) we
check with the connection as to whether it may have existed at all.
Result:
We no longer send a RST_STREAM frame from Http2MultiplexCodec for idle
streams.
Motivation:
The Http2Connection state is updated by the DefaultHttp2ConnectionDecoder after the frame listener is notified of the goaway frame. If the listener sends a frame synchronously this means the connection state will not know about the goaway it just received and we may send frames that are not allowed on the connection. This may also mean a stream object is created but it may never get taken out of the stream map unless some other event occurs (e.g. timeout).
Modifications:
- The Http2Connection state should be updated before the listener is notified of the goaway
- The Http2Connection state modification and validation should be self contained when processing a goaway instead of partially in the decoder.
Result:
No more creating streams and sending frames after a goaway has been sent or received.
Motivation:
If the local endpoint receives a GO_AWAY frame and then tries to write a stream with a streamId higher than the last know stream ID we will throw a connection error. This results in the local peer sending a GO_AWAY frame to the remote peer, but this is not necessary as the error can be isolated to the local endpoint and communicated via the ChannelFuture return value.
Modifications:
- Instead of throwing a connection error, throw a stream error that simulates the peer receiving the stream and replying with a RST
Result:
Connections are not closed abruptly when trying to create a stream on the local endpoint after a GO_AWAY frame is received.
Motivation:
If a write fails for a Http2MultiplexChannel stream channel, the channel
may be forcibly closed, but only after the promise has been failed. That
means continuations attached to the promise may see the channel in an
inconsistent state of still being open and active.
Modifications:
Move the satisfaction of the promise to after the channel cleanup logic
runs.
Result:
Listeners attached to the future that resulted in a Failed write will
see the stream channel in the correct state.
Motivation:
The HTTP/2 spec dictates that invalid pseudo-headers should cause the
request/response to be treated as malformed (8.1.2.1), and the recourse
for that is to treat the situation as a stream error of type
PROTOCOL_ERROR (8.1.2.6). However, we're treating them as a connection
error with the connection being immediately torn down and the HPACK
state potentially being corrupted.
Modifications:
The HpackDecoder now throws a StreamException for validation failures
and throwing is deffered until the end of of the decode phase to ensure
that the HPACK state isn't corrupted by returning early.
Result:
Behavior more closely aligned with the HTTP/2 spec.
Fixes#8043.
Motivation:
We deviate from the AbstractChannel implementation on deregistration by
failing the provided promise if the channel is already deregistered. In
contrast, AbstractChannel will always set the promise to successfully
done.
Modification:
Change the
Http2MultiplexCodec.DefaultHttp2StreamChannel.Http2ChannelUnsafe to
always set the promise provided to deregister as done as is the
case in AbstractChannel.
Motivation:
There is an inconsistency between the order of events in the
StreamChannel implementation in Http2MultiplexCodec and other Channel
implementations that extend AbstractChannel where channelInactive and
channelUnregistered events are not performed 'later'. This can cause an
unexected order of events for ChannelHandler implementations that call
Channel.close() in response to some event.
Modification:
The Http2MultiplexCodec.DefaultHttp2StreamChannel.Http2ChannelUnsafe was
modified to bounce the deregistration and channelInactive events through
the parent channels EventLoop.
Result:
Stream events are now in the proper order.
Fixes#8018.
Motivation:
Http2MultiplexCodec doesn't currently have an API for using the response
of a h2c upgrade request.
Modifications:
Add a new API to the Http2MultiplexCodecBuilder which allows for setting
an upgrade handler and wire it into the Http2MultiplexCodec
implementation.
Result:
When using the Http2MultiplexCodec with h2c upgrades the upgrade handler
will get added to the Http2StreamChannel which represents the
half-closed (local) response of stream 1. It is then up to the user to
manage the transition from the IO channel pipeline configuration
necessary for making the h2c upgrade request to a form where it can read
the response from the new stream channel.
Fixes#7947.
Motivation:
The `ByteBuffer emptyPingBuf()` method of Http2CodecUtils is has been dead
code since DefaultHttp2PingFrame switched from using a ByteBuf to represent
the 8 octets to a long.
Modifications:
Remove the method and the unused static ByteBuf.
Result:
Less dead code.
Fixes#8002
Motivation:
This is a followup for #7860. In the fix for #7860 we only partly fixed the problem as Http2UnknownFrame did not correctly extend HttpStreamFrame and so only worked when using the Http2FrameCodec. We need to have it extend HttpStreamFrame as otherwise Http2MultiplexCodec will reject to handle it correctly.
Modifications:
- Let Http2UnknownFrame extend HttpStreamFrame
- Add unit tests for writing and reading Http2UnkownFrame instances when the Http2MultiplexCodec is used.
Result:
Fixes https://github.com/netty/netty/issues/7969.
Motivation:
When a sender sends too large of headers it should not unnecessarily
kill the connection, as killing the connection is a heavy-handed
solution while SETTINGS_MAX_HEADER_LIST_SIZE is advisory and may be
ignored.
The maxHeaderListSizeGoAway limit in HpackDecoder is unnecessary because
any headers causing the list to exceeding the max size can simply be
thrown away. In addition, DefaultHttp2FrameReader.HeadersBlockBuilder
limits the entire block to maxHeaderListSizeGoAway. Thus individual
literals are limited to maxHeaderListSizeGoAway.
(Technically, literals are limited to 1.6x maxHeaderListSizeGoAway,
since the canonical Huffman code has a maximum compression ratio of
.625. However, the "unnecessary" limit in HpackDecoder was also being
applied to compressed sizes.)
Modifications:
Remove maxHeaderListSizeGoAway checking in HpackDecoder and instead
eagerly throw away any headers causing the list to exceed
maxHeaderListSize.
Result:
Fewer large header cases will trigger connection-killing.
DefaultHttp2FrameReader.HeadersBlockBuilder will still kill the
connection when maxHeaderListSizeGoAway is exceeded, however.
Fixes#7887
Motivation:
Integer autoboxing in this class (and possibly also the varargs arrays)
showed non-negligible CPU and garbage contribution when profiling a gRPC
service. grpc-java currently hardcodes use of Http2FrameLogger, set at
DEBUG level.
Modifications:
Wrap offending log statements in conditional blocks.
Result:
Garbage won't be produced by Http2FrameLogger when set to a disabled
logging level.
Motivation:
Streams can be deregistered so we can't assume their existence in the stream map.
Modifications:
Add a null-check in case a stream has been deregistered.
Result:
Fixes#7898.
Motivation:
We incorrectly called frame.release() in onHttp2GoAwayFrame which could lead to IllegalReferenceCountExceptions. The call of release() is inappropriate because the fireChannelRead() in onHttp2Frame() will handle it.
Modifications:
- Not call frame.release()
- Add a unit test
Result:
Fxies https://github.com/netty/netty/issues/7892.
It is possible to create streams in the half-closed state where the
stream state doesn't reflect that the request headers have been sent by
the client or the server hasn't received the request headers. This
state isn't possible in the H2 spec as a half closed stream must have
either received a full request or have received the headers from a
pushed stream. In the current implementation, this can cause the stream
created as part of an h2c upgrade request to be in this invalid state
and result in the omission of RST frames as the client doesn't believe
it has sent the request to begin with.
Modification:
The `DefaultHttp2Connection.activate` method checks the state and
modifies the status of the request headers as appropriate.
Result:
Fixes#7847.
