Motivation:
We should preferable always release the message before we notify the promise. Thhis has a few advantages:
- Release memory as soon as possible
- Listeners observe the "more correct" reference count
Modifications:
Release message before fail the promises
Result:
Faster releasing of resources. This came up in https://github.com/netty/netty/issues/10723
Motivation:
Since GraalVM version 19.3.0, instances of java.net.InetAddress (and its subclasses Inet4Address and Inet6Address) are not allowed in native image heap (that is, they cannot be stored in static fields of classes initialized at build time or be reachable through static fields of such classes). When building a native image, it makes sense to initialize at build time as many classes as possible.
But some fields of some classes in Netty (for example, NetUtil.LOCALHOST4) contain InetAddress instances. If a program is using code path that makes it possible to reach such fields at build time initialization, it becomes impossible to build a native image initializing core Netty classes initialized at runtime. An example of such a program is a client that uses netty-dns.
Modifications:
- Add netty-testsuite-native-image-client Maven module to test that such an example program can be built after the corresponding fixes
- Add native-image.properties to resolver-dns module to move initialization of some classes to runtime (some of them are parsing configuration during initialization, so it makes no sense to initialize them at build time; for others, it's needed to avoid InetAddress reachability at build time)
- Add substitutions for NetUtil.LOCALHOST4, NetUtil.LOCALHOST6 and NetUtil.LOCALHOST to overcome the InetAddress-related prohibition
- Extract some initialization code from NetUtil to NetUtilInitializations to allow it to be used by the substitutions
Result:
A client program using netty-dns with --initialize-at-build-time=io.netty builds successfully
Motivation:
Thread.stop() works by producing a ThreadDeath error in the target thread. EventLoops swallow all Throwables, which makes them effectively unkillable. This is effectively a memory leak, for our application. Beside this we should also just regrow all `Error` as there is almost no way to recover.
Modification:
Edit the EventLoops that swallow Throwables to instead rethrow Error.
Result:
`EventLoop` can crash if `Error` is thrown
Motivation:
HTTP is a plaintext protocol which means that someone may be able
to eavesdrop the data. To prevent this, HTTPS should be used whenever
possible. However, maintaining using https:// in all URLs may be
difficult. The nohttp tool can help here. The tool scans all the files
in a repository and reports where http:// is used.
Modifications:
- Added nohttp (via checkstyle) into the build process.
- Suppressed findings for the websites
that don't support HTTPS or that are not reachable
Result:
- Prevent using HTTP in the future.
- Encourage users to use HTTPS when they follow the links they found in
the code.
Motivation:
I was collecting stats for failed promises with a FixedChannelPool and I was bucketing by stats using cause.getSimpleName(). After #9152 was released, the introduction of the anonymous classes make getSimpleName() return "" causing confusion.
Modification:
Use named classes in the ChannelPool implementations. I made them private, but I can change that if you think otherwise.
Result:
The SimpleChannelPool fails the promises with a ChannelPoolFullException. The FixedChannelPool fails the promises with an AcquireTimeoutException. Also AcquireTimeoutException is more specific than just a plain TimeoutException, which is also useful for troubleshooting. If you want different class names, please advise.
Motivation:
DuplexChannel allow for half-closure, we should have a special config interface for it as well.
Modifications:
Add DuplexChannelConfig which allows to configure half-closure.
Result:
More consistent types
Motivation:
junit deprecated Assert.assertThat(...)
Modifications:
Use MatcherAssert.assertThat(...) as replacement for deprecated method
Result:
Less deprecation warnings
Motivation:
LGTM reports multiple issues. They need to be triaged,
and real ones should be fixed.
Modifications:
- Fixed multiple issues reported by LGTM, such as redundant conditions,
resource leaks, typos, possible integer overflows.
- Suppressed false-positives.
- Added a few testcases.
Result:
Fixed several possible issues, get rid of false alarms in the LGTM report.
Motivation:
We have a few classes in which we store and reuse static instances of various exceptions. When doing so it is important to also override fillInStacktrace() and so prevent the leak of the ClassLoader in the internal backtrace field.
Modifications:
- Add overrides of fillInStracktrace when needed
- Move ThrowableUtil usage in the static methods
Result:
Fixes https://github.com/netty/netty/pull/10686
Motivation:
We should use an initial buffer size with is >= 1500 (which is a common setting for MTU) to reduce the need for memory copies when a new connection is established. This is especially interesting when SSL / TLS comes into the mix.
This was ported from swiftnio:
https://github.com/apple/swift-nio/pull/1641
Modifications:
Increase the initial size from 1024 to 2048.
Result:
Possible less memory copies on new connections
Motivation:
Creating exceptions is expensive so we should only do so if really needed.
Modifications:
Only create the ConnectTimeoutException if we really need it.
Result:
Less overhead
Motivation:
In some benchmarks closing the Channel attributes to a lot of overhead due the call of fillInStackTrace(). We should reduce this overhead.
Modifications:
- Create a StacklessClosedChannelException and use it to reduce overhead.
- Only call ChannelOutboundBuffer.failFlushed(...) when there was a flushed message at all.
Result:
Less performance overhead when closing the Channel
Motivation:
I was working on the transport part in Netty (ofc, solving a major issue) and I found this typo so thought to fix it.
Modification:
Fixed Typo
Result:
No more confusion between `us` and `use`.
Motivation:
At some point a ChannelHandlerContext did have its own AttributeMap which is not true since 4.1.x was released. Unfortunally we missed to update the javadocs and so these don't reflect reality
Modifications:
Update javadocs
Result:
Fixes https://github.com/netty/netty/issues/10477
Motivation:
When we were using the netty http protocol, OOM occurred, this problem has been in 4.1.51.Final Fix [# 10424](https://github.com/netty/netty/issues/10424), even if OOM is up, the service will still receive new connection events, will occur again OOM and eventually cause the connection not to be released.
code `byteBuf = allocHandle.allocate(allocator);`
Modification:
I fail to create buffer when I try to receive new data, i determine if it is OOM then the close read event releases the connection.
```java
if (close || cause instanceof OutOfMemoryError || cause instanceof IOException) {
closeOnRead(pipeline);
}
```
Result:
Fixes # [10434](https://github.com/netty/netty/issues/10434).
Motivation:
When a switch statement is used we should always define a `default:` so we don't introduce bugs due fall-through.
Modification:
Add missing `default:`s
Result:
Less error-prone code
Motivation:
Seems like some users are suprised by the behaviour of DefaultEventExecutor when used within the ChannelPipeline. We should clarify the semantics and also mention UnordedThreadPoolEventExecutor which may be more inline with their expectations
Modifications:
Add javadocs section about UnorderedThreadPoolEventExecutor and expand details for DefaultEventExecutor
Result:
Clarify sematics
Motivation:
AbstractCoalescingBufferQueue had a bug which could lead to an empty queue while still report bytes left. This was due the fact that we decremented the pending bytes before draining the queue one-by-one. The problem here is that while the queue is drained we may notify the promise which may add again buffers to the queue for which we never decrement the bytes while we drain these
Modifications:
- Decrement the pending bytes every time we drain a buffer from the queue
- Add unit tests
Result:
Fixes https://github.com/netty/netty/issues/10286
Motivation:
In AbstractChannelHandlerContext we had some code where we tried to guard against endless loops caused by exceptions thrown by exceptionCaught(...) that would trigger exceptionCaught again. This code was proplematic for two reasons:
- It is quite expensive as we need to compare all the stacks
- We may end up not notify another handlers exceptionCaught(...) if in our exeuction stack we triggered actions that will cause an exceptionCaught somewhere else in the pipeline
Modifications:
- Just remove the detection code as we already handle everything correctly when we invoke exceptionCaught(...)
- Add unit tests
Result:
Ensure we always notify correctly and also fixes performance issue reported as https://github.com/netty/netty/issues/10165
Motivation:
Parameter maxCount needs the unit test.
Modifications:
1. Change the conditional statement to avoid the ineffective maxCount (enhance the robustness of the code merely).
2. Add the unit test for maxCount.
Result:
Enable this parameter to be tested.
Motivation:
We need to ensure we always close the Channel when we see an exception during bootstrapping it.
Modification:
Ensure we correct close the Channel if we see an exception during retrieving the Resolver from the group while bootstrapping.
Result:
Fixes#10109
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
Netty's DefaultThreadFactory that creates FastThreadLocalThread instance is widely used in NioEventLoopGroup, EpollEventLoopGroup, etc, but not OioEventLoopGroup. Although oio is quite stale, I still think this change may be useful.
Modification:
Replace oio's default thread factory with netty's DefaultThreadFactory just like NioEventLoopGroup, EpollEventLoopGroup, etc.
Result:
Faster access to FastThreadLocal in oio.
Motivation:
In general, we will close the debug log in a product environment. However, logging without external level check may still affect performance as varargs will need to allocate an array.
Modification:
Add log level check simply before logging.
Result:
Improve performance slightly in a product environment.
Motivation:
When the HttpContentCompressor is put on an alternate EventExecutor, the order of events should be
preserved so that the compressed content is correctly created.
Modifications:
- checking that the executor in the ChannelHandlerContext is the same executor as the current executor when evaluating if the handler should be skipped.
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/10067
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
We should not include the number of ServerChannel that are part of the DefaultChannelGroup when specify the initial size of the LinkedHashMap
Modifications:
Only use the number of the non ServerChannel
Result:
Reduce memory-footprint
Motivation:
The method body of fillInStackTrace method of IllegalStateException class in SimpleChannelPool.java is so simple (just return this) that there is no need to be marked as synchronized.
Modification:
Remove the synchronized flag of fillInStackTrace method.
Result:
It can improve performance slightly while creating a IllegalStateException instance.
Motivation:
The method body of fillInStackTrace method of TimeoutException class in FixedChannelPool.java is so simple (just return this) that there is no need to be marked as synchronized.
Modification:
Remove the synchronized flag of fillInStackTrace method.
Result:
It can improve performance slightly while throwing a TimeoutException.
Motivation:
Current pipeline handler replace tests are replacing handler with same name.
we need test that test handler can renamed, old handlers are removed from pipeline.
Modification:
There is coverage missing related to renaming of handlers
Result:
Adds missing tests
Co-authored-by: phani254 <phani254@yahoo.com>
Motivation:
NioEventLoopTest.testChannelsRegistered sometimes fails due a race which is related to how SelectionKey and Selector is implemented in the JDK. In the current implementation it will "lazy" remove SelectionKeys from the Set which means we may still have these included sometimes when we use size() to get the number of SelectionKeys.
Modifications:
Just retry to read the number of registered Channels if we still see 2
Result:
Fixes https://github.com/netty/netty/issues/9895
Motivation:
https://github.com/netty/netty/pull/9458 changed how we handled ChannelOptions internally to use a ConcurrentHashMap. This unfortunally had the side-effect that the ordering may be affected and not stable anymore. Here the problem is that sometimes we do validation based on two different ChannelOptions (for example we validate high and low watermarks against each other). Thus even if the user specified the options in the same order we may fail to configure them.
Modifications:
- Use again a LinkedHashMap to preserve order
- Add unit test
Result:
Apply ChannelOptions in correct and expected order
Motivation
This test is failing intermittently and upon inspection has a race
condition.
Modification
Fix race condition by waiting for async release calls to complete prior
to closing the pool.
Result
Hopefully fixed flakey test
