Motivation:
We have our own ThreadLocalRandom implementation to support older JDKs . That said we should prefer the JDK provided when running on JDK >= 7
Modification:
Using ThreadLocalRandom implementation of the JDK when possible.
Result:
Make use of JDK implementations when possible.
Motivation:
We missed some stuff in 5728e0eb2c and so the build failed on java9
Modifications:
- Add extra cmdline args when needed
- skip the autobahntestsuite as jython not works with java9
- skip the osgi testsuite as the maven plugin not works with java9
Result:
Build finally passed on java9
Motivation:
When the EPOLLRDHUP event is received we assume that the read side of the FD is no longer functional and force the input state to be shutdown. However if the channel is still active we should rely upon EPOLLIN and read to indicate there is no more data before we update the shutdown state. If we do not do this we may not read all pending data in the FD if the RecvByteBufAllocator doesn't want to consume it all in a single read operation.
Modifications:
- AbstractEpollChannel#epollRdHupReady() shouldn't force shutdown the input if the channel is active
Result:
All data can be read even if the RecvByteBufAllocator doesn't read it all in the current read loop.
Fixes https://github.com/netty/netty/issues/6303
Motivation:
We need to pass special arguments to run with jdk9 as otherwise some tests will not be able to run.
Modifications:
Allow to define extra arguments when running with jdk9
Result:
Tests pass with jdk9
Motivation:
EpollRecvByteAllocatorHandle intends to override the meaning of "maybe more data to read" which is a concept also used in all existing implementations of RecvByteBufAllocator$Handle but the interface doesn't support overriding. Because the interfaces lack the ability to propagate this computation EpollRecvByteAllocatorHandle attempts to implement a heuristic on top of the delegate which may lead to reading when we shouldn't or not reading data.
Modifications:
- Create a new interface ExtendedRecvByteBufAllocator and ExtendedHandle which allows the "maybe more data to read" between interfaces
- Deprecate RecvByteBufAllocator and change all existing implementations to extend ExtendedRecvByteBufAllocator
- transport-native-epoll should require ExtendedRecvByteBufAllocator so the "maybe more data to read" can be propagated to the ExtendedHandle
Result:
Fixes https://github.com/netty/netty/issues/6303.
Motivation:
We used various mocking frameworks. We should only use one...
Modifications:
Make usage of mocking framework consistent by only using Mockito.
Result:
Less dependencies and more consistent mocking usage.
Motivation:
Currently Netty does not wrap socket connect, bind, or accept
operations in doPrivileged blocks. Nor does it wrap cases where a dns
lookup might happen.
This prevents an application utilizing the SecurityManager from
isolating SocketPermissions to Netty.
Modifications:
I have introduced a class (SocketUtils) that wraps operations
requiring SocketPermissions in doPrivileged blocks.
Result:
A user of Netty can grant SocketPermissions explicitly to the Netty
jar, without granting it to the rest of their application.
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:
8ba5b5f740 removed some ciphers from the default list, and SocketSslEchoTest had one of these ciphers hard coded in the test. The test will fail if the cihper is not supported by default.
Modifications:
SocketSslEchoTest should ensure a cipher is used which will be supported by the peer
Result:
Test result no longer depends upon default cipher list.
Motivation:
the build doesnt seem to enforce this, so they piled up
Modifications:
removed unused import lines
Result:
less unused imports
Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
Motivation:
At the moment only DefaultFileRegion is supported when using the native epoll transport.
Modification:
- Add support for any FileRegion implementation
- Add test case
Result:
Also custom FileRegion implementation are supported when using the epoll transport.
Motivition:
NIO throws ClosedChannelException when a user tries to call shutdown*() on a closed Channel. We should do the same for the EPOLL transport
Modification:
Throw ClosedChannelException when a user tries to call shutdown*() on a closed channel.
Result:
Consistent behavior.
Motivation:
The NIO transport used an IllegalStateException if a user tried to issue another connect(...) while the connect was still in process. For this case the JDK specified a ConnectPendingException which we should use. The same issues exists in the EPOLL transport. Beside this the EPOLL transport also does not throw the right exceptions for ENETUNREACH and EISCONN errno codes.
Modifications:
- Replace IllegalStateException with ConnectPendingException in NIO and EPOLL transport
- throw correct exceptions for ENETUNREACH and EISCONN in EPOLL transport
- Add test case
Result:
More correct error handling for connect attempts when using NIO and EPOLL transport
Motivation:
We need to ensure we also call fireChannelActive() if the Channel is directly closed in a ChannelFutureListener that is belongs to the promise for the connect. Otherwise we will see missing active events.
Modifications:
Ensure we always call fireChannelActive() if the Channel was active.
Result:
No missing events.
Motivation:
We should throw a NotYetConnectedException when ENOTCONN errno is set. This is also consistent with NIO.
Modification:
Throw correct exception and add test case
Result:
More correct and consistent behavior.
Motivation:
For lack of a better way the SocketRstTest inspects the content of the exception message to check if a RST occurred. However on windows the exception message is different than on other Unix based platforms and the assertion statement fails.
Modifications:
- Hack another string check in the unit test
Result:
SocketRstTest passes on windows
Fixes https://github.com/netty/netty/issues/5335
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.
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 missed to correctly retrieve the localAddress() after we called Socket.connect(..) and so the user would always see an incorrect address when calling EpollSocketChannel.localAddress().
Modifications:
- Ensure we always retrieve the localAddress() after we called Socket.connect(...) as only after this we will be able to receive the correct address.
- Add unit test
Result:
Correct and consistent behaviour across different transports (NIO/OIO/EPOLL).
Motivation:
OIO/NIO use a volatile variable to track if a read is pending. EPOLL does not use a volatile an executes a Runnable on the event loop thread to set readPending to false. These mechansims should be consistent, and not using a volatile variable is preferable because the variable is written to frequently in the event loop thread.
OIO also does not set readPending to false before each fireChannelRead operation and may result in reading more data than the user desires.
Modifications:
- OIO/NIO should not use a volatile variable for readPending
- OIO should set readPending to false before each fireChannelRead
Result:
OIO/NIO/EPOLL are more consistent w.r.t. readPending and volatile variable operations are reduced
Fixes https://github.com/netty/netty/issues/5069
Motivation:
If SO_LINGER is set to 0 the EPOLL transport will send a FIN followed by a RST. This is not consistent with the behavior of the NIO transport. This variation in behavior can cause protocol violations in streaming protocols (e.g. HTTP) where a FIN may be interpreted as a valid end to a data stream, but RST may be treated as the data is corrupted and should be discarded.
https://github.com/netty/netty/issues/4170 Claims the behavior of NIO always issues a shutdown when close occurs. I could not find any evidence of this in Netty's NIO transport nor in the JDK's SocketChannel.close() implementation.
Modifications:
- AbstractEpollChannel should be consistent with the NIO transport and not force a shutdown on every close
- FileDescriptor to keep state in a consistent manner with the JDK and not allow a shutdown after a close
- Unit tests for NIO and EPOLL to ensure consistent behavior
Result:
EPOLL is capable of sending just a RST to terminate a connection.
Motivation:
netty-tcnative-1.1.33.Fork was released, we should upgrade. Also we should skip renegotiate tests if boringssl is used because boringssl does not support renegotiation.
Modifications:
- Upgrade to netty-tcnative-1.1.33.Fork13
- Skip renegotiate tests if boringssl is used.
Result:
Use newest version of netty-tcnative and be able to build if boringssl is used.
Motivation:
As we now can easily build static linked versions of tcnative it makes sense to run our netty build against all of them.
This helps to ensure our code works with libressl, openssl and boringssl.
Modifications:
Allow to specify -Dtcnative.artifactId= and -Dtcnative.version=
Result:
Easy to run netty build against different tcnative flavors.
Motivation:
Warnings in IDE, unclean code, negligible performance impact.
Modification:
Deletion of unused imports
Result:
No more warnings in IDE, cleaner code, negligible performance improvement.
Motivation:
Javadoc reports errors about invalid docs.
Modifications:
Fix some errors reported by javadoc.
Result:
A lot of javadoc errors are fixed by this patch.
Motivation:
RC4 is not supported by default in more recent java versions as RC4 is considered insecure. We should not use it in tests as these test will fail on more recent java version.
Modifications:
Use SSL_RSA_WITH_3DES_EDE_CBC_SHA for test.
Result:
Non failing test on more recent java versions.
Motivation:
The latest netty-tcnative fixes a bug in determining the version of the runtime openssl lib. It also publishes an artificact with the classifier linux-<arch>-fedora for fedora-based systems.
Modifications:
Modified the build files to use the "-fedora" classifier when appropriate for tcnative. Care is taken, however, to not change the classifier for the native epoll transport.
Result:
Netty is updated the the new shiny netty-tcnative.
Motivation:
As stated in the SSLSession javadocs getPeer* methods need to throw a SSLPeerUnverifiedException if peers identity has not be verified.
Modifications:
- Correctly throw SSLPeerUnverifiedException
- Add test for it.
Result:
Correctly behave like descripted in javadocs.
Motivation:
Invoking the javax.net.ssl.SSLEngine.closeInbound() method will send a
fatal alert and invalidate the SSL session if a close_notify alert has
not been received.
From the javadoc:
If the application initiated the closing process by calling
closeOutbound(), under some circumstances it is not required that the
initiator wait for the peer's corresponding close message. (See section
7.2.1 of the TLS specification (RFC 2246) for more information on
waiting for closure alerts.) In such cases, this method need not be
called.
Always invoking the closeInbound() method without regard to whether or
not the closeOutbound() method has been invoked could lead to
invalidating perfectly valid SSL sessions.
Modifications:
Added an instance variable to track whether the
SSLEngine.closeOutbound() method has been invoked. When the instance
variable is true, the SSLEngine.closeInbound() method doesn't need to be
invoked.
Result:
SSL sessions will not be invalidated if the outbound side has been
closed but a close_notify alert hasn't been received.
Motivation:
There currently exists http.HttpUtil, http2.HttpUtil, and http.HttpHeaderUtil. Having 2 HttpUtil methods can be confusing and the utilty methods in the http package could be consolidated.
Modifications:
- Rename http2.HttpUtil to http2.HttpConversionUtil
- Move http.HttpHeaderUtil methods into http.HttpUtil
Result:
Consolidated utilities whose names don't overlap.
Fixes https://github.com/netty/netty/issues/4120
Motivation:
Sometimes the user already has a PrivateKey / X509Certificate which should be used to create a new SslContext. At the moment we only allow to construct it via Files.
Modifications:
- Add new methods to the SslContextBuilder to allow creating a SslContext from PrivateKey / X509Certificate
- Mark all public constructors of *SslContext as @Deprecated, the user should use SslContextBuilder
- Update tests to us SslContextBuilder.
Result:
Creating of SslContext is possible with PrivateKay/X509Certificate
Motivation:
Remove RC4 from default ciphers as it is not known as secure anymore.
Modifications:
Remove RC4
Result:
Not use an insecure cipher as default.
Motivation:
DatagramUnitcastTest sometimes fails with BindException for an unknown reason.
Modifications:
Retry up to 3 times with a new free port when bind() fails with BindException
Result:
More build stability
Motivation:
SocketSslEchoTest.testSslEcho() has a race condition where a renegotiation future can be done before:
assertThat(renegoFuture.isDone(), is(false));
Modifications:
Remove the offending assertion.
Result:
More build stability
Motivation:
Due a bug we not correctly handled connection refused errors and so failed the connect promise with the wrong exception.
Beside this we some times even triggered fireChannelActive() which is not correct.
Modifications:
- Add testcase
- correctly detect connect errors
Result:
Correct and consistent handling.
Motivation:
To prevent from DOS attacks it can be useful to disable remote initiated renegotiation.
Modifications:
Add new flag to OpenSslContext that can be used to disable it
Adding a testcase
Result:
Remote initiated renegotion requests can be disabled now.
Motivation:
Because we tried to grab the SSL renegotation future to early we could see test-failures.
Modifications:
Access the future at the correct time.
Result:
No more test-failures.
Motivation:
The current heap dump compression preset (9) requires way too much
memory (768 MiB at maximum for dictionary), resulting in OOME in many
cases.
Modifications:
- Use the default preset (6) which uses 8 MiB dictionary.
- Do not fail abruptly even when OOME has been raised.
Result:
More stable heap dump acquisition
Motivation:
The SSL peer who did not initiate renegotiation sometimes does not get
the notification for renegotition due to an unknown reason.
Modification:
Until the exact cause is understood, relax the assertions of the flaky
tests.
Result:
Build stability
Motivation:
Some SCTP applications require the SCTP unordered flag.
This flag was not exposed by Netty so applications were unable
to use it.
Modifications:
- Add unordered flag to SctpMessage.
- {Nio,Oio}SctpChannel pass unordered flag to MessageInfo on write.
- SctpOutboundByteStreamHandler may optionally request unordered
delivery for all outbound messages.
- Added test case to SctpEchoTest using unordered flag.
Result:
Fixes#3698. New constructors and methods added to SctpMessage and
SctpOutboundByteStreamHandler, but changes are backward compatible.
Motiviation:
Our tests for non-auto-read did actually not test this correctly as auto-read was never disabled on the Bootstrap and ServerBootstrap.
Modifications:
- Correctly disable auto-read on Bootstrap and ServerBootstrap
- Fix tests to call ChannelHandlerContext.read() once a Channel becomes active.
Result:
Correctly test that non-auto-read works.
Motivation:
To use WebSocketClientHandshaker / WebSocketServerHandshaker it's currently a requirement of having a HttpObjectAggregator in the ChannelPipeline. This is not a big deal when a user only wants to server WebSockets but is a limitation if the server serves WebSockets and normal HTTP traffic.
Modifications:
Allow to use WebSocketClientHandshaker and WebSocketServerHandshaker without HttpObjectAggregator in the ChannelPipeline.
Result:
More flexibility
Motivation:
Using Unix Domain Sockets can be very useful when communication should take place on the same host and has less overhead then using loopback. We should support this with the native epoll transport.
Modifications:
- Add support for Unix Domain Sockets.
- Adjust testsuite to be able to reuse tests.
Result:
Unix Domain Sockets are now support when using native epoll transport.
Motivation:
Several issues were shown by various ticket (#2900#2956).
Also use the improvement on writability user management from #3036.
And finally add a mixte handler, both for Global and Channels, with
the advantages of being uniquely created and using less memory and
less shaping.
Issue #2900
When a huge amount of data are written, the current behavior of the
TrafficShaping handler is to limit the delay to 15s, whatever the delay
the previous write has. This is wrong, and when a huge amount of writes
are done in a short time, the traffic is not correctly shapened.
Moreover, there is a high risk of OOM if one is not using in his/her own
handler for instance ChannelFuture.addListener() to handle the write
bufferisation in the TrafficShapingHandler.
This fix use the "user-defined writability flags" from #3036 to
allow the TrafficShapingHandlers to "user-defined" managed writability
directly, as for reading, thus using the default isWritable() and
channelWritabilityChanged().
This allows for instance HttpChunkedInput to be fully compatible.
The "bandwidth" compute on write is only on "acquired" write orders, not
on "real" write orders, which is wrong from statistic point of view.
Issue #2956
When using GlobalTrafficShaping, every write (and read) are
synchronized, thus leading to a drop of performance.
ChannelTrafficShaping is not touched by this issue since synchronized is
then correct (handler is per channel, so the synchronized).
Modifications:
The current write delay computation takes into account the previous
write delay and time to check is the 15s delay (maxTime) is really
exceeded or not (using last scheduled write time). The algorithm is
simplified and in the same time more accurate.
This proposal uses the #3036 improvement on user-defined writability
flags.
When the real write occurs, the statistics are update accordingly on a
new attribute (getRealWriteThroughput()).
To limit the synchronisations, all synchronized on
GlobalTrafficShapingHandler on submitWrite were removed. They are
replaced with a lock per channel (since synchronization is still needed
to prevent unordered write per channel), as in the sendAllValid method
for the very same reason.
Also all synchronized on TrafficCounter on read/writeTimeToWait() are
removed as they are unnecessary since already locked before by the
caller.
Still the creation and remove operations on lock per channel (PerChannel
object) are synchronized to prevent concurrency issue on this critical
part, but then limited.
Additionnal changes:
1) Use System.nanoTime() instead of System.currentTimeMillis() and
minimize calls
2) Remove / 10 ° 10 since no more sleep usage
3) Use nanoTime instead of currentTime such that time spend is computed,
not real time clock. Therefore the "now" relative time (nanoTime based)
is passed on all sub methods.
4) Take care of removal of the handler to force write all pending writes
and release read too
8) Review Javadoc to explicit:
- recommandations to take into account isWritable
- recommandations to provide reasonable message size according to
traffic shaping limit
- explicit "best effort" traffic shaping behavior when changing
configuration dynamically
Add a MixteGlobalChannelTrafficShapingHandler which allows to use only one
handler for mixing Global and Channel TSH. I enables to save more memory and
tries to optimize the traffic among various channels.
Result:
The traffic shaping is more stable, even with a huge number of writes in
short time by taking into consideration last scheduled write time.
The current implementation of TrafficShapingHandler using user-defined
writability flags and default isWritable() and
fireChannelWritabilityChanged works as expected.
The statistics are more valuable (asked write vs real write).
The Global TrafficShapingHandler should now have less "global"
synchronization, hoping to the minimum, but still per Channel as needed.
The GlobalChannel TrafficShapingHandler allows to have only one handler for all channels while still offering per channel in addition to global traffic shaping.
And finally maintain backward compatibility.
Motivation:
We only support openssl for server side at the moment but it would be also useful for client side.
Modification:
* Upgrade to new netty-tcnative snapshot to support client side openssl support
* Add OpenSslClientContext which can be used to create SslEngine for client side usage
* Factor out common logic between OpenSslClientContext and OpenSslServerContent into new abstract base class called OpenSslContext
* Correctly detect handshake failures as soon as possible
* Guard against segfault caused by multiple calls to destroyPools(). This can happen if OpenSslContext throws an exception in the constructor and the finalize() method is called later during GC
Result:
openssl can be used for client and servers now.
Motivation:
TrafficShapingHandlerTest uses Logback API directly, which is
discouraged. Also, it overrides the global default log level, which
silences the DEBUG messages from other tests.
Modifications:
Remove the direct use of Logback API
Result:
The tests executed after TrafficShapingHandlerTest logs their DEBUG
messages correctly.
Motivation:
We need more information to understand why SocketSslEchoTest fails
sporadically in the CI machine.
Modifications:
- Refactor SocketSslEchoTest so that it is easier to retrieve the
information about renegotiation and the current progress
Result:
We will get more information when the test fails.
Motivation:
Tests sometimes time out because it took too long to compress the
generated heap dump.
Modifications:
- Move the compression logic to a new method 'compressHeapDumps()'
- Call TestUtils.compressHeapDumps() at the end of the tests, so that
the tests do not fail because of timeout
Result:
JUnit reports the real cause of the test failure instead of timeout
exception.
Motivation:
So far, we generated and deployed test JARs to Maven repositories. The
deployed JAR had the classifier 'test-jar'. The test JAR is consumed by
transport-native-epoll as a test dependency.
The problem is, when netty-transport-native-epoll pulls the test JAR as
a dependency, that Maven resolves its transitive dependencies at
'compile' and 'runtime' scope only, which is incorrect.
I was bitten by this problem recently while trying to add a new
dependency to netty-testsuite. Because I added a new dependency at the
'test' scope, the new dependency was not pulled transitively by
transport-native-epoll and caused an unexpected build failure.
- d6160208c3
- bf77bb4c3a
Modifications:
- Move all classes in netty-testsuite from src/test to src/main
- Update the 'compile' scope dependencies of netty-testsuite
- Override the test directory configuration properties of the surefire
plugin
- Do not generate the test JAR anymore
- Update the dependency of netty-transport-native-epoll
Result:
It is less error-prone to add a new dependency to netty-testsuite.
Motivation:
It takes too long to download the heap dump from the CI server.
Modifications:
Compress the heap dump as much as possible.
Result:
When heap dump is generated by certain test failure, the generated heap
dump file is about 3 times smaller than before, although the compression
time will increase the build time when the test fails.
