Motivation
In #8758, @doom369 reported an infinite loop bug in CompositeByteBuf
which was introduced in #8437.
This is the same small fix for that, along with fixes for two other bugs
found while re-inspecting the changes and adding unit tests.
Modification
- Replace recursive call to toComponentIndex with toComponentIndex0 as
intended
- Add missed "lastAccessed" racy cache invalidation in capacity(int)
method
- Fix incorrect determination of initial offset in non-zero cIndex case
of updateComponentOffsets method
- New unit tests for previously uncovered methods
Results
Fewer bugs.
Motivation:
We need to release the message when we throw an IllegalArgumentException because of a validation failure of the promise to eliminate the risk of a memory leak.
Modifications:
- Consistently release the message before rethrow
- Add testcase.
Result:
Fixes https://github.com/netty/netty/issues/8765.
* Decouble EventLoop details from the IO handling for each transport to allow easy re-use of code and customization
Motiviation:
As today extending EventLoop implementations to add custom logic / metrics / instrumentations is only possible in a very limited way if at all. This is due the fact that most implementations are final or even package-private. That said even if these would be public there are the ability to do something useful with these is very limited as the IO processing and task processing are very tightly coupled. All of the mentioned things are a big pain point in netty 4.x and need improvement.
Modifications:
This changeset decoubled the IO processing logic from the task processing logic for the main transport (NIO, Epoll, KQueue) by introducing the concept of an IoHandler. The IoHandler itself is responsible to wait for IO readiness and process these IO events. The execution of the IoHandler itself is done by the SingleThreadEventLoop as part of its EventLoop processing. This allows to use the same EventLoopGroup (MultiThreadEventLoupGroup) for all the mentioned transports by just specify a different IoHandlerFactory during construction.
Beside this core API change this changeset also allows to easily extend SingleThreadEventExecutor / SingleThreadEventLoop to add custom logic to it which then can be reused by all the transports. The ideas are very similar to what is provided by ScheduledThreadPoolExecutor (that is part of the JDK). This allows for example things like:
* Adding instrumentation / metrics:
* how many Channels are registered on an SingleThreadEventLoop
* how many Channels were handled during the IO processing in an EventLoop run
* how many task were handled during the last EventLoop / EventExecutor run
* how many outstanding tasks we have
...
...
* Implementing custom strategies for choosing the next EventExecutor / EventLoop to use based on these metrics.
* Use different Promise / Future / ScheduledFuture implementations
* decorate Runnable / Callables when submitted to the EventExecutor / EventLoop
As a lot of functionalities are folded into the MultiThreadEventLoopGroup and SingleThreadEventLoopGroup this changeset also removes:
* AbstractEventLoop
* AbstractEventLoopGroup
* EventExecutorChooser
* EventExecutorChooserFactory
* DefaultEventLoopGroup
* DefaultEventExecutor
* DefaultEventExecutorGroup
Result:
Fixes https://github.com/netty/netty/issues/8514 .
Motivation:
We should leave the responsibility to choose the EventExecutor for a ChannelHandler to the user for more flexibility and to keep things simple.
Modification:
- Change method signatures to take an EventExecutor and not an EventExecutorGroup
- Remove special ChannelOption that allowed to enable / disable EventExecutor pinning
Result:
Simpler and more flexible code.
Motivation:
Custom Netty ThreadLocalRandom and ThreadLocalRandomProvider classes are no longer needed and can be removed.
Modification:
Remove own ThreadLocalRandom
Result:
Less code to maintain
Motivation:
PlatformDependent.newConcurrentHashMap() is no longer needed so it could be easily removed and new ConcurrentHashMap<>() inlined instead of invoking PlatformDependent.newConcurrentHashMap().
Modification:
Use ConcurrentHashMap provided by the JDK directly.
Result:
Less code to maintain.
Motivation:
No need to initialize charsets from the string. We can take already allocated charset from StandardCharsets class.
Modification:
Replace Charset.forName("US-ASCII") with StandardCharsets.US_ASCII.
Removed Charset[] values() method and internal static variable as it was used only in tests.
Result:
Reuse what the JDK provides
Motivation:
Simplify code with newer Java API. It also reduce levels of indirection, but I don't
think we can gain any visible performance improvements out of this.
Modifications:
Replace Collection.newSetFromMap(new ConcurrentHashMap()) with ConcurrentHashMap.newKeySet().
Result:
Use Java8 APIs.
Motivation:
We can use the diamond operator these days.
Modification:
Use diamond operator whenever possible.
Result:
More modern code and less boiler-plate.
Motivation:
Replace "if else" conditions with string switch. It is easier to read the code, for large "if else" constructions switch also could be faster.
Modification:
Replaced "if else" with a string switch.
Result:
Use new language features
Motivation:
Since Java 7 we can automatically close resources in try () construction.
Modification:
Changed all try catches in the code with autoclose try (resource)
Result:
Less boiler-plate
Motivation:
After switching to Java 8 we no longer need LongCounter and LongAdderCounter. We can directly replace them with LongAdder.
Modification:
Removed LongCounter and LongAdderCounter classes.
Result:
Less code to maintain
Motivation:
Netty uses own Integer.compare and Long.compare methods. Since Java 7 we can use Java implementation instead.
Modification:
Remove own implementation
Result:
Less code to maintain
Motivation:
IntergerHolder was left from some old days and not used anymore.
Modification:
Remove IntergerHolder from the codebase.
Result:
Removed unused code
Motivation:
The concurrent set is present in Java 8 and above so we can use it instead of own implementation.
Modification:
io.netty.utik.internal.ConcurrentSet replaced with ConcurrentHashMap.newKeySet().
Result:
Less code to maintain.
Motivation:
Invoking ChannelHandlers is not free and can result in some overhead when the ChannelPipeline becomes very long. This is especially true if most handlers will just forward the call to the next handler in the pipeline. When the user extends Channel*HandlerAdapter we can easily detect if can just skip the handler and invoke the next handler in the pipeline directly. This reduce the overhead of dispatch but also reduce the call-stack in many cases.
Modifications:
Detect if we can skip the handler when walking the pipeline.
Result:
Reduce overhead for long pipelines.
Benchmark (extraHandlers) Mode Cnt Score Error Units
DefaultChannelPipelineBenchmark.propagateEventOld 4 thrpt 10 267313.031 ± 9131.140 ops/s
DefaultChannelPipelineBenchmark.propagateEvent 4 thrpt 10 824825.673 ± 12727.594 ops/s
Motivation:
Current implementation extract header value as String. We have an idiomatic way for checking presence of a header value.
Modification:
Use HttpHeaders#contains for checking if if contains Expect: 100-continue.
Result:
Use idiomatic way + simplify boolean logic.
Motivation:
While we are not yet quite sure if we want to require Java11 as minimum we are at least sure we want to use java8 as minimum.
Modifications:
Change minimum version to java8 and update some tests which failed compilation after this change.
Result:
Use Java8 as minimum and be able to use Java8 features.
Motivation:
testChannelInitializerEventExecutor() did sometimes fail as we sometimes miss to count down the latch. This can happen when we remove the handler from the pipeline before channelUnregistered(...) was called for it.
Modifications:
Countdown the latch in handlerRemoved(...).
Result:
Fix flaky test.
Motivation:
I want to fix bug in vert.x project (eclipse-vertx/vert.x#2562) caused by ComposedLastHttpContent result being null. I don't know if it is intentional that this last decoded chuck in the issue returns null, but if not - I am providing fix for that.
Modification:
* Added new constructor in ComposedLastHttpContent allowing to pass DecoderResult
* set DecoderResult.SUCCESS for created ComposedLastHttpContent in HttpContentEncoder
* set DecoderResult.SUCCESS for created ComposedLastHttpContent in HttpContentDecoder
Result:
Fixeseclipse-vertx/vert.x#2562
Motivation:
While using Load Balancers or HA support is needed there are cases when UDP channel need to bind to IP Address which is not available on network interfaces locally.
Modification:
Modified EpollDatagramChannelConfig to allow IP_FREEBIND option
Result:
Fixes ##8727.
Motivation:
testWriteTaskRejected was racy as we did not ensure we dispatched all events to the executor before shutting it down.
Modifications:
Add a latch to ensure we dispatched everything.
Result:
Fix racy test that failed sometimes before.
Motivation:
When a write error happens during writing of flowcontrolled data frames we miss to correctly detect this in the write loop which may result in an infinite loop as we will never detect that the frame should be removed from the queue.
Modifications:
- When we fail a flowcontrolled data frame we ensure that the next frame.write(...) call will signal back that the whole frame was handled and so can be removed.
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8707.
Motivation:
When using multiple nameservers and a nameserver respond with NXDOMAIN we should only fail the query if the nameserver in question is authoritive or no nameservers are left to try.
Modifications:
- Try next nameserver if NXDOMAIN was returned but the nameserver is not authoritive
- Adjust testcase to respect correct behaviour.
Result:
Fixes https://github.com/netty/netty/issues/8261
Motivation:
Since the updating to OpenJDK 11.0.2 the OSGI testsuite fails. We should dissable it until there is a version of the used plugins that works with this OpenJDK version.
Modifications:
Skip osgi testsuite when using JDK11.
Result:
Build pass again with JDK11.
Motiviation:
Because of how we implemented the registration / deregistration of an EventLoop it was not possible to wrap an EventLoop implementation and use it with a Channel.
Modification:
- Introduce EventLoop.Unsafe which is responsible for the actual registration.
- Move validation of EventLoop / Channel combo to the EventLoop
- Add unit test that verifies that wrapping works
Result:
Be able to wrap an EventLoop and so add some extra functionality.
Motivation:
We should always build with the latest JDK releases.
Modifications:
Update JDK8 and JDK11 versions to the latest.
Result:
Run CI jobs on the latest JDK release.
Motivation:
The CorruptedFrameException from the finish() method of the Utf8Validator gets propagated to other handlers while the connection is still open.
Modification:
Override exceptionCaught method of the Utf8FrameValidator and close the connection if it is a CorruptedFrameException.
Result:
The CorruptedFrameException gets propagated to other handlers only after properly closing the connection.
Motivation:
There are the first EA bulds for OpenJDK 13. We should support to build with it and run builds on the CI.
Modifications:
- Add profile for JDK 13
- Add docker config to run with JDK 13.
Result:
Building and testing with OpenJDK 13 is possible.
Motivation:
ChunkedWriteHandler should report write operation as failed
in case *any* chunked was not written. Right now this is not
true for the last chunk.
Modifications:
* Check if the appropriate write operation was succesfull when
reporting the last chunk
* Skip writing chunks if the write operation was already marked
as "done"
* Test cases to cover write failures when dealing with chunked input
Result:
Fix https://github.com/netty/netty/issues/8700
Motivation:
We should access the Constructor of the passed in class in the Constructor of ReflectiveChannelFactory only to reduce the overhead but also fail-fast.
Modifications:
Access the Constructor early.
Result:
Fails fast and less performance overhead.
Motivation:
Incrementing two variables in sync is not necessary when only one will do.
Modifications:
- Remove `j` from `for` loop and replace with `i`.
- Add more unit testing scenarios to cover changed code.
Results:
Unnecessary variable removed.
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:
The javadocs stating `IndexOutOfBoundsException` is thrown were
different from what `ByteBuf` actually did. We want to ensure the
Javadocs represent reality.
Modifications:
Updated javadocs on `write*`, `ensureWriteable`, `capacity`, and
`maxCapacity` methods.
Results:
Javadocs more closely match actual behaviour.
Motivation:
A new EA release was done for OpenJDK12.
Modifications:
Use OpenJDK12 EA 27 when running CI jobs for JDK 12.
Result:
Test against latest OpenJDK 12 EA build.
Motivation:
We should remove the ChannelPool and related implementations. It is often the case that having protocol knowledge can result in more effective pooling and ChannelPool currently doesn’t have this knowledge. This responsibility is assumed to be implemented at layers higher in the stack than Netty.
Modifications:
Remove io.netty.channel.pool.*
Result:
Less code to maintain, fixes https://github.com/netty/netty/issues/8549.
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.
Motivation:
Due a race in DefaultChannelPipeline / AbstractChannelHandlerContext it was possible to have only handlerRemoved(...) called during tearing down the pipeline, even when handlerAdded(...) was never called. We need to ensure we either call both of none to guarantee a proper lifecycle of the handler.
Modifications:
- Enforce handlerAdded(...) / handlerRemoved(...) semantics / ordering
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8676 / https://github.com/netty/netty/issues/6536 .
Motivation:
We do not correctly detect loops when follow CNAMEs and so may try to follow it without any success.
Modifications:
- Correctly detect CNAME loops
- Do not cache CNAME entries which point to itself
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/8687.
