Motivation
A memory leak related to DNS resolution was reported in #9634,
specifically linked to the TCP retry fallback functionality that was
introduced relatively recently. Upon inspection it's apparent that there
are some error paths where the original UDP response might not be fully
released, and more significantly the TCP response actually leaks every
time on the fallback success path.
It turns out that a bug in the unit test meant that the intended TCP
fallback path was not actually exercised, so it did not expose the main
leak in question.
Modifications
- Fix DnsNameResolverTest#testTruncated0 dummy server fallback logic to
first read transaction id of retried query and use it in replayed
response
- Adjust semantic of internal DnsQueryContext#finish method to always
take refcount ownership of passed in envelope
- Reorder some logic in DnsResponseHandler fallback handling to verify
the context of the response is expected, and ensure that the query
response are either released or propagated in all cases. This also
reduces a number of redundant retain/release pairings
Result
Fixes#9634
Motivation:
We can use the `@SuppressJava6Requirement` annotation to be more precise about when we use Java6+ APIs. This helps us to ensure we always protect these places.
Modifications:
Make use of `@SuppressJava6Requirement` explicit
Result:
Fixes https://github.com/netty/netty/issues/2509.
Motivation:
There is not need to use a CAS as everything is synchronized anyway. We can simplify the code a bit by not using it.
Modifications:
- Just remove the CAS operation
- Change from int to boolean
Result:
Code cleanup
Motivation
The recently-introduced event loop scheduling hooks can be exploited by
the epoll transport to avoid waking the event loop when scheduling
future tasks if there is a timer already set to wake up sooner.
There is also a "default" timeout which will wake the event
loop after 1 second if there are no pending future tasks. The
performance impact of these wakeups themselves is likely negligible but
there's significant overhead in having to re-arm the timer every time
the event loop goes to sleep (see #7816). It's not 100% clear why this
timeout was there originally but we're sure it's no longer needed.
Modification
Combine the existing volatile wakenUp and non-volatile prevDeadlineNanos
fields into a single AtomicLong that stores the next scheduled wakeup
time while the event loop is in epoll_wait, and is -1 while it is awake.
Use this as a guard to debounce wakeups from both immediate scheduled
tasks and future scheduled tasks, the latter using the new
before/afterScheduledTaskSubmitted overrides and based on whether the
new deadline occurs prior to an already-scheduled timer.
A similar optimization was already added to NioEventLoop, but it still
uses two separate volatiles. We should consider similar streamlining of
that in a future update.
Result
Fewer event loop wakeups when scheduling future tasks, greatly reduced
overhead when no future tasks are scheduled.
Motivation:
Currently when the SslHandler coalesces outbound bytes it always
allocates a direct byte buffer. This does not make sense if the JDK
engine is being used as the bytes will have to be copied back to heap
bytes for the engine to operate on them.
Modifications:
Inspect engine type when coalescing outbound bytes and allocate heap
buffer if heap bytes are preferred by the engine.
Result:
Improved performance for JDK engine. Better performance in environments
without direct buffer pooling.
Motivation:
We should use the latest commons-compress release to fix CVE-2019-12402 (even it is only a test dependency)
Modifications:
Update commons-compress to 1.19
Result:
Fix security alert
### Motivation:
I've now found two libraries that use Netty to be vulnerable to [CWE-113: Improper Neutralization of CRLF Sequences in HTTP Headers ('HTTP Response Splitting')](https://cwe.mitre.org/data/definitions/113.html) due to using `new DefaultHttpHeaders(false)`.
Some part of me hopes that this warning will help dissuade library authors from disabling this important security check.
### Modification:
Add documentation to `DefaultHttpHeaders(boolean)` to warn about the implications of `false`.
### Result:
This improves the documentation on `DefaultHttpHeaders`.
Motivation:
In the current implementation of Base64 decoder an invalid
character `\u00BD` treated as `=`.
Also character `\u007F` leads to ArrayIndexOutOfBoundsException.
Modification:
Explicitly checks that all input bytes are ASCII characters
(greater than zero). Fix `decodabet` tables.
Result:
Correctly validation input bytes in Base64 decoder.
Motivation:
031c2e2e88 introduced some change to reduce the risk of have the `ReferenceCountedOpenSslContext` be destroyed while the `ReferenceCountedSslEngine` is still in us. Unfortunaly it missed to adjust a few tests which make assumptions about the refCnt of the context.
Modifications:
Adjust tests to take new semenatics into acount.
Result:
No more tests failures
Motivation:
We should not commit vscode specific files, so at it to gitignore
Modifications:
Add files to .gitignore
Result:
Correctly ignore ide related files
Motivation:
We should use adaptjdk 13 and not oracle openjdk 13 when building with Java 13
Modifications:
Use adopt@1.13.0-0
Result:
More consistent java vendor usage
Motivation
NioEventLoopTest#testChannelsRegistered() fails intermittently due to
use of SingleThreadEventLoop#channelsRegistered() which is not
threadsafe and unreliable when called from outside the event loop.
Modifications
Add static registeredChannels method to NioEventLoopTest and
AbstractSingleThreadEventLoopTest to call from the tests via event loop
instead of directly.
Result
Hopefully fewer test failures
Motivation:
Http2MultiplexCodec extends Http2FrameCodec extends Http2ConnectionHandler. It appears Http2MultiplexCodec overrode the channelWritabilityChanged method, which prevented the flow controller from becoming active. In the case the parent channel becomes unwritable, and then later becomes writable, it needs to indicate that the child channels can still write data. This is slightly confusing, because the child channels may still themselves be unwritable, but should still drain their data to the parent channel.
Modification:
Still propagate writability changes to the HTTP/2 flow controller
Result:
Fixes https://github.com/netty/netty/issues/9636
Motivation:
There is a goto statement above the current position of initialize dynamicMethods, and dynamicMethods is used after the goto which might cause undefined behavior.
Modifications:
Initialize dynamicMehtods at the top.
Result:
No more undefined behavior.
Motivation
This is a vestige that was removed in the original PR #9535 before it
was reverted, but we missed it when re-applying in #9586.
It means there is a possible race condition because a wakeup event could
be missed while shutting down, but the consequences aren't serious since
there's a 1 second safeguard timeout when waiting for it.
Modification
Remove call to epollWaitNow() in EpollEventLoop#closeAll()
Result
Cleanup redundant code, avoid shutdown delay race condition
Motivation
The current event loop shutdown logic is quite fragile and in the
epoll/NIO cases relies on the default 1 second wait/select timeout that
applies when there are no scheduled tasks. Without this default timeout
the shutdown would hang indefinitely.
The timeout only takes effect in this case because queued scheduled
tasks are first cancelled in
SingleThreadEventExecutor#confirmShutdown(), but I _think_ even this
isn't robust, since the main task queue is subsequently serviced which
could result in some new scheduled task being queued with much later
deadline.
It also means shutdowns are unnecessarily delayed by up to 1 second.
Modifications
- Add/extend unit tests to expose the issue
- Adjust SingleThreadEventExecutor shutdown and confirmShutdown methods
to explicitly add no-op tasks to the taskQueue so that the subsequent
event loop iteration doesn't enter blocking wait (as looks like was
originally intended)
Results
Faster and more robust shutdown of event loops, allows removal of the
default wait timeout
Motivation:
Native image compilation is failing without extra flags:
```
Warning: Aborting stand-alone image build. No instances of io.netty.buffer.UnpooledHeapByteBuf are allowed in the image heap as this class should be initialized at image runtime. Object has been initialized by the io.netty.handler.codec.http2.Http2ConnectionHandler class initializer with a trace:
at io.netty.buffer.Unpooled.wrappedBuffer(Unpooled.java:157)
at io.netty.handler.codec.http2.Http2ConnectionHandler.<clinit>(Http2ConnectionHandler.java:74)
. To fix the issue mark io.netty.buffer.UnpooledHeapByteBuf for build-time initialization with --initialize-at-build-time=io.netty.buffer.UnpooledHeapByteBuf or use the the information from the trace to find the culprit and --initialize-at-run-time=<culprit> to prevent its instantiation.
Detailed message:
Trace: object io.netty.buffer.ReadOnlyByteBuf
object io.netty.buffer.UnreleasableByteBuf
method io.netty.handler.codec.http2.Http2ConnectionHandler.access$500()
Call path from entry point to io.netty.handler.codec.http2.Http2ConnectionHandler.access$500():
at io.netty.handler.codec.http2.Http2ConnectionHandler.access$500(Http2ConnectionHandler.java:66)
at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.readClientPrefaceString(Http2ConnectionHandler.java:299)
at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.decode(Http2ConnectionHandler.java:239)
at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:505)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:444)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:283)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.access$600(AbstractChannelHandlerContext.java:56)
at io.netty.channel.AbstractChannelHandlerContext$7.run(AbstractChannelHandlerContext.java:365)
at com.oracle.svm.core.jdk.RuntimeSupport.executeHooks(RuntimeSupport.java:144)
at com.oracle.svm.core.jdk.RuntimeSupport.executeStartupHooks(RuntimeSupport.java:89)
at com.oracle.svm.core.JavaMainWrapper.runCore(JavaMainWrapper.java:143)
at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:186)
at com.oracle.svm.core.code.IsolateEnterStub.JavaMainWrapper_run_5087f5482cc9a6abc971913ece43acb471d2631b(generated:0)
```
Modification:
Add `io.netty.handler.codec.http2.Http2ConnectionHandler` for runtime compilation, as the buffer library's `io.netty.buffer.UnpooledHeapByteBuf` is also marked for runtime.
Result:
Native image compilation works again.
Motivation:
`DefaultHttp2UnknownFrame#equals` may produce NPE due to
incorrect comparison of `stream` field.
Modification:
- Fix the `stream` field compare.
- Cleanup usage of class fields: use direct access instead of getters
(because the class is final).
Result:
No NPE in `equals` method.
Motivation:
Classes `AbstractHttp2StreamChannel.Http2StreamChannelConfig`
and `DnsNameResolver.AddressedEnvelopeAdapter` may be static:
it doesn't reference its enclosing instance.
Modification:
Add `static` modifier.
Result:
Prevents a possible memory leak and uses less memory per class instance.
Motivation:
With the Netty ref-counted OpenSSL implementation the parent SslContext
maintains state necessary for the SslEngine's it produces. However, it's
possible for the parent context to be closed and release those resources
before the child engines are finished which causes problems.
Modification:
Spawned ReferenceCountedOpenSslEngine's retain a reference to their
parent ReferenceCountedOpenSslContext.
Result:
The lifetime of the shared data is extended to include the lifetime of
the dependents.
Motivation:
On JDK > 9 Netty uses Unsafe to write two internal JDK fields: sun.nio.ch.SelectorImp.selectedKeys and sun.nio.ch.SelectorImpl.publicSelectedKeys. This is done in transport/src/main/java/io/netty/channel/nio/NioEventLoop.java:225, in openSelector() method. The GraalVM analysis cannot do the Unsafe registration automatically because the object field offset computation is hidden behind two layers of calls.
Modifications:
This PR updates the Netty GraalVM configuration by registering those fields for unsafe access.
Result:
Improved support for Netty on GraalVM with JDK > 9.
Motivation:
It is not possible to build Netty on an s390_64 platform.
Modifications:
Modify pom.xml so that s390_64 is acceptable as os.detected.arch.
Result:
Netty can be built on an s390_64 platform.
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Motivation:
We would like to enable Netty also on a big endian platform such as
s390_64. We need to fix a function which assumes that the target
platform is little endian.
Modifications:
Modify netty_unix_socket_accept() to write an address length as jbyte
instead of jsize.
Result:
Netty can be enabled on a big endian platform.
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Motivation:
Due a bug we did not always correctly calculate the next buffer size in AdaptiveRecvByteBufAllocator.
Modification:
Fix calculation and add unit test
Result:
Correct calculation is always used.
Motivation
When ScheduledFutureTasks complete, there's no need to retain a ref to
the wrapped task. Clearing it could help in particular with the case
where many scheduled tasks have been cancelled but their queue removal
delayed (since it is done lazily).
Modifications
This comprises just the PromiseTask changes from #9580. Upon completion,
replace the task reference with a static sentinel depending on the type
of completion (so that it will be reflected by toString).
Result
More expedient collection of cancelled task objects
Motivation:
The build script for the module Netty Transport Native Epoll can cause
intermittent build break due to broken pipe by /usr/bin/ldd. This issue
likely to occur on a build environment with multiple processors.
Modifications:
The root cause is that the consumer head command finishes earlier the
producer ldd command. Buffering the outputs of the ldd command by an
intermediate tail command avoids the broken pipe.
Result:
A build on multiple processors can finish successfully.
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Motivation:
At the moment we use Unpooled.buffer(...) in InboundHttp2ToHttpAdapter when we need to do a copy of the message. We should better use the configured ByteBufAllocator for the Channel
Modifications:
Change internal interface to also take the ByteBufAllocator as argument and use it when we need to allocate a ByteBuf.
Result:
Use the "correct" ByteBufAllocator in InboundHttp2ToHttpAdapter in all cases
Motivation:
At the moment we set release to false before we call writeData(...). This could let to the sitatuation that we will miss to release the message if writeData(...) throws. We should set release to false after we called writeData(...) to ensure the ownership of the buffer is correctly transferred.
Modifications:
- Set release to false after writeData(...) was successfully called only
Result:
No possibility for a buffer leak
Motivation:
We did miss to take Http2FrameCodecBuilder.isValidateHeaders() into account when a Http2FrameWriter was set on the builder and always assumed validation should be enabled.
Modifications:
Remove hardcode value and use configured value
Result:
Http2FrameCodecBuilder.isValidateHeaders() is respected in all cases
Motivation:
Recycler$Stack.pop will occurs `ArrayIndexOutOfBoundsException` in some race cases, we should double check `size` even after `scavenge` called.
Modifications:
Double check `size` after `scavenge`
Result:
avoid ArrayIndexOutOfBoundsException in `pop`
Motivation
Currently a static AtomicLong is used to allocate a unique id whenever a
task is scheduled to any event loop. This could be a source of
contention if delayed tasks are scheduled at a high frequency and can be
easily avoided by having a non-volatile id counter per queue.
Modifications
- Replace static AtomicLong ScheduledFutureTask#nextTaskId with a long
field in AbstractScheduledExecutorService
- Set ScheduledFutureTask#id based on this when adding the task to the
queue (in event loop) instead of at construction time
- Add simple benchmark
Result
Less contention / cache-miss possibility when scheduling future tasks
Before:
Benchmark (num) Mode Cnt Score Error Units
scheduleLots 100000 thrpt 20 346.008 ± 21.931 ops/s
Benchmark (num) Mode Cnt Score Error Units
scheduleLots 100000 thrpt 20 654.824 ± 22.064 ops/s
Motivation:
At the moment we not consistently (and also not correctly) free allocated native memory in all cases during loading the JNI library. This can lead to native memory leaks in the unlikely case of failure while trying to load the library.
Beside this we also not always correctly handle the case when a new java object can not be created in native code because of out of memory.
Modification:
- Copy some macros from netty-tcnative to be able to handle errors in a more easy fashion
- Correctly account for New* functions to return NULL
- Share code
Result:
More robust and clean JNI code
Motivation:
Users can reuse the same FileChannel for different ChunkedNioFile
instances without being worried that FileChannel::position will be
changed concurrently by them.
In addition, FileChannel::read with absolute position allows to
use on *nix pread that is more efficient then fread.
Modifications:
Always use absolute FileChannel::read ops
Result:
Faster and more flexible uses of FileChannel for ChunkedNioFile
Motivation
This is another iteration of #9476.
Modifications
Instead of maintaining a count of all writes performed and then using
reads during shutdown to ensure all are accounted for, just set a flag
after each write and don't reset it until the corresponding event has
been returned from epoll_wait.
This requires that while a write is still pending we don't reset
wakenUp, i.e. continue to block writes from the wakeup() method.
Result
Race condition eliminated. Fixes#9362
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
Error: Class that is marked for delaying initialization to run time got initialized during image building: io.netty.handler.codec.http2.Http2CodecUtil. Try marking this class for build-time initialization with --initialize-at-build-time=io.netty.handler.codec.http2.Http2CodecUtil
Error: Use -H:+ReportExceptionStackTraces to print stacktrace of underlying exception
Error: Image build request failed with exit status 1
Modification:
After debugging, it seems the culprit is io.netty.handler.codec.http2.Http2ClientUpgradeCodec, which also needs runtime initialisation.
Result:
Fixes #micronaut-projects/micronaut-grpc#8
Motivation:
It is not safe to cache a jclass without obtaining a global reference via NewGlobalRef.
Modifications:
Correctly use NewGlobalRef(...) before caching
Result:
Correctly cache jclass instance
Motivation:
We just released a new version of netty-tcnative.
Modifications:
Bump up to netty-tcnative 2.0.26.Final
Result:
Use latest netty-tcnative release
Motivation:
Due some bug we did endup with ClassCastExceptions in some cases. Beside this we also did not correctly handle the case when ReferenceCountedOpenSslEngineTest did produce tasks to run in on test.
Modifications:
- Correctly unwrap the engine before to fix ClassCastExceptions
- Run delegated tasks when needed.
Result:
All tests pass with different OpenSSL implementations (OpenSSL, BoringSSL etc)
Motivation:
Running tests with a `KQueueDomainSocketChannel` showed worse performance than an `NioSocketChannel`. It turns out that the default send buffer size for Nio sockets is 64k while for KQueue sockets it's 8k. I verified that manually setting the socket's send buffer size improved perf to expected levels.
Modification:
Plumb the `SO_SNDBUF` and `SO_RCVBUF` options into the `*DomainSocketChannelConfig`.
Result:
Can now configure send and receive buffer sizes for domain sockets.
Motivation:
Optimize the QueryStringEncoder for lower memory overhead and higher encode speed.
Modification:
Encode the space to + directly, and reuse the uriStringBuilder rather then create a new one.
Result:
Improved performance
Motivation:
When parsing HTTP headers special care needs to be taken when a whitespace is detected in the header name.
Modifications:
- Ignore whitespace when decoding response (just like before)
- Throw exception when whitespace is detected during parsing
- Add unit tests
Result:
Fixes https://github.com/netty/netty/issues/9571
Motivation:
Socks5InitialRequestDecoder does not correctly handle fragmentation
Modifications:
- Delete detection of not enough bytes as ReplyingDecoder already handles all of this correctly.
- Add unit test
Result:
Fixes#9574.
Motivation
Currently an epoll_ctl syscall is made every time there is a change to
the event interest flags (EPOLLIN, EPOLLOUT, etc) of a channel. These
are only done in the event loop so can be aggregated into 0 or 1 such
calls per channel prior to the next call to epoll_wait.
Modifications
I think further streamlining/simplification is possible but for now I've
tried to minimize structural changes and added the aggregation beneath
the existing flag manipulation logic.
A new AbstractChannel#activeFlags field records the flags last set on
the epoll fd for that channel. Calls to setFlag/clearFlag update the
flags field as before but instead of calling epoll_ctl immediately, just
set or clear a bit for the channel in a new bitset in the associated
EpollEventLoop to reflect whether there's any change to the last set
value.
Prior to calling epoll_wait the event loop makes the appropriate
epoll_ctl(EPOLL_CTL_MOD) call once for each channel who's bit is set.
Result
Fewer syscalls, particularly in some auto-read=false cases. Simplified
error handling from centralization of these calls.
Motivation:
peek() is implemented in a similar way to poll() for the mpsc queue, thus it is more like a consumer call.
It is possible that we could have multiple thread call peek() and possibly one thread calls poll() at at the same time.
This lead to multiple consumer scenario, which violates the multiple producer single consumer condition and could lead to spin in an infinite loop in peek()
Modification:
Use isEmpty() instead of peek() to check if task queue is empty
Result:
Dont violate the mpsc semantics.
