Motivation:
Revert d0943dcd30. Delaying the notification of writability change may lead to notification being missed. This is a ABA type of concurrency problem.
Modifications:
- Revert d0943dcd30.
Result:
channelWritabilityChange will be called on every change, and will not be suppressed due to ABA scenario.
Motivation:
ByteBuf.readBytes(...) uses Unpooled.buffer(...) internally which will use a heap ByteBuf and also not able to make use of the allocator which may be pooled. We should better make use of the allocator.
Modifications:
Use the allocator for thenew buffer.
Result:
Take allocator into account when copy bytes.
Motiviation:
Sometimes it is useful to dump the status of the PooledByteBufAllocator and log it. Doing this is currently a bit cumbersome as the user needs to basically iterate through all the metrics and compose the String. we would better provide an easy way to do this.
Modification:
Add dumpStats() method.
Result:
Easier to get a view into the status of the allocator.
Motivation:
Sometimes a user only has access to a preconfigured SSLContext but still would like to use our ssl sub-system. For this situations it would be very useful if the user could create a JdkSslContext instance from an existing SSLContext.
Modifications:
- Create new public constructors in JdkSslContext which allow to wrap an existing SSLContext and make the class non-abstract
- Mark JdkSslServerContext and JdkSslClientContext as deprecated as the user should not directly use these.
Result:
It's now possible to create an JdkSslContext from an existing SSLContext.
Motivation:
We missed to correctly retrieve the localAddress() after we called Socket.connect(..) and so the user would always see an incorrect address when calling EpollSocketChannel.localAddress().
Modifications:
- Ensure we always retrieve the localAddress() after we called Socket.connect(...) as only after this we will be able to receive the correct address.
- Add unit test
Result:
Correct and consistent behaviour across different transports (NIO/OIO/EPOLL).
Motivation:
PoolChunkList.allocate(...) should return false without the need to walk all the contained PoolChunks when the requested capacity is larger then the capacity that can be allocated out of the PoolChunks in respect to the minUsage() and maxUsage() of the PoolChunkList.
Modifications:
Precompute the maximal capacity that can be allocated out of the PoolChunks that are contained in the PoolChunkList and use this to fast return from the allocate(...) method if an allocation capacity larger then that is requested.
Result:
Faster detection of allocations that can not be handled by the PoolChunkList and so faster allocations in general via the PoolArena.
Motivation:
To better understand how much memory is used by Netty for ByteBufs it is useful to understand how many bytes are currently active (allocated) per PoolArena.
Modifications:
- Add PoolArenaMetric.numActiveBytes()
Result:
The user is able to get better insight into the PooledByteBufAllocator.
Motivation:
To make it easier to understand PoolChunk and PoolArena we should cleanup duplicated code.
Modifications:
- Move reused code into methods
- Use Math.max(...)
Result:
Cleaner code and easier to understand.
Motivation:
We use ByteBuf.readBytes(int) in various places where we could either remove it completely or use readSlice(int).retain().
Modifications:
- Remove ByteBuf.readBytes(int) when possible or replace by readSlice(int).retain().
Result:
Faster code.
Motivation:
When doing a normal allocation in PoolArena we also tried to allocate out of the PoolChunkList that only contains completely full PoolChunks. This makes no sense as these are full anyway so an allocation will never work here and just gives a perf hit as we need to walk the whole list of PoolChunks in the list.
Modifications:
Not try to allocate from PoolChunkList that only contains full PoolChunks
Result:
Faster allocation times when a new PoolChunk must be created.
Motivation:
Commit 0bc93dd introduced a potential assertion failure, if the deprecated method would be used.
Modifications:
Fix the potential assertion error.
Result:
Regression removed
Motivation:
441aa4c575 conditionally set the readFlag based upon if maybeMoreDataToRead is set. It is possible that the read flag will not be set, and nothing will be read by executeEpollInReadyRunnable and no actual data will be read even though the user requested it.
Modifications:
- Always set the readFlag in doBeginRead
- Make it so only a single epollInReadyRunnable can execute for a channel at a time
Result:
Less chance of missing read events in EPOLL transport.
Motivation:
There is one extra } for WriteBufferWaterMark's javadoc:
{@linkplain #high} high water mark}
The generated javadoc will show the content: "the high high water mark}"
Modifications:
remove the }
Result:
The generated javadoc will show the content: "the high water mark" instead of "the high high water mark}"
Motivation:
It's better to make all InternalLoggerFactory implementations be singletons according to the discussions in #5047
Modifications:
Make all InternalLoggerFactory implementations be singletons and hide the construtors.
Result:
All InternalLoggerFactory implementations be singletons.
Motivation:
Prior to 5b48fc284e setting readPending to false when autoReadClear was called was enough because when/if the EventLoop woke up with a read event we would first check if readPending was true and if it is not, we would return early. Now that readPending will only be set in the EventLoop it is not necessary to check readPending at the start of the read methods, and we should instead remove the OP_READ from the SelectionKey when we also set readPending to false.
Modifications:
- autoReadCleared should call AbstractNioUnsafe.removeReadOp
Result:
NIO is now consistent with EPOLL and removes the READ operation on the selector when autoRead is cleared.
Motivation:
When writing a SMTP client a provided SMTP codec that follows RFC2821 is useful.
Modification:
Add client side codec and test.
Results:
People who want to write a SMTP client can reuse the codec.
Motivation:
When upgrading h2c, I found that sometimes both of http2 settings frame and http response message was arrived before receiving upgrade success event. It was because ByteToMessageDecoder propagated its internally buffered message to the next handler when removing itself from pipeline.(refer to ByteToMessageDecoder#handlerRemoved)
I think it's better to propagate upgrade success event when handling 101 switching protocol response.
Modifications:
Upgrade success event will be propagated before removing source codec.
Result:
It guarantees that upgrade success event will be arrived first at the next handler.
Motivation:
People need to set all length fields manually when creating a memcache message and it's error prone. See #2736 for more dicussion.
Modifications:
This patch adds the logic to update the keyLength, extrasLength and totalBodyLength when key, extras or content is set.
Result:
The length fields of memcache messages will be updated automatically.
Motivation:
OIO/NIO use a volatile variable to track if a read is pending. EPOLL does not use a volatile an executes a Runnable on the event loop thread to set readPending to false. These mechansims should be consistent, and not using a volatile variable is preferable because the variable is written to frequently in the event loop thread.
OIO also does not set readPending to false before each fireChannelRead operation and may result in reading more data than the user desires.
Modifications:
- OIO/NIO should not use a volatile variable for readPending
- OIO should set readPending to false before each fireChannelRead
Result:
OIO/NIO/EPOLL are more consistent w.r.t. readPending and volatile variable operations are reduced
Fixes https://github.com/netty/netty/issues/5069
Motivation:
We should better use Math utilities as these are intrinsics. This is a cleanup for ea3ffb8536.
Modifications:
Use Math utilities.
Result:
Cleaner code and use of intrinsics.
Motivation:
Fixes#5084. We (gRPC) encountered a bug that was triggered by
grpc/grpc-java@d927180. After that commit, event loop threads are
created per task by NioEventLoopGroup, and inherits the thread group of
the caller, which in our case is an application-provided request-scope
thread. Things go south when the application tries to manipulate (e.g.,
interrupt and join) all threads of the request-scope thread group, which
unexpectedly include the event loop threads.
Modifications:
DefaultThreadFactory will save the current thread group in constructor,
and apply it to all new threads.
Result:
Threads created by DefaultThreadFactory will be in the same thread group
as the thread where the factory is created.
Motivation:
When a PoolChunk needs to get moved to the previous PoolChunkList because of the minUsage / maxUsage constraints we always just moved it one level which is incorrect and so could lead to have PoolChunks in the wrong PoolChunkList (in respect to their minUsage / maxUsage settings). This then could have the effect that PoolChunks are not released / freed in a timely fashion and so.
Modifications:
- Correctly move PoolChunks between PoolChunkLists, which includes moving it multiple "levels".
- Add unit test
Result:
Correctlty move the PoolChunk to PoolChunkList when it is freed, even if its multiple layers.
Related: #3449
Motivation:
When a user shut down an EventExecutor/Loop prematurely, a Promise will
fail to execute its listeners. When it happens, DefaultPromise will log
a message at ERROR level, but there's no way to get notified about it
programmatically.
Modifications:
Do not catch and log the RejectedExecutionException unconditionally,
but only catch and log for non-late listener notifications, so that a
user gets notified on submission failure at least when the listener is
late.
Result:
Remedies #3449 to some extent, although we will need fundamental fix for
that, such as #3566
Motivation:
When always triggered fireChannelWritabilityChanged() directly when the update the pending bytes in the ChannelOutboundBuffer was made from within the EventLoop. This is problematic as this can cause some re-entrance issue if the user has a custom ChannelOutboundHandler that does multiple writes from within the write(...) method and also has a handler that will intercept the channelWritabilityChanged event and trigger another write when the Channel is writable. This can also easily happen if the user just use a MessageToMessageEncoder subclass and triggers a write from channelWritabilityChanged().
Beside this we also triggered fireChannelWritabilityChanged() too often when a user did a write from outside the EventLoop. In this case we increased the pending bytes of the outboundbuffer before scheduled the actual write and decreased again before the write then takes place. Both of this may trigger a fireChannelWritabilityChanged() event which then may be re-triggered once the actual write ends again in the ChannelOutboundBuffer.
The third gotcha was that a user may get multiple events even if the writability of the channel not changed.
Modification:
- Always invoke the fireChannelWritabilityChanged() later on the EventLoop.
- Only trigger the fireChannelWritabilityChanged() if the channel is still active and if the writability of the channel changed. No need to cause events that were already triggered without a real writability change.
- when write(...) is called from outside the EventLoop we only increase the pending bytes in the outbound buffer (so that Channel.isWritable() is updated directly) but not cause a fireChannelWritabilityChanged(). The fireChannelWritabilityChanged() is then triggered once the task is picked up by the EventLoop as usual.
Result:
No more re-entrance possible because of writes from within channelWritabilityChanged(...) method and no events without a real writability change.
Motivation:
The PoolChunkList.minUsage() and maxUsage() needs to take special action to translate Integer.MIN_VALUE / MAX_VALUE as these are used internal for tail and head of the linked-list structure.
Modifications:
- Correct the minUsage() and maxUsage() methods.
- Add unit test.
Result:
Correct metrics
Motivation:
fcbeebf6df introduced a unit test to verify ApplicationProtocolNegotiationHandler is compatible with SniHandler. However only the server attempts ALPN and verifies that it completes and the client doesn't verify the handshake is completed. This can lead to the client side SSL engine to prematurely close and throw an exception.
Modifications:
- The client should wait for the SSL handshake and ALPN to complete before the test exits.
Result:
SniHandlerTest.testSniWithApnHandler is more reliable.
Motivation:
441aa4c575 introduced a bug in transport-native-epoll where readPending is set to false before a read is attempted, but this should happen before fireChannelRead is called. The NIO transport also only sets the readPending variable to false on the first read in the event loop. This means that if the user only calls read() on the first channelRead(..) the select loop will still listen for read events even if the user does not call read() on subsequent channelRead() or channelReadComplete() in the same event loop run. If the user only needs 2 channelRead() calls then by default they will may get 14 more channelRead() calls in the current event loop, and then 16 more when the event loop is woken up for a read event. This will also read data off the TCP stack and allow the peer to queue more data in the local RECV buffers.
Modifications:
- readPending should be set to false before each call to channelRead()
- make NIO readPending set to false consistent with EPOLL
Result:
NIO and EPOLL transport set readPending to false at correct times which don't read more data than intended by the user.
Fixes https://github.com/netty/netty/issues/5082
Motivation:
When a promise is notified that was already added to the ChannelOutboundBuffer and we try to notify it later on we only see a warning that it was notified before. This is often not very useful as we have no idea where it was notified at all. We can do better in case it was failed before (which is most of the times the case) and just also log the cause that was used for it.
Modifications:
Add the cause that was used to notify the promise when we fail to notify it as part of the ChannelOutboundBuffer.
Result:
Easier to debug user errors.
Motivation:
There is a spelling error in FileRegion.transfered() as it should be transferred().
Modifications:
Deprecate old method and add a new one.
Result:
Fix typo and can remove the old method later.
Motivation:
See #3095
Modifications:
Add Log4J2LoggerFactory and Log4J2Logger which is an InternalLogger implementation based on log4j2.
Result:
The user can use log4j2 directly without a special slf4j binding.
Motivation:
DefaultChannelHandlerInvoker currently blindly cast to AbstractChannelHandlerContext without checking if the ChannelHandlerContext is really a sub-type of it. It should check it first and if not just use slow-path implementation.
Modifications:
Do instanceof check first and if it fails just create a new Runnable instance of used the cached.
Result:
DefaultChannelHandlerInvoker works with any ChannelHandlerContext implementations.
Motivation:
Sometimes it is useful to allow to disable the leak detection of buffers if the UnpooledByteBufAllocator is used. This is for example true if the app wants to leak buffers into user code but not want to put the burden on the user to always release the buffer.
Modifications:
Add another constructor to UnpooledByteBufAllocator that allows to completely disable leak-detection for all buffers that are allocator out of the UnpooledByteBufAllocator.
Result:
It's possible to disable leak-detection when the UnpooledByteBufAllocator is used.
Motivation:
We should only increment the metric for the huge / normal allocation after it is done. Also we should only decrement once deallocate.
Modifications:
- Move increment after the allocation.
- Fix deallocation metric and move it after deallocation
Result:
More correct metrics.
Motivation:
PoolThreadCache includes the wrong value when throwing a IllegalArgumentException because of freeSweepAllocationThreshold.
Modifications:
Use the correct value.
Result:
Correct exception message.
Motivation:
ApplicationProtocolNegotiationHandler attempts to get a reference to an SslHandler in handlerAdded, but when SNI is in use the actual SslHandler will be added to the pipeline dynamically at some later time. When the handshake completes ApplicationProtocolNegotiationHandler throws an IllegalStateException because its reference to SslHandler is null.
Modifications:
- Instead of saving a reference to SslHandler in handlerAdded just search the pipeline when the SslHandler is needed
Result:
ApplicationProtocolNegotiationHandler support SniHandler.
Fixes https://github.com/netty/netty/issues/5066
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.
