Motivation:
To keep our code clean we should fail the build when unused c code is detected.
Modifications:
- Add '-Wunused-variable' to build flags
Result:
Cleaner code.
request with a 'content-encoding: chunked' header
Motivation:
It is valid to send a response to a HEAD request that contains a transfer-encoding: chunked header, but it is not valid to include a body, and there is no way to do this using the netty4 HttpServerCodec.
The root cause is that the netty4 HttpObjectEncoder will transition to the state ST_CONTENT_CHUNK and the only way to transition back to ST_INIT is through the encodeChunkedContent method which will write the terminating length (0\r\n\r\n\r\n), a protocol error when responding to a HEAD request
Modifications:
- Keep track of the method of the request and depending on it handle the response differently when encoding it.
- Added a unit test.
Result:
Correclty handle HEAD responses that are chunked.
Motivation:
We should assert that the leak aware buffers correctly close the ResourceLeakTracker in the unit tests.
Modifications:
- Keep track of NoopResourceLeakTrackers and check if these were closed once the test completes
- Fix bugs in tests so the buffers are all released.
Result:
Better tests for leak aware buffers
Motivation:
c2f4daa7398a9363fde8e51bf52c0d0323f870a5 added a unit test but used a too small test timeout.
Modifications:
Increase timeout.
Result:
Test should have enough time to complete on the CI.
Motivation:
Often its useful to run the tests with different commandline arguments (like different system properties).
Modifications:
Introduce argLine.javaProperties which can be set from the commandline as well to add arguments that should be append when run the unit tests.
Result:
More flexible way to run the tests.
Motivation:
InternalLoggerFactory either sets a default logger factory
implementation based on the logging implementations on the classpath, or
applications can set a logger factory explicitly. If applications wait
too long to set the logger factory, Netty will have already set a logger
factory leading to some objects using one logging implementation and
other objets using another logging implementation. This can happen too
if the application tries to set the logger factory twice, which is
likely a bug in the application. Yet, the Javadocs for
InternalLoggerFactory warn against this saying that
InternalLoggerFactory#setLoggerFactory "should be called as early as
possible and shouldn't be called more than once". Instead, Netty should
guard against this.
Modications:
We replace the logger factory field with an atomic reference on which we
can do CAS operations to safely guard against it being set twice. We
also add an internal holder class that captures the static interface of
InternalLoggerFactory that can aid in testing.
Result:
The logging factory can not be set twice, and applications that want to
set the logging factory must do it before any Netty classes are
initialized (or the default logger factory will be set).
Motivation:
If caches are disabled it does not make sense to schedule a task that will free up memory consumed by the caches.
Modifications:
Do not schedule if caches are disabled.
Result:
Less overhead.
Motivation:
We need to ensure we handle the case when BUFFER_OVERFLOW happens during unwrap but the readable bytes are bigger then the expected applicationBufferSize. Otherwise we may produce an IllegalArgumentException as we will try to allocate a buffer with capacity < 0.
Modifications:
- Guard against this case.
- Ensure we not double release buffer on exception when doing unwrap.
Result:
No more exception when running testsuite with java 9.
Motivation:
boringssl uses different messages for the ssl alerts which are all uppercase. As we try to match case as well this fails in SSLErrorTest as we expect lower-case.
This test was introduced by 9b7fb2f3620449c8cfafa62fd294cb82600f043a.
Modifications:
Ensure we first translate everything to lower-case before doing the assert.
Result:
SSLErrorTest also pass when boringssl is used.
Motivation:
When attempting to flamegraph netty w/ epoll it was noticed the stacks are lost going from
java to epoll lib.
Modifications:
added the -fno-omit-framepointer flag to compiler flags to ensure the fp is kept intact
Result:
Flamegraphs will now show native code in the same stack as java code using perf-java-flames
Motivation:
e102a008b63d0063581ba242539f0a8d473cae00 changed a conditional where previously the NIO ServerChannel would not be closed in the event of an exception.
Modifications:
- Restore the logic prior to e102a008b63d0063581ba242539f0a8d473cae00 which does not automatically close ServerChannels for IOExceptions
Result:
NIO ServerChannel doesn't close automatically for an IOException.
Motivation:
We need to ensure the tracked object can not be GC'ed before ResourceLeak.close() is called as otherwise we may get false-positives reported by the ResourceLeakDetector. This can happen as the JIT / GC may be able to figure out that we do not need the tracked object anymore and so already enqueue it for collection before we actually get a chance to close the enclosing ResourceLeak.
Modifications:
- Add ResourceLeakTracker and deprecate the old ResourceLeak
- Fix some javadocs to correctly release buffers.
- Add a unit test for ResourceLeakDetector that shows that ResourceLeakTracker has not the problems.
Result:
No more false-positives reported by ResourceLeakDetector when ResourceLeakDetector.track(...) is used.
Motivation:
We should not catch ConcurrentModificationException as this can never happen because things are executed on the EventLoop thread.
Modifications:
Remove try / catch
Result:
Cleaner code.
Motivation:
We need to ensure we not call handshake() when the engine is already closed. Beside this our implementation of isOutboundDone() was not correct as it not took the pending data in the outbound buffer into acount (which may be also generated as part of an ssl alert). Beside this we also called SSL_shutdown(...) while we were still in init state which will produce an error and so noise in the log with openssl later versions.
This is also in some extend related to #5931 .
Modifications:
- Ensure we not call handshake() when already closed
- Correctly implement isOutboundDone()
- Not call SSL_shutdown(...) when still in init state
- Added test-cases
Result:
More correct behaviour of our openssl SSLEngine implementation.
Motivation:
When non SSL data is passed into SSLEngine.unwrap(...) we need to throw an SSLException. This was not done at the moment. Even worse we threw an IllegalArgumentException as we tried to allocate a direct buffer with capacity of -1.
Modifications:
- Guard against non SSL data and added an unit test.
- Make code more consistent
Result:
Correct behaving SSLEngine implementation.
Motivation:
Java9 will be released soon so we should ensure we can compile netty with Java9 and run all our tests. This will help to make sure Netty will be usable with Java9.
Modification:
- Add some workarounds to be able to compile with Java9, note that the full profile is not supported with Java9 atm.
- Remove some usage of internal APIs to be able to compile on java9
- Not support Alpn / Npn and so not run the tests when using Java9 for now. We will do a follow up PR to add support.
Result:
Its possible to build netty and run its testsuite with Java9.
Motivation:
PooledByteBufAllocatorTest uses an ArrayQueue but access it from multiple threads (not concurrently but still from different threads). This may leak to memory visibility issues.
Modifications:
- Use a concurrent queue
- Some cleanup
Result:
Non racy test code.
Motivation:
If a user allocates a lot from outside the EventLoop we may end up creating a lot of caches in the PooledByteBufAllocator. This may be wasteful and so it may be useful for an other to configure that caches should only be used from within EventLoops.
Modifications:
Add new constructor which allows to configure the caching behaviour.
Result:
More flexible configuration of PooledByteBufAllocator possible
Motivation:
42fba015ce82ab4ab30e547c888db82fe74094e9 changed the implemention of ResourceLeakDetector to improve performance. While this was done a branch was missed that can be removed. Beside this using a Boolean as value for the ConcurrentMap is sub-optimal as when calling remove(key, value) an uncessary instanceof check and cast is needed on each removal.
Modifications:
- Remove branch which is not needed anymore
- Replace usage of Boolean as value type of the ConcurrentMap and use our own special type which only compute hash-code one time and use a == operation for equals(...) to reduce overhead present when using Boolean.
Result:
Faster and cleaner ResourceLeakDetector.
Motivation:
We missed to set active = true in EpollServerDomainSocketChannel.doBind(...) which also means that channelActive(...) was never triggered.
Modifications:
Correct set active = true in doBind(...)
Result:
EpollServerDomainSocketChannel is correctly set to active when bound.
Motivation:
We support using Netty without sun.misc.Unsafe, so we should also support building it without it. This way we can also run all tests without sun.misc.Unsafe and so see if it works as expected.
Modifications:
Correctly skip tests that depend on sun.misc.Unsafe if its not present or -Dio.netty.noUnsafe=true is used.
Result:
Be able to build netty without sun.misc.Unsafe
Motivation:
SwappedByteBuf.unwrap() not returned the wrapped buffer but the buffer that was wrapped by the original buffer. This is not correct.
Modifications:
Correctly return wrapped buffer and fix test.
Result:
SwappedByteBuf.unwrap() works as expected.
Motivation:
It's important that we do not pass in the original ChannelPromise to safeClose(...) as when flush(...) will throw an Exception it will be propagated to the AbstractChannelHandlerContext which will try to fail the promise because of this. This will then fail as it was already completed by safeClose(...).
Modifications:
Create a new ChannelPromise and pass it to safeClose(...).
Result:
No more confusing logs because of failing to fail the promise.
Motivation:
We had a few tests PooledByteBufAllocatorTests which used parkNanos(...) to give a resource enough time to get destroyed. This is race and may not be good enough.
Modifications:
Ensure the ThreadCache is really destroyed.
Result:
No more racy tests that depend on ThreadCaches.
Motivation:
We are now more careful to flush alerts that are generated when errors occur. We should also be more careful in unit tests to release any buffers that may be queued due to potential errors wich result in alerts.
Modifications:
- When SslHandlerTest uses EmbeddedChannel we should always call finishAndReleaseAll
Results:
Fixes https://github.com/netty/netty/issues/6057
Motivation:
Netty has a flag (io.netty.noUnsafe) for specifying to Netty to not be
unsafe. Yet, when initializing PlatformDependent0, Netty still tries to
be unsafe. For application that specify to Netty to not be unsafe and
run under a security manager, this can lead to an obnoxious (debug
level) stack trace. Since Netty was told not to be unsafe, Netty should
not try to be unsafe.
Modifications:
The initialization logic in PlatformDependent0 should take into account
that Netty was told not to be unsafe. This means that we need to
initialize PlatformDependent#IS_EXPLICIT_NO_UNSAFE as soon as possible,
before the static initializer for PlatformDependent0 has a chance to
run. Thus the following modifications are made:
- initialize PlatformDependent#IS_EXPLICIT_NO_UNSAFE before any other
code in PlatformDependent causes PlatformDependent0 to initialize
- expose the value of PlatformDependent#IS_EXPLICIT_NO_UNSAFE for
reading in PlatformDependent0
- take the value of PlatformDependent#IS_EXPLICIT_NO_UNSAFE into
account in PlatformDependent0
Result:
Netty does not try to be unsafe when told not to be unsafe.
Motivation:
For applications that set their own logger factory, they want that
logger factory to be the one logger factory. Yet, Netty eagerly
initializes this and then triggers initialization of other classes
before the application has had a chance to set its preferred logger
factory.
Modifications:
With this commit there are two key changes:
- Netty does not attempt to eagerly initialize the default logger
factory, only doing so if the application layer above Netty has not
already set a logger factory
- do not eagerly initialize unrelated classes from the logger factory;
while the motivation behind this was to initialize ThreadLocalRandom
as soon as possible in case it has to block reading from /dev/random,
this can be worked around for applications where it is problematic by
setting securerandom.source=file:/dev/urandom in their Java system
security policy (no, it is not less secure; do not even get me
started on myths about /dev/random)
Result:
Netty uses the logger factory that the application prefers, and does not
initialize unrelated classes.
Motivation:
When the SslHandler.unwrap(...) (which is called via decode(...)) method did produce an SSLException it was possible that the produced alert was not send to the remote peer. This could lead to staling connections if the remote peer did wait for such an alert and the connection was not closed.
Modifications:
- Ensure we try to flush any pending data when a SSLException is thrown during unwrapping.
- Fix SniHandlerTest to correct test this
- Add explicit new test in SslHandlerTest to verify behaviour with all SslProviders.
Result:
The alert is correctly send to the remote peer in all cases.
Motivation:
4bba7526e2f58018817972f38279cc232f519100 introduced changes which made pooled and unpooled derived buffers inconsistent in a few ways:
- Pooled derived buffers always generated a duplicate buffer when duplicate() was called and always generated a sliced buffer when slice() was called. Unpooled derived buffers some times generated a sliced buffer when duplicate() was called.
- The indexes that were set for duplicate buffers generated from slices were not always consistent.
There were also some various bugs in the derived pooled buffer implementation.
Modifications:
- Make pooled/unpooled consistently generate duplicate buffers when duplicate() is called and sliced buffers when slice() is called.
- Fix bugs in the derived pooled buffer
Result:
More consistent behavior from the derived pooled/unpooled buffers.
Modifications:
LocalChannel#releaseInboundBuffers should always clear/release the queue and set readInProgress to false
Result:
LocalChannel queue is more reliably cleaned up.
Motivation:
LocalChannel attempts to close its peer socket when ever it is closed. However if the channels are on different EventLoops we may attempt to process events for the peer channel on the wrong EventLoop.
Modifications:
- Ensure the close process ensures we are on the correct thread before accessing data
Result:
More correct LocalChannel close code.
Motivation:
We tried to detect the correct alert to use depending on the CertificateException that is thrown by the TrustManager. This not worked all the time as depending on the TrustManager implementation it may also wrap a CertPathValidatorException.
Modification:
- Try to unwrap the CertificateException if needed and detect the right alert via the CertPathValidatorException.
- Add unit to verify
Result:
Send the correct alert depending on the CertificateException when using OpenSslEngine.
Motivation:
PlatformDependent#getSystemClassLoader may throw a wide variety of exceptions based upon the environment. We should handle all exceptions and continue initializing the slow path if an exception occurs.
Modifications:
- Catch Throwable in cases where PlatformDependent#getSystemClassLoader is used
Result:
Fixes https://github.com/netty/netty/issues/6038
Motiviation:
We used ReferenceCountUtil.releaseLater(...) in our tests which simplifies a bit the releasing of ReferenceCounted objects. The problem with this is that while it simplifies stuff it increase memory usage a lot as memory may not be freed up in a timely manner.
Modifications:
- Deprecate releaseLater(...)
- Remove usage of releaseLater(...) in tests.
Result:
Less memory needed to build netty while running the tests.
Motivation:
00fc239995ce7b683993c88bb82b30dbd5fe7982 introduced a change to HashedWheelTimerTest which attempted to wait for an explicit event notification until more timer events can be added. However HashedWheelTimer will execute the timer Runnable before removing it from the queue and decrementing the total count. This make it difficult for users to know when it is safe to add another timer task as the limit is approached.
Modifications:
- HashedWheelTimer should remove the timer Runnable before executing the task.
Result:
Users can more reliably add new timers when the limit is reached and HashedWheelTimerTest will no longer fail spuriously due to this race condition.
Motivation:
In preparation for support of Conscrypt, I'm consolidating all of the engine-specific details so that it's easier to add new engine types that affect the behavior of SslHandler.
Modifications:
Added an enum SslEngineType that provides SSL engine-specific details.
Result:
SslHandler is more extensible for other engine types.
Motivation:
The SniHandlerTest.testServerNameParsing did fail when SslProvider.JDK was used as it the JDK SSLEngineImpl does not send an alert.
Modifications:
Ensure tests pass with JDK and OPENSSL ssl implementations.
Result:
SniHandlerTest will run with all SslProvider and not fail when SslProvider.JDK is used.
Motiviation:
We need to ensure we only consume as much da as we can maximal put in one ssl record to not produce a BUFFER_OVERFLOW when calling wrap(...).
Modification:
- Limit the amount of data that we consume based on the maximal plain text size that can be put in one ssl record
- Add testcase to verify the fix
- Tighten up testcases to ensure the amount of produced and consumed data in SslEngineResult matches the buffers. If not the tests will fail now.
Result:
Correct and conform behavior of OpenSslEngine.wrap(...) and better test coverage during handshaking in general.
Motivation:
Netty provides a adaptor from ByteBuf to Java's InputStream interface. The JDK Stream interfaces have an explicit lifetime because they implement the Closable interface. This lifetime may be differnt than the ByteBuf which is wrapped, and controlled by the interface which accepts the JDK Stream. However Netty's ByteBufInputStream currently does not take reference count ownership of the underlying ByteBuf. There may be no way for existing classes which only accept the InputStream interface to communicate when they are done with the stream, other than calling close(). This means that when the stream is closed it may be appropriate to release the underlying ByteBuf, as the ownership of the underlying ByteBuf resource may be transferred to the Java Stream.
Motivation:
- ByteBufInputStream.close() supports taking reference count ownership of the underyling ByteBuf
Result:
ByteBufInputStream can assume reference count ownership so the underlying ByteBuf can be cleaned up when the stream is closed.
Motivation:
HashWheelTimerTest has busy/wait and sleep statements which are not necessary. We also depend upon a com.google.common.base.Supplier which isn't necessary.
Modifications:
- Remove buys wait loops and timeouts where possible
Result:
HashWheelTimerTest more explicit in verifying conditions and less reliant on wait times.
Motivation:
If the rate at which new timeouts are created is very high and the created timeouts are not cancelled, then the JVM can crash because of out of heap space. There should be a guard in the implementation to prevent this.
Modifications:
The constructor of HashedWheelTimer now takes an optional max pending timeouts parameter beyond which it will reject new timeouts by throwing RejectedExecutionException.
Result:
After this change, if the max pending timeouts parameter is passed as constructor argument to HashedWheelTimer, then it keeps a track of pending timeouts that aren't yet expired or cancelled. When a new timeout is being created, it checks for current pending timeouts and if it's equal to or greater than provided max pending timeouts, then it throws RejectedExecutionException.
Motivation:
If the wsURL contains an encoded query, it will be decoded when generating the raw path. For example if the wsURL is http://test.org/path?a=1%3A5, the returned raw path would be /path?a=1:5
Modifications:
Use wsURL.getRawQuery() rather than wsURL.getQuery()
Result:
rawPath will now return /path?a=1%3A5
Motivation:
OpenSslEngine.wrap(...) and OpenSslEngie.unwrap(...) may consume bytes even if an BUFFER_OVERFLOW / BUFFER_UNDERFLOW is detected. This is not correct as it should only consume bytes if it can process them without storing data between unwrap(...) / wrap(...) calls. Beside this it also should only process one record at a time.
Modifications:
- Correctly detect BUFFER_OVERFLOW / BUFFER_UNDERFLOW and only consume bytes if non of them is detected.
- Only process one record per call.
Result:
OpenSslEngine behaves like stated in the javadocs of SSLEngine.
Motivation:
ObjectOutputStream uses a Channel Attribute to cache a ObjectOutputStream which is backed by a ByteBuf that may be released after an object is encoded and the underlying buffer is written to the channel. On subsequent encode operations the cached ObjectOutputStream will be invalid and lead to a reference count exception.
Modifications:
- CompatibleObjectEncoder should not cache a ObjectOutputStream.
Result:
CompatibleObjectEncoder doesn't use a cached object backed by a released ByteBuf.
Motivation:
We should not use the InternalThreadLocalMap where access may be done from outside the EventLoop as this may create a lot of memory usage while not be reused anyway.
Modifications:
Not use InternalThreadLocalMap in places where the code-path will likely be executed from outside the EventLoop.
Result:
Less memory bloat.
Motivation:
I had a need to know the user credentials of a connected unix domain socket.
Modifications:
Added a class to encapsulate user credentials (UID, GID, and the PID).
Augemented the Socket class to provide the JNI native interface to return this new class
Augemented the c code to call getSockOpts passing <a href=http://man7.org/linux/man-pages/man7/socket.7.html>SO_PEERCRED</a>
Then surfaced the ability to get user credentials in the EpollDomainSocketChannel
Result:
The EpollDomainSocketChannel now has a the following function signature:
public PeerCredentials peerCredentials() throws IOException allowing a caller to get the UID, GID, and PID of the linux process
connected to the unix domain socket.
Motivation:
To guard against the case that a user will enqueue a lot of empty or small buffers and so raise an OOME we need to also take the overhead of the ChannelOutboundBuffer / PendingWriteQueue into account when detect if a Channel is writable or not. This is related to #5856.
Modifications:
When calculate the memory for an message that is enqueued also add some extra bytes depending on the implementation.
Result:
Better guard against OOME.
