Motivation:
If the user uses 0 as quiet period we should shutdown without any delay if possible.
Modifications:
Ensure we not introduce extra delay when a shutdown quit period of 0 is used.
Result:
EventLoop shutdown as fast as expected.
Motivation:
At the moment we call initChannel(...) in the channelRegistered(...) method which has the effect that if another ChannelInitializer is added within the initChannel(...) method the ordering of the added handlers is not correct and surprising. This is as the whole initChannel(...) method block is executed before the initChannel(...) block of the added ChannelInitializer is handled.
Modifications:
Call initChannel(...) from within handlerAdded(...) if the Channel is registered already. This is true in all cases for our DefaultChannelPipeline implementation. This way the ordering is always as expected. We still keep the old behaviour as well to not break code for other ChannelPipeline implementations (if someone ever wrote one).
Result:
Correct and expected ordering of ChannelHandlers.
Motivation:
I received a report the its not possible to add another ChannelInitialiter in the initChannel(...) method, so we should add a test case for it.
Modifications:
Added testcase.
Result:
Validate that all works as expected.
Motivation:
When a ChannelInitializer is used via ServerBootstrap.handler(...) the users handlers may be added after the internal ServerBootstrapAcceptor. This should not happen.
Modifications:
Delay the adding of the ServerBootstrapAcceptor until the initChannel(....) method returns.
Result:
Correct order of handlers in the ServerChannels ChannelPipeline.
Motivation:
This change is part of the change done in PR #5395 to provide an `AUTO_FLUSH` capability.
Splitting this change will enable to try other ways of implementing `AUTO_FLUSH`.
Modifications:
Two methods:
```java
void executeAfterEventLoopIteration(Runnable task);
boolean removeAfterEventLoopIterationTask(Runnable task);
```
are added to `SingleThreadEventLoop` class for adding/removing a task to be executed at the end of current/next iteration of this `eventloop`.
In order to support the above, a few methods are added to `SingleThreadEventExecutor`
```java
protected void afterRunningAllTasks() { }
```
This is invoked after all tasks are run for this executor OR if the passed timeout value for `runAllTasks(long timeoutNanos)` is expired.
Added a queue of `tailTasks` to `SingleThreadEventLoop` to hold all tasks to be executed at the end of every iteration.
Result:
`SingleThreadEventLoop` now has the ability to execute tasks at the end of an eventloop iteration.
Motivation:
For some use-cases it would be useful to know the number of bytes queued in the PendingWriteQueue without the need to dequeue them.
Modifications:
Add PendingWriteQueue.bytes().
Result:
Be able to get the number of bytes queued.
Motivation:
Commit 4c048d069d99891d6a83859b469c39b4ff0f4ae1 moved the logic of calling handlerAdded(...) to the channelRegistered(...) callback of the head of the DefaultChannelPipeline. Unfortunatlly this may execute the callbacks to late as a user may add handlers to the pipeline in the ChannelFutureListener attached to the registration future. This can lead to incorrect ordering.
Modifications:
Ensure we always invoke ChannelHandler.handlerAdded(...) for all handlers before the registration promise is notified.
Result:
Not possible of incorrect ordering or missed events.
Motivation:
We pinned the EventExecutor for a Channel in DefaultChannelPipeline. Which means if the user added multiple handlers with the same EventExecutorGroup to the ChannelPipeline it will use the same EventExecutor for all of these handlers. This may be unexpected and even not what the user wants. If the user want to use the same one for all of them it can be done by obtain an EventExecutor and pass the same instance to the add methods. Because of this we should allow to not pin.
Modifications:
Allow to disable pinning of EventExecutor for Channel based on EventExecutorGroup via ChannelOption.
Result:
Less confusing and more flexible usage of EventExecutorGroup when adding ChannelHandlers to the ChannelPipeline.
Motivation:
In commit f984870ccca133d6056e8b0df0b2352f8f90b0fe I made a change which operated under invalide assumption that tasks executed by an EventExecutor will always be processed in a serial fashion. This is true for SingleThreadEventExecutor sub-classes but not part of the EventExecutor interface contract.
Because of this change implementations of EventExecutor which not strictly execute tasks in a serial fashion may miss events before handlerAdded(...) is called. This is strictly speaking not correct as there is not guarantee in this case that handlerAdded(...) will be called as first task (as there is no ordering guarentee).
Cassandra itself ships such an EventExecutor implementation which has no strict ordering to spread load across multiple threads.
Modifications:
- Add new OrderedEventExecutor interface and let SingleThreadEventExecutor / EventLoop implement / extend it.
- Only expose "restriction" of skipping events until handlerAdded(...) is called for OrderedEventExecutor implementations
- Add ThreadPoolEventExecutor implementation which executes tasks in an unordered fashion. This is used in added unit test but can also be used for protocols which not expose an strict ordering.
- Add unit test.
Result:
Resurrect the possibility to implement an EventExecutor which does not enforce serial execution of events and be able to use it with the DefaultChannelPipeline.
Motivation:
When `ChannelFactory#newChannel` crashed, `AbstractBootstrap#initAndRegister` propagates the exception to the caller instead of failing the promise.
Modifications:
- Catch exceptions from `ChannelFactory#newChannel`.
- Notify promise of such failure.
Result:
`AbstractBootstrap` gracefully handles connect failures.
Motivation:
JCTools supports both non-unsafe, unsafe versions of queues and JDK6 which allows us to shade the library in netty-common allowing it to stay "zero dependency".
Modifications:
- Remove copy paste JCTools code and shade the library (dependencies that are shaded should be removed from the <dependencies> section of the generated POM).
- Remove usage of OneTimeTask and remove it all together.
Result:
Less code to maintain and easier to update JCTools and less GC pressure as the queue implementation nt creates so much garbage
Motivation:
There is a small race while adding handlers to the pipeline because callHandlerAddedForAllHandlers() may not be run when the user calls add* but the Channel is already registered.
Modifications:
Ensure we always delay handlerAdded(..) / handlerRemoved(...) until callHandlerAddedForAllHandlers() was called.
Result:
No more race on pipeline modifications possible.
Motivation:
While doing 8fe3c83e4ca9a64c03f5adcb9f056d9e9440a389 I made a change which disallowed using null as name for handlers in the pipeline (this generated a new name before).
Modifications:
Revert to old behaviour and adding test case.
Result:
Allow null name again
Motivation:
At the moment the user is responsible to increase the writer index of the composite buffer when a new component is added. We should add some methods that handle this for the user as this is the most popular usage of the composite buffer.
Modifications:
Add new methods that autoamtically increase the writerIndex when buffers are added.
Result:
Easier usage of CompositeByteBuf.
Motivation:
EventLoopGroup.register doesn't need the Channel paramter when ChannelPromise is provided as we can get the Channel from ChannelPromise. Resolves#2422.
Modifications:
- Add EventLoopGroup.register(ChannelPromise)
- Deprecate EventLoopGroup.register(Channel, ChannelPromise)
Result:
EventLoopGroup.register is more convenient as people only need to set one parameter.
Motivation:
If the user will use addLast(...) on the ChannelPipeline of EmbeddedChannel after its constructor was run it will break the EmbeddedChannel as it will not be able to collect inbound messages and exceptions.
Modifications:
Ensure addLast(...) work as expected by move the logic of handling messages and exceptions ti protected methods of DefaultChannelPipeline and use a custom implementation for EmbeddedChannel
Result:
addLast(...) works as expected when using EmbeddedChannel.
Motivation:
When a user has multiple EventLoops in an EventLoopGroup and calls pipeline.add* / remove* / replace from an EventLoop that belongs to another Channel it is possible to deadlock if the other EventLoop does the same.
Modification:
- Only ensure the actual modification takes place in a synchronized block and not wait until the handlerAdded(...) / handlerRemoved(...) method is called. This is ok as we submit the task to the executor while still holding the look and so ensure correct order of pipeline modifications.
- Ensure if an AbstractChannelHandlerContext is put in the linked-list structure but the handlerAdded(...) method was not called we skip it until handlerAdded(...) was called. This is needed to ensure handlerAdded(...) is always called first.
Result:
Its not possible to deadlock when modify the DefaultChannelPipeline.
Related: #4333#4421#5128
Motivation:
slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.
Modifications:
- Add the following methods which creates a non-recyclable derived buffer
- retainedSlice()
- retainedDuplicate()
- readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
- a user can replace the content of the holder in a consistent way
- copy/duplicate/retainedDuplicate() can delegate the holder
construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
- Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
- Make ReplayingDecoderByteBuf.reject() return an exception instead of
throwing it so that its callers don't need to add dummy return
statement
Result:
Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
Motivation:
We tried to provide the ability for the user to change the semantics of the threading-model by delegate the invoking of the ChannelHandler to the ChannelHandlerInvoker. Unfortunually this not really worked out quite well and resulted in just more complexity and splitting of code that belongs together. We should remove the ChannelHandlerInvoker again and just do the same as in 4.0
Modifications:
Remove ChannelHandlerInvoker again and replace its usage in Http2MultiplexCodec
Result:
Easier code and less bad abstractions.
Motivation:
To be more consistent we should use ConnectException when we fail the connect attempt because no LocalServerChannel exists with the given address.
Modifications:
Use correct exception.
Result:
More consistent handling of connection refused between different transports.
Motivation:
Bootstrap.connect(...) tries to obtain the EventLoop of a Channel before it may be registered. This will cause an IllegalStateException. We need to ensure we handle the cause of late registration and not throw in this case.
Modifications:
Ensure we only try to access the EventLoop after the Channel is registered and handle the case of late registration.
Result:
Bootstrap.connect(...) not fails on late registration.
Motivation:
The LateListener logic is prone to infinite loops and relies on being processed in the EventExecutor's thread for synchronization, but this EventExecutor may not be constant. An infinite loop can occur if the EventExecutor's execute method does not introduce a context switch in LateListener.run. The EventExecutor can be changed by classes which inherit from DefaultPromise. For example the DefaultChannelPromise will return w/e EventLoop the channel is registered to, but this EventLoop can change (re-registration).
Modifications:
- Remove the LateListener concept and instead use a single Object to maintain the listeners while still preserving notification order
- Make the result member variable an atomic variable so it can be outside the synchronized(this) blocks
- Cleanup/simplify existing state management code
Result:
Fixes https://github.com/netty/netty/issues/5185
Motivation:
Revert d0943dcd30b08eb4043aeb88fd983bcebf8c3432. Delaying the notification of writability change may lead to notification being missed. This is a ABA type of concurrency problem.
Modifications:
- Revert d0943dcd30b08eb4043aeb88fd983bcebf8c3432.
Result:
channelWritabilityChange will be called on every change, and will not be suppressed due to ABA scenario.
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:
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:
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:
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:
PendingWriteQueue should guard against re-entrant writes once
removeAndFailAll() is run.
Modifications:
removeAndFailAll() should repeat until the queue is finally empty.
Result:
assertEmpty() will always hold.
Motivation:
e2f5012 added unit tests which did not verify the buffer was released as it was intended to.
Modification:
- Unit tests must verify release is called
Result:
Unit tests enforce that ByteBufs are released.
Motivation:
DefaultChannelHandlerInvoker.invokeWrite calls a utility method validatePromise which may throw if the arguments are not valid. If this method throws then the message will not be released.
Modifications:
- If an exception is thrown the message should be released
Result:
No more leak in DefaultChannelHandlerInvoker.invokeWrite
Motiviation:
We should ensure that handlerAdded(...) and handlerRemoved(...) is always called from the EventExecutor that also invokes the other methods of the ChannelHandler. Also we need to ensure we always call handlerAdded(...) before any other method can be calld to ensure correct ordering.
Motifications:
- Ensure that the right thread is used to call the methods
- Ensure correct ordering
- Add tests
Result:
Respect the thread-model for handlerAdded(...) and handlerRemoved(...) and preserve correct ordering in all cases.
Motivation:
When a channel was registered before and is re-registered we need to respect ChannelConfig.isAutoRead() and so start reading one the registration task completes. This was done "by luck" before 15162202fb82e2293624a86bfc27a9c5c35960be.
Modifications:
Explicit start reading once a Channel was re-registered if isAutoRead() is true.
Result:
Correctly receive data after re-registration completes.
Motivation:
Due a regression introduced by e969b6917c848c83f02617386f0f73d8f0e130a2 we missed to pass the original ChannelPromise to the next ChannelOutboundHandler and so
may never notify the origin ChannelPromise. This is related to #4805.
Modifications:
- Correctly pass the ChannelPromise
- Add unit test.
Result:
Correctly pass the ChannelPromise on deregister(...)
Motivation:
The AbstractChannel(Channel parent) constructor was previously hard-coded to always
call DefaultChannelId.newInstance(), and this made it difficult to use a custom
ChannelId implementation with some commonly used Channel implementations.
Modifications:
Introduced newId() method in AbstractChannel, which by default returns
DefaultChannelId.newInstance() but can be overridden by subclasses. Added
ensureDefaultChannelId() test to AbstractChannelTest, to ensure the prior
behavior of calling DefaultChannelId.newInstance() still holds with the
AbstractChannel(Channel parent) constructor.
Result:
AbstractChannel now has the protected newId() method, but there is no functional
difference.
Motivation:
ChannelInboundHandler and ChannelOutboundHandler both can implement exceptionCaught(...) method and so we need to dispatch to both of them.
Modifications:
- Correctly first dispatch exceptionCaught to the ChannelInboundHandler but also make sure the next handler it will be dispatched to will be the ChannelOutboundHandler
- Add removeInboundHandler() and removeOutboundHandler() which allows to remove one of the combined handlers
- Let *Codec extends it and not ChannelHandlerAppender
- Remove ChannelHandlerAppender
Result:
Correctly handle events and also have same behavior as in 4.0
Motivation:
Our contract in Channels is that the promise should always be notified before the actual callbacks of the ChannelInboundHandler are called. This was not done in the LocalChannel and so the behavior was different to other Channel implementations.
Modifications:
- First complete the ChannelPromise then call fireChannelActive()
- Guard against NPE when doClose() was called before the task was executed.
Result:
Consistent behavior between LocalChannel and other Channel implementations.
Motivation:
In AbstractChannelTest we not correctly mocked some methods which could lead to test errors. That said it only showed up here when running from the IDE and not from the cmdLine.
Modifications:
Mock methods that are needed for the test
Result:
Test pass in the IDE as well.
Motivation:
At the moment EmbeddedChannel always handle close() and disconnect() the same way which also means that ChannelOutboundHandler.disconnect(...) will never called. We should allow to specify if these are handle different or not to make the use of EmbeddedChannel more flexible.
Modifications:
Add 2 other constructors which allow to specify if disconnect / close are handled the same way or differently.
Result:
More flexible usage of EmbeddedChannel possible.
Motivation:
As discussed in #4529, NameResolver design shouldn't be resolving SocketAddresses (or String name + port) and return InetSocketAddresses. It should resolve String names and return InetAddresses.
This SocketAddress to InetSocketAddresses resolution is actually a different concern, used by Bootstrap.
Modifications:
Extract SocketAddress to InetSocketAddresses resolution concern to a new class hierarchy named AddressResolver.
These AddressResolvers delegate to NameResolvers.
Result:
Better separation of concerns.
Note that new AddressResolvers generate a bit more allocations because of the intermediate Promise and List<InetAddress>.
Motivation:
ChannelMetadata has a field minMaxMessagesPerRead which can be confusing. There are also some cases where static instances are used and the default value for channel type is not being applied.
Modifications:
- use a default value which is set unconditionally to simplify
- make sure static instances of MaxMessagesRecvByteBufAllocator are not used if the intention is that the default maxMessagesPerRead should be derived from the channel type.
Result:
Less confusing interfaces in ChannelMetadata and ChannelConfig. Default maxMessagesPerRead is correctly applied.
Motivation:
The CoalescingBufferQueueTest is somewhat relaxed with its releasing of test buffers, using safeRelease to generically deal with tests that may or may not release the buffers. SafeRelease generates logs, however, when the release fails.
Modifications:
Tightened up the individual test methods to verify that the buffers are released properly.
Result:
Fixes#4497
Motivation:
The previous DefaultChannelPipeline#destroy() implementation, introduced in #3156, is suboptimal as it can cause the for loop to continuously spin if the executor used by a given handler is unable to "recognize" the event loop.
It could be objected that it's the custom executor responsibility to properly implement the inEventLoop() method, but some implementetaions might not be able to do that for performance reasons, and even so, it's always better to be safe against API misuse, in particular when it is not possible to fail fast and the alternative is rather some sutle behaviour.
Modifications:
The patch simply avoids the recursive spin by explicitly passing the "in event loop" condition as a boolean parameter, preserving the same guarantees offered by #3156. A unit test has also been added.
Result:
All channel events are correctly called and no high CPU usage is seen anymore.
Motivation:
Once a FixedChannelPool was closed we must not allow to acquire or release Channels to prevent assert errors.
Modifications:
Fail release and acquire calls when FixedChannelPool is closed.
Result:
No more assert errors.1
Motivation:
EmbeddedChannelId#hashCode() and equals() do not behave correctly if an
instance is serialized and then deserialized. Additionally,
EmbeddedChannel does not allow use of any other type of ChannelId, and
EmbeddedChannelId is (mostly) a singleton instance. This creates a
problem for unit tests that construct multiple EmbeddedChannels and
expect each channel to have a unique ID.
Modifications:
EmbeddedChannelId is modified so equals() will return true and
hashCode() will return the same value for any instance of the class.
EmbeddedChannel is modified to allow a ChannelId to be specified when
the channel is constructed. Tests added for both changes.
Result:
EmbeddedChannelId now behaves correctly when deserialized, and
EmbeddedChannels can now have unique IDs.
Motivation:
PendingWriteQueueTest needs some cleanup.
Modifications:
- Cleanup code to remove deprecation warnings
- use static imports
Result:
No more warnings
Motivation:
We missed to run all pending tasks when EmbeddedChannel.close(...) or disconnect(...) was called. Because of this channelInactive(...) / channelUnregistered(...) of the handlers were never called.
Modifications:
Correctly run all pending tasks and cancel all not ready scheduled tasks when close or disconnect was called.
Result:
Correctly run tasks on close / disconnect and have channelInactive(...) / channelUnregistered(...) called.
Motivation:
If LocalChannel doWrite executes while the peer's state changes from CONNECTED to CLOSED it is possible that some promise's won't be completed and buffers will be leaked.
Modifications:
- Check the peer's state in doWrite to avoid a race condition
Result:
All write operations should release, and the associated promise should be completed.
