Motivation:
EpollDomainSocketGatheringWriteTest. testGatheringWriteBig takes on average about 20-25 seconds on the CI servers, but there is a 30 second timeout being applied which leads to what maybe false positive test failures.
Modifications:
- Increase the test timeout to 120 seconds globally and 60 seconds to wait for all writes per test
Result:
Higher timeout for potentially less false positive test failures.
Motivation:
When using the OIO transport we need to act on byte[] when writing and reading from / to the underyling Socket. So we should ensure we use heap buffers by default to reduce memory copies.
Modifications:
Ensure we prefer heap buffers by default for the OIO transport.
Result:
Possible less memory copies.
Motivation:
SocketGatherWriteTest has been observed to fail and it has numerous issues which when resolved may help reduce the test failures.
Modifications:
- A volatile counter and a spin/sleep loop is used to trigger test termination. Incrementing a volatile is generally bad practice and can be avoided in this situation. This mechanism can be replaced by a promise. This mechanism should also trigger upon exception or channel inactive.
- The TestHandler maintains an internal buffer, but it is not released. We now only create a buffer on the server side and release it after comparing the expected results.
- The composite buffer creation logic can be simplified, also the existing composite buffer doesn't take into account the buffer's reader index when building buf2.
Result:
Cleaner test.
Motivation:
SocketStringEchoTest has been observed to fail and it has numerous issues which when resolved may help reduce the test failures.
Modifications:
- A volatile counter and a spin/sleep loop is used to trigger test termination. Incrementing a volatile is generally bad practice and can be avoided in this situation. This mechanism can be replaced by a promise. This mechanism should also trigger upon exception or channel inactive.
- Asserts are done in the Netty threads. Although these should result in a exceptionCaught the test may not observe these failures because it is spinning waiting for the count to reach the desired value.
Result:
Cleaner test.
Motivation:
We need to ensure we always null out (or set) the address on the java.net.DatagramPacket when doing read or write operation as the same instance is used across different calls.
Modifications:
Null out the address if needed.
Result:
Ensure the correct remote address is used when connect / disconnect between calls and also mix these with calls that directly specify the remote address for adatagram packets.
Motivation:
We need to support SO_TIMEOUT for the OioDatagramChannel but we miss this atm as we not have special handling for it in the DatagramChannelConfig impl that we use. Because of this the following log lines showed up when running the testsuite:
20:31:26.299 [main] WARN io.netty.bootstrap.Bootstrap - Unknown channel option 'SO_TIMEOUT' for channel '[id: 0x7cb9183c]'
Modifications:
- Add OioDatagramChannelConfig and impl
- Correctly set SO_TIMEOUT in testsuite
Result:
Support SO_TIMEOUT for OioDatagramChannel and so faster execution of datagram related tests in the testsuite
Motivation:
NativeLibraryLoader has some code-duplication that can be removed.
Modifications:
Remove duplicated code and just use provided methods of PlatformDependent.
Result:
Less code duplication, fixes [#3756].
Motivation:
'insideString' and 'openBraces' need a proper handling when streaming
Json array over multiple writes and an element decoding was started but
not completed.
Related to #6969
Modifications:
If the idx is reset:
- 'insideString' has to be reset to 'false' in order to indicate that
array element will be decoded from the beginning
- 'openBraces' has to be reset to '1' to indicate that Json array
decoding is in progress.
Result:
Json array is properly decoded when in streaming mode
Motivation:
The javadocs of Http2RemoteFlowController.isWritable(...) are incorrect.
Modifications:
Update javadocs to reflect reality.
Result:
Correct javadocs.
Motivation:
The labels identifying the frame types in Http2FrameLogger are not always correct.
Modification:
- Correct the string labels to indicate the right frame type in Http2FrameLogger
Result:
Logs are more correct.
Motivation:
Issue #6695 states that there is an issue when writing empty content via HttpResponseEncoder.
Modifications:
Add two test-cases.
Result:
Verified that all works as expected.
Motivation:
We used asm 6.0_ALPHA when building on java9 as the latest stable release not works with java9. asm 6.0_BETA was just released so we should update.
Modifications:
Upgrade asm version
Result:
Not use ALPHA release anymore
Motivation:
Implementations of DuplexChannel delegate the shutdownOutput to the underlying transport, but do not take any action on the ChannelOutboundBuffer. In the event of a write failure due to the underlying transport failing and application may attempt to shutdown the output and allow the read side the transport to finish and detect the close. However this may result in an issue where writes are failed, this generates a writability change, we continue to write more data, and this may lead to another writability change, and this loop may continue. Shutting down the output should fail all pending writes and not allow any future writes to avoid this scenario.
Modifications:
- Implementations of DuplexChannel should null out the ChannelOutboundBuffer and fail all pending writes
Result:
More controlled sequencing for shutting down the output side of a channel.
Motivation:
In Http2ConnectionHandler we call flush(...) in channelReadComplete(...) to ensure we update the flow-controller and write stuff to the remote peer. We should better flip the order and so may be able to pick up more bytes.
Modifications:
Change order of calls.
Result:
Better performance
Motivation:
Its wasteful and also confusing that channelReadComplete() is called even if there was no message forwarded to the next handler.
Modifications:
- Only call ctx.fireChannelReadComplete() if at least one message was decoded
- Add unit test
Result:
Less confusing behavior. Fixes [#4312].
Motivation:
Now that the NativeLibraryLoader implicitly detects the shaded package prefix we no longer need the io.netty.packagePrefix system property.
Modifications:
- Remove io.netty.packagePrefix processing from NativeLibraryLoader
Result:
Code is cleaner.
Motivation:
We need to ensure we not try to load any conscrypt classes directly (which means without using reflection) in the same class that is used to check if conscrypt is available. This is needed as otherwise we will have the following problem when try to use netty on java7:
java.lang.UnsupportedClassVersionError: org/conscrypt/BufferAllocator : Unsupported major.minor version 52.0
at io.netty.handler.ssl.ConscryptJdkSslEngineInteropTest.checkConscrypt(ConscryptJdkSslEngineInteropTest.java:49)
This regression was introduced by 4448b8f42f and detected on the CI when using:
mvn clean package -DtestJavaHome=$JAVA7_HOME
Modifications:
Move the detection code in an extra class and use it.
Result:
Works correctly also when using Java7.
Motivation:
We did not correctly handle connect() and disconnect() in EpollDatagramChannel / KQueueDatagramChannel and so the behavior was different compared to NioDatagramChannel.
Modifications:
- Correct implement connect and disconnect methods
- Share connect and related code
- Add tests
Result:
EpollDatagramChannel / KQueueDatagramChannel also supports correctly connect() and disconnect() methods.
Motivation:
Starting with 1.0.0.RC9, conscrypt supports a buffer allocator.
Modifications:
- Updated the creation process for the engine to pass through the
ByteBufAllocator.
- Wrap a ByteBufAllocator with an adapter for conscrypt.
- Added a property to optionally control whether conscrypt uses
Netty's buffer allocator.
Result:
Netty+conscrypt will support using Netty's ByteBufAllocator.
Motivation:
When we use pipeline.replace and we still had ongoing inbound, then
there will be some problem that inbound message would go to wrong
handlers. So we add handler first, and remove self after add, so that
the next handler will be the correct one.
Modifications:
Uses remove after addAfter instead of replace.
Result:
Fixed#6881
Motivation:
1. Hash function in the Snappy encoding is wrong probably: used '+' instead of '*'. See the reference implementation [1].
2. Size of the hash table is calculated, but not applied.
Modifications:
1. Fix hash function: replace addition by multiplication.
2. Allocate hash table with calculated size.
3. Use an `Integer.numberOfLeadingZeros` trick for calculate log2.
4. Release buffers in tests.
Result:
1. Better compression. In the test `encodeAndDecodeLongTextUsesCopy` now compressed size is 175 instead of 180 before this change.
2. No redundant allocations for hash table.
3. A bit faster the calc of shift (less an expensive math operations).
[1] 513df5fb5a/snappy.cc (L67)
Motivation:
Http2FrameCodec should use Http2Settings.defaultSettings() when no Http2Settings were specified by the user.
Modifications:
Replace new Http2Settings() with Http2Settings.defaultSettings()
Result:
Use correct Http2Settings by default when using Http2FrameCodec in all cases.
Motivation:
IP_TRANSPARENT support is not complete, the option can currently only be set on EpollServerSocket. Setting the option on an EpollSocket is also requires so as to be able to bind a socket to a non-local address as described in ip(7)
http://man7.org/linux/man-pages/man7/ip.7.html
"TProxy redirection with the iptables TPROXY target also
requires that this option be set on the redirected socket."
Modifications:
Added IP_TRANSPARENT socket option to EpollSocketChannelConfig
Result:
A redirecting socket can be created with a non-local IP address as required for TPROXY
Motivation:
In our http1 hello world example we only flush on channelReadComplete(...) to make better use of gathering writes. We should do the same in http2.
Modifications:
Only flush in channelReadComplete(...)
Result:
Better performance and more consistent examples.
Motivation:
When a user called ctx.close() and used the EmbeddedChannel we did not correctly run all pending tasks which means channelInactive was never called.
Modifications:
Ensure we run all pending tasks after all operations that may change the Channel state and are part of the Channel.Unsafe impl.
Result:
Fixes [#6894].
Motivation:
We need to ensure we only create the ResourceLeak when the constructor not throws.
Modifications:
Ensure ResourceLeakDetector.track(...) is only called if the constructor of ReferenceCoundedOpenSslEngine not throws.
Result:
No more false-positves.
Motivation:
ErrorProne complains that the array override doesn't match the
vararg super call. See http://errorprone.info/bugpattern/Overrides
Additionally, almost every other Future uses the vararg form, so
it would be stylistically consistent to keep it that way.
Modifications:
Use vararg override.
Result:
Cleaner, less naggy code.
Motivation:
DefaultChannelPipeline.estimatorHandle needs to be volatile as its accessed from different threads.
Modifications:
Make DefaultChannelPipeline.estimatorHandle volatile and correctly init it via CAS
Result:
No more race.
Motivation:
Currentry logger create hex dump even if log write will not apply.
It's unecessary GC overhead.
Modifications:
Restore optimization from #3492
Result:
Fixes#7025
Motivation:
There should be a way to allow graceful shutdown to wait for all open streams to close without a timeout. Using gracefulShutdownTimeoutMillis with a large value is a bit of a hack, and has a gotcha that sufficiently large values will overflow the long, resulting in a ClosingChannelFutureListener that executes immediately.
Modification:
Allow to use gracefulShutdownTimeoutMillis(-1) to express waiting until all streams are closed.
Result:
We can now shutdown the connection without a forced timeout.
Motivation:
We previously used pollLast() to retrieve a Channel from the queue that backs SimpleChannelPool. This could lead to the problem that some Channels are very unfrequently used and so when these are used the connection was already be closed and so could not be reused.
Modifications:
Allow to configure if the last recent used Channel should be used or the "oldest".
Result:
More flexible usage of ChannelPools
Motivation:
We should not use ipv4 google dns servers if the app is configured to run ipv6.
Modifications:
Use either ipv4 or ipv6 dns servers depending on the system config.
Result:
More correct behaviour
Motivation:
We used an int[] to store all values that are returned in the struct for TCP_INFO which is not good enough as it uses usigned int values.
Modifications:
- Change int[] to long[] and correctly cast values.
Result:
No more truncated values.
Motivation:
When the hostname portion can not be extracted we should just skip the server as otherwise we will produce and exception when trying to create the InetSocketAddress.
This was happing when trying to run the test-suite on a system and using java7:
java.lang.IllegalArgumentException: hostname can't be null
at java.net.InetSocketAddress.checkHost(InetSocketAddress.java:149)
at java.net.InetSocketAddress.<init>(InetSocketAddress.java:216)
at io.netty.util.internal.SocketUtils$10.run(SocketUtils.java:171)
at io.netty.util.internal.SocketUtils$10.run(SocketUtils.java:168)
at java.security.AccessController.doPrivileged(Native Method)
at io.netty.util.internal.SocketUtils.socketAddress(SocketUtils.java:168)
at io.netty.resolver.dns.DefaultDnsServerAddressStreamProvider.<clinit>(DefaultDnsServerAddressStreamProvider.java:74)
at io.netty.resolver.dns.DnsServerAddressesTest.testDefaultAddresses(DnsServerAddressesTest.java:39)
Modifications:
Skip if hostname can not be extracted.
Result:
No more java.lang.ExceptionInInitializerError.
Motivation:
JNDI allows to specify an port so we should respect it.
Modifications:
Use the specified port and if none is specifed use 53.
Result:
Correct handling of JNDI configured DNS.
Motivation:
`Epoll.ensureAvailability()` is called multiple times, once in
static initialization and in a couple of the constructors. This is
redundant and confusing to read.
Modifications:
Move `Epoll.ensureAvailability()` call into an instance initializer
and remove all other references. This ensures that every EELG
checks availability, while still delaying the check until
construction. This pattern is used when there are multiple ctors,
as in this class.
Result:
Easier to read code.
Motivation:
ByteBuf#ensureWritable(int,boolean) returns an int indicating the status of the resize operation. For buffers that are unmodifiable or cannot be resized this method shouldn't throw but just return 1.
ByteBuf#ensureWriteable(int) should throw unmodifiable buffers.
Modifications:
- ReadOnlyByteBuf should be updated as described above.
- Add a unit test to SslHandler which verifies the read only buffer can be tolerated in the aggregation algorithm.
Result:
Fixes https://github.com/netty/netty/issues/7002.
Motivation:
Lots of usages of SelfSignedCertificates were not deleting the certs at
the end of the test. This includes setupHandlers() which is used by
extending classes. Although these files will be deleted at JVM exit and
deleting them early does not free the JVM from trying to delete them at
shutdown, it's good practice to delete eagerly and since users sometimes
use tests as a form of documentation, it'd be good for them to see the
explicit deletes.
Modifications:
Add missing delete() calls to ½ of the SelfSignedCertificates-using
tests.
Result:
Tests that more clearly communicates which resources are created and
may accumulate without early delete.
Motivation:
Previously filterCipherSuites was being passed the OpenSSL-formatted
cipher names. Commit 43ae974 introduced a regression as it swapped to the
RFC/JDK format, except that user-provided ciphers were not converted and
remained in the OpenSSL format.
This mis-match would cause all user-provided to be thrown away, leading
to failure trying to set zero ciphers:
Exception in thread "main" javax.net.ssl.SSLException: failed to set cipher suite: []
at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:299)
at io.netty.handler.ssl.OpenSslContext.<init>(OpenSslContext.java:43)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:347)
at io.netty.handler.ssl.OpenSslServerContext.<init>(OpenSslServerContext.java:335)
at io.netty.handler.ssl.SslContext.newServerContextInternal(SslContext.java:421)
at io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:441)
Caused by: java.lang.Exception: Unable to configure permitted SSL ciphers (error:100000b1:SSL routines:OPENSSL_internal:NO_CIPHER_MATCH)
at io.netty.internal.tcnative.SSLContext.setCipherSuite(Native Method)
at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:295)
... 7 more
Modifications:
Remove the reformatting of user-provided ciphers, as they are already in
the RFC/JDK format.
Result:
No regression, and the internals stay sane using the RFC/JDK format.
Motivation:
08748344d8 introduced two new tests which did not take into account that the multipart delimiter can be between 2 and 16 bytes long.
Modifications:
Take the multipart delimiter length into account.
Result:
Fixes [#7001]
Motivation:
We need to ensure we not allow calling set/writeCharsequence on an released ByteBuf.
Modifications:
Add test-cases
Result:
Proves fix of [#6951].
Motivation:
We need to fail the promise if a failure during handshake happens.
Modification:
Correctly fail the promise.
Result:
Correct websocket client example. Fixes [#6998]
