Motivation:
- The decoded name should always end with a dot (.), but we currently
strip it, which is incorrect.
- (O) 0 -> "."
- (X) 0 -> ""
- (O) 5 netty 2 io 0 -> "netty.io."
- (X) 5 netty 2 io 0 -> "netty.io"
- The encoded name should end with a null-label, which is a label whose
length is 0, but we currently append an extra NUL, causing FORMERR(1)
on a strict DNS server:
- (O) . -> 0
- (X) . -> 0 0
- (O) netty.io. -> 5 netty 2 io 0
- (X) netty.io. -> 5 netty 2 io 0 0
Modifications:
- Make sure to append '.' when decoding a name.
- Improve index checks so that the decoder can raise
CorruptFrameException instead of IIOBE
- Do not encode extra NUL
- Add more tests
Result:
Robustness and correctness
Motivation:
bfbef036a8 made EPOLL respect autoRead while in ET mode. However it is possible that we may miss data pending on the RECV queue if autoRead is off. This is because maybeMoreDataToRead is updated after fireChannelRead and if a user calls read() from here maybeMoreDataToRead will be false because it is updated after the fireChannelRead call. The way maybeMoreDataToRead was updated also causes a single channel to continuously read on the event loop and not relinquish and give other channels to try reading.
Modifications:
- Ensure maybeMoreDataToRead is always set after all user events, and is evaluated with readPending to execute a epollInReady on the EventLoop
- Combine the checkResetEpollIn and maybeMoreDataToRead logic to invoke a epollInReady later into the epollInFinally method due to similar responsibilities
- Update unit tests to reflect the user calling read() on the event loop from channelRead()
Result:
EPOLL ET with autoRead set to false will not leave data on the RECV queue.
Motivation:
Commit 2696778 changed MqttEncoder to be a singelton but missed to add @Sharable annotation. This broke the encoder as it can not be added to multiple pipelines.
Modifications:
Add @Sharable annotation
Result:
MqttEncoder can be used in multiple pipelines again.
Motivation:
Setting the WRITE_BUFFER_LOW_WATER_MARK before WRITE_BUFFER_HIGH_WATER_MARK results in an internal Exception (appears only in the logs) if the value is larger than the default high water mark value. The WRITE_BUFFER_HIGH_WATER_MARK call appears to have no effect in this context.
Setting the values in the reverse order works.
Modifications:
- deprecated ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK and
ChannelOption.WRITE_BUFFER_LOW_WATER_MARK.
- add one new option called ChannelOption.WRITE_BUFFER_WATER_MARK.
Result:
The high/low water mark values limits caused by default values are removed.
Setting the WRITE_BUFFER_LOW_WATER_MARK before WRITE_BUFFER_HIGH_WATER_MARK results in an internal Exception (appears only in the logs) if the value is larger than the default high water mark value. The WRITE_BUFFER_HIGH_WATER_MARK call appears to have no effect in this context.
Setting the values in the reverse order works.
Motivation:
If a handler is added to the pipeline within ChannelInitializer::initChannel via
addFirst(...) then it will not receive the channelRegistered event. The same
handler added via addLast(...) will receive the event. This different behavior
is unlikely to be expected by users and can cause confusion.
Modifications:
Let ChannelInitializer::channelRegistered propagate the event by passing it to
the pipeline instead of firing it on the ChannelHandlerContext.
Result:
The channelRegistered event is propagated to handlers regardless of the method
used to add it to the pipeline (addFirst/addLast).
Motivation:
NIO now supports a pluggable select strategy, but EPOLL currently doesn't support this. We should strive for feature parity for EPOLL.
Modifications:
- Add SelectStrategy to EPOLL transport.
Result:
EPOLL transport supports SelectStategy.
Motivation:
Under high throughput/low latency workloads, selector wakeups are
degrading performance when the incoming operations are triggered
from outside of the event loop. This is a common scenario for
"client" applications where the originating input is coming from
application threads rather from the socket attached inside the
event loops.
As a result, it can be desirable to defer the blocking select
so that incoming tasks (write/flush) do not need to wakeup
the selector.
Modifications:
This changeset adds the notion of a generic SelectStrategy which,
based on its contract, allows the implementation to optionally
defer the blocking select based on some custom criteria.
The default implementation resembles the original behaviour, that
is if tasks are in the queue `selectNow()` and move on, and if no
tasks need to be processed go into the blocking select and wait
for wakeup.
The strategy can be customized per `NioEventLoopGroup` in the
constructor.
Result:
High performance client applications are now given the chance to
customize for how long the actual selector blocking should be
deferred by employing a custom select strategy.
Motivation:
There is no need to make DefaultChannelId package private as it may be useful for the user. For example EmbeddedChannel allows to inject a ChannelId when it is constructed. For this case the user can just use DefaultChannelId.
Modifications:
Change visibility of DefaultChannelId to public.
Result:
It's possible to create a new instance of DefaultChannelId by the user.
Motivation:
We need to ensure we run all pending tasks before doing any flush in writeOutbound(...) to ensure all pending tasks are run first. Also we should remove the assert of the future and just add a listener to it so it is processed later if needed. This is true as a user may schedule a write for later execution.
Modifications:
- Remove assert of future in writeOutbound(...)
- Correctly run pending tasks before doing the flush and also before doing the close of the channel.
- Add unit tests to proof the defect is fixed.
Result:
Correclty handle the situation of delayed writes.
Motivation:
We need to break out of the read loop for two reasons:
- If the input was shutdown in between (which may be the case when the user did it in the
fireChannelRead(...) method we should not try to read again to not produce any
miss-leading exceptions.
- If the user closes the channel we need to ensure we not try to read from it again as
the filedescriptor may be re-used already by the OS if the system is handling a lot of
concurrent connections and so needs a lot of filedescriptors. If not do this we risk
reading data from a filedescriptor that belongs to another socket then the socket that
was "wrapped" by this Channel implementation.
Modification:
Break the reading loop if the input was shutdown from within the channelRead(...) method.
Result:
No more meaningless exceptions and no risk to read data from wrong socket after the original was closed.
Motivation:
There are some use cases when a client may only be willing to read from a channel once
its previous write is finished (eg: serial dispatchers in Finagle). In this case, a
connection with SslHandler installed and ctx.channel().config().isAutoRead() == false
will stall in 100% of cases no matter what order of "channel active", "write", "flush"
events was.
The use case is following (how Finagle serial dispatchers work):
1. Client writeAndFlushes and waits on a write-promise to perform read() once it's satisfied.
2. A write-promise will only be satisfied once SslHandler finishes with handshaking and
sends the unencrypted queued message.
3. The handshaking process itself requires a number of read()s done by a client but the
SslHandler doesn't request them explicitly assuming that either auto-read is enabled
or client requested at least one read() already.
4. At this point a client will stall with NEED_UNWRAP status returned from underlying engine.
Modifiations:
Always request a read() on NEED_UNWRAP returned from engine if
a) it's handshaking and
b) auto read is disabled and
c) it wasn't requested already.
Result:
SslHandler is now completely tolerant of whether or not auto-read is enabled and client
is explicitly reading a channel.
Motivation:
We should throw a more helpful exception when a non PKCS#8 key is used by the user.
Modifications:
Change exception message to give a hint what is wrong.
Result:
Easier for user to understand whats wrong with their used key.
Motivation:
This allows using handlers for Streams in normal Netty-style. Frames are
read/written to the channel as messages, not directly as a
callback/method call. Handlers allow mixing and can ease HTTP/1 and
HTTP/2 interoperability by eventually supporting HTTP/1 handlers in
HTTP/2 and vise versa.
Modifications:
New handler Http2MultiplexCodec that converts from the current HTTP/2
API to a message-based API and child channels for streams.
Result:
The basics are done for server-side: new streams trigger creation of new
channels in much the same appearance to how new connections trigger new
channel creation. The basic frames HEADERS and DATA are handled, but
also GOAWAY and RST_STREAM.
Inbound flow control is implemented, but outbound is not. That will be
done later, along with not completing write promises on the child
channel until the write actually completes on the parent.
There is not yet support for outbound priority/weight, push promises,
and many other features.
There is a generic Object that may be set on stream frames. This also
paves the way for client-side support which needs a way to refer to
yet-to-be-created streams (due to how HEADERS allocates a stream id, and
the allocation order must be the same as transmission order).
Motivation:
8dbf5d02e5 modified the shutdown code for Socket but did not correctly calculate the change in shutdown state and only applying this change. This is significant because if sockets are being opening and closed quickly and the underlying FD happens to be reused we need to take care that we don't unintentionally change the state of the new FD by acting on an object which represents the old incarnation of that FD.
Modifications:
- Calculate the shutdown change, and only apply what has changed, or exit if no change.
Result:
Socket.shutdown can not inadvertently affect the state of another logical FD.
Motivation:
cf171ff525 introduced a change in behavior when dealing with closing channel in the read loop. This changed behavior may use stale state to determine if a channel should be shutdown and may be incorrect.
Modifications:
- Revert the usage of potentially stale state
Result:
Closing a channel in the read loop is based upon current state instead of potentially stale state.
Motivation:
Zero-length names needs to be "prefixed" by the length as well when encoded into a ByteBuf. Also some servers not correctly prefix these so we should ensure we can workaround this and even decode in such case.
Modifications:
- Always encode the length of the name into the ByteBuf even if its zero-length.
- If there are no readable bytes for the name just asume its an empty name to workaround dns servers that not fully respect the RFC.
Result:
Correctly encode zero-length names and be able to decode empty names even when the rfc is not strictly followed.
Motivation:
Often the user uses EmbeddedChannel within unit tests where the only "important" thing is to know if any pending messages were in the buffer and then release these.
We should provide methods for this so the user not need to manually loop through these and release.
Modifications:
Add methods to easily handle releasing of messages.
Result:
Less boiler-plate code for the user to write.
Motivation:
codec-mqtt had some typos and was not restrict enough in terms of making things final and private constructors.
Modifications:
- Fix typos
- Make most pojos final
- Remove redundant else blocks.
Result:
Cleaner and more restrict code.
Motivation:
Compile crash w/ JDK8:
```
[ERROR]
/Users/slandelle/Documents/dev/workspaces/workspace-ahc2/async-http-clie
nt-project/netty-bp/codec-dns/src/main/java/io/netty/handler/codec/dns/D
nsMessageUtil.java:[176,16] reference to append is ambiguous
both method append(java.lang.String) in java.lang.StringBuilder and
method append(java.lang.StringBuffer) in java.lang.StringBuilder match
```
Modification:
Force type explicitly
Result:
Class compile w/ JDK8
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:
We should upgrade to latest netty-tcnative version.
Modifications:
Upgrade to version 1.1.33.Fork15
Result:
Latest netty-tcnative version is used.
Motivation:
The current slow path of FastThreadLocal is much slower than JDK ThreadLocal. See #4418
Modifications:
- Add FastThreadLocalSlowPathBenchmark for the flow path of FastThreadLocal
- Add final to speed up the slow path of FastThreadLocal
Result:
The slow path of FastThreadLocal is improved.
Motivation:
The code of transport-native-epoll missed some things in terms of static keywords, @deprecated annotations and other minor things.
Modifications:
- Add missing @deprecated annotation
- Not using FQCN in javadocs
- Add static keyword where possible
- Use final fields when possible
- Remove throws IOException from method where it is not needed.
Result:
Cleaner code.
Motivation:
DefaultStompFrame.toString() implementations returned a String that contained DefaultFullStompFrame.
Modifications:
Replace DefaultFullStompFrame with DefaultStompFrame.
Result:
Less confusing and more correct return value of toString()
Motivation:
DefaultStompFrame.retain(increment) missed to pass on the increment parameter.
Modifications:
Correctly pass on increment paramter.
Result:
Correctly handle the retain when increment value is given.
Motivation:
There were some warning in the resolver-dns code base.
Modifications:
- Fix javadocs
- Use the base class to call static method.
Result:
Cleaner code.
Motivation:
SSLContext.buildTrustManagerFactory(...) builds a KeyStore to
initialize the TrustManagerFactory from an array of X509Certificates,
assuming that array is a chain and that each certificate will have a
unique Subject Distinguised Name.
However, the collection of certificates used as trust anchors is generally
not a chain (it is an unordered collection), and it is legitimate for it
to contain multiple certificates with the same Subject DN.
The existing code uses the Subject DN as the alias name when filling in
the `KeyStore`, thereby overwriting other certificates with the same
Subject DN in this collection, so some certificates may be discarded.
In addition, the code related to building trust managers can take an array of
X509Certificate instances to use as trust anchors. The variable name is
usually trustCertChain, and the documentation refers to them as a "chain".
However, while it makes sense to talk about a "chain" from a keymanager
point of view, these certificates are just an unordered collection in a
trust manager. (There is no chaining requirement, having the Subject DN
matching its predecessor's Issuer DN.)
This can create confusion to for users not used with PKI concepts.
Modifications:
SSLContext.buildTrustManagerFactory(...) now uses a distinct alias for each
array (simply using a counter, since this name is never used for reference
later). This patch also includes a unit test with CA certificates using the
same Subject DN.
Also renamed trustCertChain into trustCertCollection, and changed the
references to "chain" in the Javadoc.
Result:
Each loaded certificate now has a unique identifier when loaded, so it is
now possible to use multiple certificates with the same Subject DN as
trust anchors.
Hopefully, renaming the parameter should also reduce confusion around PKI
concepts.
Motivation:
DefaultCookie constructor performs a name validation that doesn’t match
RFC6265. Moreover, such validation is already performed in strict
encoders and decoders.
Modifications:
Drop DefaultCookie name validation, rely on encoders and decoders.
Result:
no more duplicate broken validation
Motivation:
We need to handle the trailing dot in the correct manner when creating DNS questions and responses.
Modifications:
- Add a trailing dot if not given to the hostname when construct a AbstractDnsRecord (this is the same as dig does).
Result:
Correctly handle trailing dots.
Motivation:
The method setBytes creates temporary heap buffer when source buffer is read-only.
But this temporary buffer is not used correctly and may lead to data corruption.
This problem occurs when target buffer is pooled and temporary buffer
arrayOffset() is not zero.
Modifications:
Use correct arrayOffset when calling PlatformDependent.copyMemory.
Unit test was added to test this case.
Result:
Setting buffer content works correctly when target is pooled buffer and source
is read-only ByteBuffer.
Motivation:
We need to ensure we call ctx.flush() before closing the actual channel when an handshake failure took place. If we miss to do so we may not send all pending data to the remote peer which also include SSL alerts.
Modifications:
Ensure we call ctx.flush() before ctx.close() on a handshake error.
Result:
All pending data (including SSL alerts) are written to the remote peer on a handshake error.
Motivation:
It should be possible to disable the Recycler with -Dio.netty.recycler.maxCapacity=0, but because of a typo this is not the case.
Modifications:
Replace <= with < to make it posible to disable the Recycler.
Result:
Correct behaviour when using -Dio.netty.recycler.maxCapacity=0
Motivation:
Fix a typo in the log message of the static initializer of Recycler.
Modifications:
Fix typo.
Result:
Correctly log system property io.netty.recycler.maxCapacity.
Motivation:
We currently not supported using KeyManagerFactory with OpenSslClientContext and so should throw an exception if the user tries to do so. This will at least not give suprising and hard to debug problems later.
Modifications:
Throw exception if a user tries to construct a OpenSslClientContext with a KeyManagerFactory
Result:
Fail fast if the user tries to use something that is not supported.
Motivation:
We need to ensure we do all checks inside of the try / catch block so we free native memory that was allocated in the constructor of the super class in a timely manner.
Modifications:
Move all checks inside of the try block.
Result:
Correctly release native memory (and not depend on the finalizer) when a check in the constructors fails
Motivation:
We missed to pass the decrement value to the wrapped FullHttpRequest and so missed to decrement the reference count in the correct way.
Modifications:
Correctly pass the decrement value to the wrapped request.
Result:
UpgradeEvent.release(decrement) works as expected.
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:
In commit acbca192bd we changed to have our native operations which either gall getsockopt or setsockopt throw IOExceptions (to be more specific we throw a ClosedChannelException in some cases). Unfortunally I missed to also do the same for getSoError() and missed to add throws IOException to the native methods.
Modifications:
- Correctly throw IOException from getSoError()
- Add throws IOException to native methods where it was missed.
Result:
Correct declaration of getSoError() and other native methods.
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.
