Motivation:
The DNS resolver may use default configuration inherited from the environment. This means the ndots value may change and result in test failure if the tests don't explicitly set the assumed value.
Modifications:
- Explicitly set ndots in resolver-dns unit tests so we don't fail if the environment overrides the search domain and ndots
Result:
Unit tests are less dependent upon the enviroment they run in.
Fixes https://github.com/netty/netty/issues/6966.
Motivation:
PR https://github.com/netty/netty/pull/6803 corrected an error in the return status of the OpenSslEngine. We should now be able to restore the SslHandler optimization.
Modifications:
- This reverts commit 7f3b75a509.
Result:
SslHandler optimization is restored.
Motivation:
JCTools 2.0.2 provides an unbounded MPSC linked queue. Before we shaded JCTools we had our own unbounded MPSC linked queue and used it in various places but gave this up because there was no public equivalent available in JCTools at the time.
Modifications:
- Use JCTool's MPSC linked queue when no upper bound is specified
Result:
Fixes https://github.com/netty/netty/issues/5951
Motivation:
Each call to SSL_write may introduce about ~100 bytes of overhead. The OpenSslEngine (based upon OpenSSL) is not able to do gathering writes so this means each wrap operation will incur the ~100 byte overhead. This commit attempts to increase goodput by aggregating the plaintext in chunks of <a href="https://tools.ietf.org/html/rfc5246#section-6.2">2^14</a>. If many small chunks are written this can increase goodput, decrease the amount of calls to SSL_write, and decrease overall encryption operations.
Modifications:
- Introduce SslHandlerCoalescingBufferQueue in SslHandler which will aggregate up to 2^14 chunks of plaintext by default
- Introduce SslHandler#setWrapDataSize to control how much data should be aggregated for each write. Aggregation can be disabled by setting this value to <= 0.
Result:
Better goodput when using SslHandler and the OpenSslEngine.
Motivation:
The JDK SSLEngine documentation says that a call to wrap/unwrap "will attempt to consume one complete SSL/TLS network packet" [1]. This limitation can result in thrashing in the pipeline to decode and encode data that may be spread amongst multiple SSL/TLS network packets.
ReferenceCountedOpenSslEngine also does not correct account for the overhead introduced by each individual SSL_write call if there are multiple ByteBuffers passed to the wrap() method.
Modifications:
- OpenSslEngine and SslHandler supports a mode to not comply with the limitation to only deal with a single SSL/TLS network packet per call
- ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write
- SslHandler shouldn't cache maxPacketBufferSize as aggressively because this value may change before/after the handshake.
Result:
OpenSslEngine and SslHanadler can handle multiple SSL/TLS network packet per call.
[1] https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html
Motivation:
1. Some encoders used a `ByteBuf#writeBytes` to write short constant byte array (2-3 bytes). This can be replaced with more faster `ByteBuf#writeShort` or `ByteBuf#writeMedium` which do not access the memory.
2. Two chained calls of the `ByteBuf#setByte` with constants can be replaced with one `ByteBuf#setShort` to reduce index checks.
3. The signature of method `HttpHeadersEncoder#encoderHeader` has an unnecessary `throws`.
Modifications:
1. Use `ByteBuf#writeShort` or `ByteBuf#writeMedium` instead of `ByteBuf#writeBytes` for the constants.
2. Use `ByteBuf#setShort` instead of chained call of the `ByteBuf#setByte` with constants.
3. Remove an unnecessary `throws` from `HttpHeadersEncoder#encoderHeader`.
Result:
A bit faster writes constants into buffers.
Motivation:
We should also use realloc when shrink the buffer to eliminate extra allocations / memory copies when possible.
Modifications:
Use realloc for expanding and shrinking when possible.
Result:
Less memory copies and allocations
Motivation:
In some environments, the HTTP CONNECT handshake requires special headers to work.
Modification:
Update HttpProxyHandler to accept a HttpHeaders argument.
Result:
The header is passed along in the HTTP CONNECT request, and the proxy request can be successfully completed.
Motivation:
An intermediate list is creating in the `EpollEventLoop#closeAll` to prevent ConcurrentModificationException. But this is not the obvious purpose has no comment.
Modifications:
Add comment to clarify the appointment of the intermediate collection.
Result:
More clear code.
Motivation:
UnixResolverDnsServerAddressStreamProvider currently throws an exception if /etc/resolver exists but it empty. This shouldn't be an exception and can be tolerated as if there is no contribution from /etc/resolver.
Modifications:
- Treat /etc/resolver as present and empty the same as not being present
Result:
UnixResolverDnsServerAddressStreamProvider initialization can tolerate empty /etc/resolver directory.
Motivation:
InetSocketAddress#getHostName() may attempt a reverse lookup which may lead to test failures because the expected address will not match.
Modifications:
- Use InetSocketAddress#getHostString() which will not attempt any lookups and instead return the original String
Result:
UnixResolverDnsServerAddressStreamProviderTest is more reliable.
Motivation:
SslHandlerTest#testCompositeBufSizeEstimationGuaranteesSynchronousWrite has been observed to fail on CI servers. Knowing how many bytes were seen by the client would be helpful.
Modifications:
- Add bytesSeen to the exception if the client closes early.
Result:
More debug info available.
Motivation:
We used an intermediate collection to store the read DatagramPackets and only fired these through the pipeline once wewere done with the reading loop. This is not needed and can also increase memory usage.
Modifications:
Remove intermediate collection
Result:
Less overhead and possible less memory usage during read loop.
Motivation:
If there are multiple DNS servers to query Java's DNS resolver will attempt to resolve A and AAAA records in sequential order and will terminate with a failure once all DNS servers have been exhausted. Netty's DNS server will share the same DnsServerAddressStream for the different record types which may send the A question to the first host and the AAAA question to the second host. Netty's DNS resolution also may not progress to the next DNS server in all situations and doesn't have a means to know when resolution has completed.
Modifications:
- DnsServerAddressStream should support new methods to allow the same stream to be used to issue multiple queries (e.g. A and AAAA) against the same host.
- DnsServerAddressStream should support a method to determine when the stream will start to repeat, and therefore a failure can be returned.
- Introduce SequentialDnsServerAddressStreamProvider for sequential use cases
Result:
Fixes https://github.com/netty/netty/issues/6926.
Motivation:
`SocketChannelUDT` from barchart-udt does not have the java 7 `public abstract SocketChannel bind(SocketAddress local)` method. Calling the abstract method `SocketChannel.bind(SocketAddress localAddress)` for `SocketChannelUDT` leads to an `AbstractMethodError` runtime error.
Modifications:
Make workaround with explicit call of `SocketChannelUDT.bind(SocketAddress local)` as it done in `NioUdtByteConnectorChannel`.
Result:
Fixes [#6934].
Motivation:
The kqueue documentation states that 'Calling close() on a file descriptor will remove any kevents that reference the descriptor.' [1], but doesn't mention if this cleanup will be done synchronously. Under some circumstances it has been observed that cleanup was not done immediately and when KQueueEventLoop attempted to access the channel associated with the event the JVM would crash, a ClassCastException, or generally undefined behavior would occur because of invalid pointer references.
[1] https://www.freebsd.org/cgi/man.cgi?query=kqueue&sektion=2
Modifications:
- AbstractKqueueChannel#doClose should not rely upon this assumption and instead should call doDeregister() to ensure cleanup is done synchronously.
- Deleting a kevent should also set the jniSelfPtr stored in the udata of that kevent to NULL, to ensure we will not dereference it later.
Result:
No more kqueue crash due to close/cleanup sequencing.
Motivation:
In the SocksCmdRequest and SocksCmdResponse constructors a host param converts from IDN to ascii compatible form regardless address type.
Modifications:
Use `IDN#toASCII` only for `DOMAIN` address type.
Result:
More correct host handling in socks commands.
Motivation:
Methods `ByteBufUtil#writeUtf8` and `ByteBufUtil#writeAscii` contains a check `ByteBuf#ensureWritable` before the calling `ByteBuf#writeBytes`. But the `ByteBuf#writeBytes` also do a such check inside.
Modifications:
Make checks more targeted.
Result:
Less redundant method calls.
Motivation:
The behaviour of the FixedChannelPool.release was inconsistent with the
SimpleChannelPool implementation, in that given promise is returned.
In the FixedChannelPool implementation a new promise was return and
this meant that the completion of that promise can be different.
Specifically on releasing a channel to a closed pool, the parameter
promise is failed with an IllegalStateException but the returned one
will have been successful (as it was completed by call to super
.release)
Modification:
Return the given promise as the result of FixedChannelPool.release
Result:
Returned promise will reflect the result of the release operation.
Motivation:
Channels returned to a FixedChannelPool after closing it remain active.
Since channels that where acquired from the pool are not closed during the close operation, they remain open even after releasing the channel back to the pool where they are then in accessible and become in-effect a connection leak.
Modification:
Close the released channel on releasing back to a closed pool.
Result:
Much harder to create a connection leak by closing an active
FixedChannelPool instance.
Motivation:
DefaultHttp2ConnectionEncoder#writeHeaders attempts to find a stream object, and if one doesn't exist it tries to create one. However in the event that the local endpoint has received a RST_STREAM frame before writing the response headers we attempt to create a stream. Since this stream ID is for the incorrect endpoint we then generate a GO_AWAY for what appears to be a protocol error, but can instead be failed locally.
Modifications:
- Just fail the local promise in the above situation instead of sending a GO_AWAY
Result:
Less severe consequences if the server asynchronously sends headers after a RST_STREAM has been received.
Fixes https://github.com/netty/netty/issues/6906.
Motivation:
We rely upon the linker being non-lazy to test compatibility the native library compatibility for kqueue, but the default mode of operation is to lazy link.
Modifications:
- We should modify the build scripts to inform the linker that this library should not be lazy linked
- Error messages changes
dyld: lazy symbol binding failed: Symbol not found: _clock_gettime
java.lang.UnsatisfiedLinkError: unsupported JNI version 0xFFFFFFFF required by .../libnetty-transport-native-kqueue.dylib
Result:
Link errors are detected upon library load time.
Motivation:
We had a typo in the method name of the EpollSocketChannelConfig.
Modifications:
Deprecate old method and introduce a new one.
Result:
Fixes [#6909]
Motivation:
Right now HttpRequestEncoder does insertion of slash for url like http://localhost?pararm=1 before the question mark. It is done not effectively.
Modification:
Code:
new StringBuilder(len + 1)
.append(uri, 0, index)
.append(SLASH)
.append(uri, index, len)
.toString();
Replaced with:
new StringBuilder(uri)
.insert(index, SLASH)
.toString();
Result:
Faster HttpRequestEncoder. Additional small test. Attached benchmark in PR.
Benchmark Mode Cnt Score Error Units
HttpRequestEncoderInsertBenchmark.newEncoder thrpt 40 3704843.303 ± 98950.919 ops/s
HttpRequestEncoderInsertBenchmark.oldEncoder thrpt 40 3284236.960 ± 134433.217 ops/s
Motivation:
1. `ByteBuf` contains methods to writing `CharSequence` which optimized for UTF-8 and ASCII encodings. We can also apply optimization for ISO-8859-1.
2. In many places appropriate methods are not used.
Modifications:
1. Apply optimization for ISO-8859-1 encoding in the `ByteBuf#setCharSequence` realizations.
2. Apply appropriate methods for writing `CharSequences` into buffers.
Result:
Reduce overhead from string-to-bytes conversion.
Motivation:
ErrorProne warns about missing cases in switch statements that
appear as an oversight.
Modifcation:
Add the last case to statement to ensure all cases are covered.
Result:
Able to enable Error Prone static analysis
Motivation:
These headers can be used to prevent clickjacking.
Modifications:
Add static fields for content-security-policy and x-frame-options
Result:
Expose general useful names
fix not execute unit test in transport-native-unix-common-tests module
Motivation:
- Commit 047da11 introduced an bug for still copy byteBuf for composed of n(n <= IOV_MAX) NIO direct buffers
- Commit 3c4dfed add UnixChannelUtilTest in transport-native-unix-common-tests module, but not execute in maven compile
as issue #6825, #6870
Modifications:
- modified UnixChannelUtil#isBufferCopyNeededForWrite(ByteBuf), and UnixChannelUtilTest
- move UnixChannelUtilTest into transport-native-unix-common module, and add packet scope method UnixChannelUtil#isBufferCopyNeededForWrite(ByteBuf, int)
Result:
- no copy byteBuf for composed of n(n <= IOV_MAX) NIO direct buffers
- auto execute unit tests in UnixChannelUtilTest and it is easier to mock IOV_MAX
Motivation:
ba80fbbe05 modified the UnknownHostException to not include the search domain if the DNS query failed, but this masks what DNS query actually failed. Have the full hostname (including the search domain) provides more visibility and may help diagnose a configuration error if queries are unexpectedly failing.
Modifications:
- Remove DnsNameResolverContext#pristineHostname
Result:
UnknownHostException is more accurate and reflect what hostname actually resulted in failure.
Motivation:
A life cycle of QueryStringEncoder is simple: create, append params, convert to String. Current realization collect params in the list, and calculate an URI string in `toString` method. We can simplify this: don't store params to the list, and immediately append parameters to the `StringBuilder`.
Modifications:
- Remove list for params and remove a tuple class `Param`.
- Use one common `StringBuilder` and append parameters into it.
- Resolve `TODO` in the `encodeParam` method.
Result:
Less allocations (no `ArrayList`, no `Param` tuples). Second `toString` call is faster.
Motivation:
Enable static linking for Java 8. These commits are the same as those introduced to netty tcnative. The goal is to allow lots of JNI libraries to be statically linked together without having conflict `JNI_OnLoad` methods.
Modification:
* add JNI_OnLoad suffixes to enable static linking
* Add static names to the list of libraries that try to be loaded
* Enable compiling with JNI 1.8
* Sort includes
Result:
Enable statically linked JNI code.
Motivation:
1. Internal class `SocksCommonUtils` contains a method `intToIp` that also exists in the `NetUtil`.
2. A `SocksCommonUtils#ipv6toCompressedForm` is never used.
Modifications:
1. Replace `intToIp` method usage with `NetUtil#intToIpAddress`.
2. Remove unused methods from `SocksCommonUtils`.
Result:
Less code for supports.
Motivation:
PR #6811 introduced a public utility methods to decode hex dump and its parts, but they are not visible from netty-common.
Modifications:
1. Move the `decodeHexByte`, `decodeHexDump` and `decodeHexNibble` methods into `StringUtils`.
2. Apply these methods where applicable.
3. Remove similar methods from other locations (e.g. `HpackHex` test class).
Result:
Less code duplication.
Motivation:
DnsQueryLifecycleObserver is designed to capture the life cycle of every query. DnsNameResolverContext has a custom trace mechanism which consists of a StringBuilder and manual calls throughout the class. We can remove some special case code in DnsNameResolverContext and instead use a special implementation of DnsQueryLifecycleObserver when trace is enabled.
Modifications:
- Remove all references to the boolean trace variables in DnsNameResolverContext and DnsNameResolver
- Introduce TraceDnsQueryLifecycleObserver which will be used when trace is enabled and will log similar data as what trace currently provides
Result:
Less special case code in DnsNameResolverContext and instead delegate to TraceDnsQueryLifecycleObserver to capture trace information.
The code in question has this comment, but it is *after* the fall
so the static analysis flags it.
This is described in http://errorprone.info/bugpattern/FallThrough
Modifications:
Move fall through comment to where the fall actually occurs
Result:
More compatible with Error Prone tools
Motivation:
Google requires stricter compilation by adding -Werror and enabling many other warnings.
Modification:
* fix warning caused by -Wmissing-braces
* Use the address of `sendmmsg` rather than the function itself when
checking for presence. This resovles the warning caused by
`-Wpointer-bool-conversion`.
More detail:
When compiling on Linux, `sendmmsg` is always present, so the
function is always nonnull. When compiling elsewhere, the
function is defined as `__attribute__((weak))` which means it
may be absent at link time. This is controlled by
`IO_NETTY_SENDMMSG_NOT_FOUND`, which is off by default.
The reason for the error is due to the risk of accidentally not
calling the function. By adding `&` before the function, there
is no ambiguity. (the result of the fn call cannot have its
address taken.)
* use != to check for sendmmsg
Result:
Easier compilation.
Motivation:
If the content-length does not parse as a number, leniency causes this
to instead be parsed as the default value. This leads to bodies being
silently ignored on requests which can be incredibly dangerous. Instead,
if the content-length header is invalid, an exception should be thrown
for upstream handling.
Modifications:
This commit removes the leniency in parsing the content-length header by
allowing a number format exception, if thrown, to escape from the method
rather than falling back to the default value.
Result:
In invalid content-length header will not be silently ignored.
