Motivation:
The `#ensureAccessible` method in `UnpooledHeapByteBuf#capacity` used
to prevent NPE if buffer is released and `array` is `null`. In all
other implementations of `ByteBuf` the accessible is not checked by
`capacity` method. We can assign an empty array to `array`
in the `deallocate` and don't worry about NPE in the `#capacity`.
This will help reduce the number of repeated calls of the
`#ensureAccessible` in many operations with `UnpooledHeapByteBuf`.
Modifications:
1. Remove `#ensureAccessible` call from `UnpooledHeapByteBuf#capacity`.
Use the `EmptyArrays#EMPTY_BYTES` instead of `null` in `#deallocate`.
2. Fix access checks in `AbstractUnsafeSwappedByteBuf` and
`AbstractByteBuf#slice` that relied on `#ensureAccessible`
in `UnpooledHeapByteBuf#capacity`. This was found by unit tests.
Result:
Less double calls of `#ensureAccessible` for `UnpooledHeapByteBuf`.
Motivation:
Unnecessary flushes reduce the amount of flush coalescing that can happen at higher levels and thus can increase number of packets (because of TCP_NODELAY) and lower throughput (due to syscalls, TLS frames, etc)
Modifications:
Replace writeAndFlush(...) with write(...)
Result:
Fixes https://github.com/netty/netty/issues/7837.
Motivation:
SctpMessageCompletionHandler stores fragments in a Map but not release the stored ByteBuf when the handler is removed.
Modifications:
Release all buffers that are still in the Map when handlerRemoved(...) is called.
Result:
No more leaks for fragemented messages.
Motivation:
There may be meaningful 'connection' headers that exist on a request
that is used to attempt a HTTP/1.x upgrade request that will be
clobbered.
Modifications:
HttpClientUpgradeHandler uses the `HttpHeaders.add` instead of
`HttpHeaders.set` when adding the 'upgrade' field.
Result:
Fixes#7823, existing 'connection' headers are preserved.
* NetUtil valid IP methods to accept CharSequence
Motivation:
NetUtil has methods to determine if a String is a valid IP address. These methods don't rely upon String specific methods and can use CharSequence instead.
Modifications:
- Use CharSequence instead of String for the IP validator methods.
- Avoid object allocation in AsciiString#indexOf(char,int) and reduce
byte code
Result:
No more copy operation required if a CharSequence exists.
Motivation:
NetUtilBenchmark is using out of date data, throws an exception in the benchmark, and allocates a Set on each run.
Modifications:
- Update the benchmark and reduce each run's overhead
Result:
NetUtilBenchmark is updated.
Motivation:
ChunkedWriteHandler.doFlush is called twice from the same write if the channelWritabilityChanged event is invoked during the write. The buffer is already written so no extra data is sent on the socket but it causes the "promise already done" exception to be thrown.
This error happens only when the message is not ChunkedInput.
Modification:
Clear out the currentWrite reference before the ctx.write call, such that next time when the method is invoked the same object is not used twice.
Result:
Fixes#7819
* Add DnsNameResolver.resolveAll(DnsQuestion)
Motivation:
A user is currently expected to use DnsNameResolver.query() when he or
she wants to look up the full DNS records rather than just InetAddres.
However, query() only performs a single query. It does not handle
/etc/hosts file, redirection, CNAMEs or multiple name servers.
As a result, such a user has to duplicate all the logic in
DnsNameResolverContext.
Modifications:
- Refactor DnsNameResolverContext so that it can send queries for
arbitrary record types.
- Rename DnsNameResolverContext to DnsResolveContext
- Add DnsAddressResolveContext which extends DnsResolveContext for
A/AAAA lookup
- Add DnsRecordResolveContext which extends DnsResolveContext for
arbitrary lookup
- Add DnsNameResolverContext.resolveAll(DnsQuestion) and its variants
- Change DnsNameResolverContext.resolve() delegates the resolve request
to resolveAll() for simplicity
- Move the code that decodes A/AAAA record content to DnsAddressDecoder
Result:
- Fixes#7795
- A user does not have to duplicate DnsNameResolverContext in his or her
own code to implement the usual DNS resolver behavior.
Motivation:
Currently copying a direct ByteBuf copies it fully into the heap before writing it to an output stream.
The can result in huge memory usage on the heap.
Modification:
copy the bytebuf contents via an 8k buffer into the output stream
Result:
Fixes#7804
Motivation:
AbstractNioByteChannel will detect that the remote end of the socket has
been closed and propagate a user event through the pipeline. However if
the user has auto read on, or calls read again, we may propagate the
same user events again. If the underlying transport continuously
notifies us that there is read activity this will happen in a spin loop
which consumes unnecessary CPU.
Modifications:
- AbstractNioByteChannel's unsafe read() should check if the input side
of the socket has been shutdown before processing the event. This is
consistent with EPOLL and KQUEUE transports.
- add unit test with @normanmaurer's help, and make transports consistent with respect to user events
Result:
No more read spin loop in NIO when the channel is half closed.
Motivation:
There is a race between both flushing the upgrade response and receiving
more data before the flush ChannelPromise can fire and reshape the
pipeline. Since We have already committed to an upgrade by writing the
upgrade response, we need to be immediately prepared for handling the
next protocol.
Modifications:
The pipeline reshaping logic in HttpServerUpgradeHandler has been moved
out of the ChannelFutureListener attached to the write of the upgrade
response and happens immediately after the writeAndFlush call, but
before the method returns.
Result:
The pipeline is no longer subject to receiving more data before the
pipeline has been reformed.
Motivation:
Sometimes it is very convenient to remove the handler from pipeline without throwing the exception in case those handler doesn't exist in the pipeline.
Modification:
Added 3 overloaded methods to DefaultChannelPipeline, but not added to ChannelHandler due to back compatibility.
Result:
Fixes#7662
Motivation:
We should only schedule one timeout to wait for the close notify to be done.
Modifications:
Keep track of if we already scheduled a timeout for close notify and if so not schedule another one.
Result:
No duplicated timeouts.
Motivation:
HttpProxyHandler uses `NetUtil#toSocketAddressString` to compute
CONNECT url and Host header.
The url is correct when the address is unresolved, as
`NetUtil#toSocketAddressString` will then use
`getHoststring`/`getHostname`. If the address is already resolved, the
url will be based on the IP instead of the hostname.
There’s an additional minor issue with the Host header: default port
443 should be omitted.
Modifications:
* Introduce NetUtil#getHostname
* Introduce HttpUtil#formatHostnameForHttp to format an
InetSocketAddress to
HTTP format
* Change url computation to favor hostname instead of IP
* Introduce HttpProxyHandler ignoreDefaultPortsInConnectHostHeader
parameter to ignore 80 and 443 ports in Host header
Result:
HttpProxyHandler performs properly when connecting to a resolved address
Motivation:
Our code was not correct in AbstractNioMessageChannel.closeOnReadError(....) which lead to the situation that we always tried to continue reading no matter what exception was thrown when using the NioServerSocketChannel. Also even on an IOException we should check if the Channel itself is still active or not and if not stop reading.
Modifications:
Fix closeOnReadError impl and added test.
Result:
Correctly stop reading on NioServerSocketChannel when error happens during read.
Motivation:
If close(...) reports EINTR there is nothing sane we can do so it makes no sense to even report it. See also:
https://github.com/apple/swift-nio/pull/217
Modifications:
Just ignore EINTR when calling close(...)
Result:
Less noise in the logs.
Motivation:
LibreSSL removed support for NPN in its 2.6.1+ releases.
Modifications:
Skip NPN tests in libressl 2.6.1+
Result:
Be able to run netty tests against libressl 2.6.1+ as well.
Motivation:
6e5fd9311f fixed a bug in EmptyHeaders which was never noticed before because we had no tests.
Modifications:
Add tests for EmptyHeaders.
Result:
EmptyHeaders is tested now.
Motivation:
EmptyHeaders#get with a default value argument returns null. It should never return null, and instead it should return the default value.
Modifications:
- EmptyHeaders#get with a default value should return that default value
Result:
More correct implementation of the Headers API.
Motivation:
We had some code duplication in ChunkedWriteHandler.
Modifications:
Factor out duplicated code into private methods and reuse it.
Result:
Less code duplication.
Motivation:
We did not correctly set the stream id in the headers of HttpMessage when converting a Http2HeadersFrame. This is based on https://github.com/netty/netty/pull/7778 so thanks to @jprante.
Modifications:
- Correctly set the id when possible in the header.
- Add test case
Result:
Correctly include stream id.
Motivation:
DefaultChannelGroup.contains(...) did one more instanceof check then needed.
Modifications:
Simplify contains(...) and remove one instanceof check.
Result:
Simplier and cheaper implementation.
Motivation:
We missed to correctly record the stacktrace of the creation of an ResourceLeak record. This could either have the effect to log the wrote stacktrace for creation or not log a stacktrace at all if the object was dropped on the floor after it was created.
Modifications:
Correctly create a Record on creation of the object.
Result:
Fixes https://github.com/netty/netty/issues/7781.
Motivation:
Right now PendingWriteQueue.removeAndWriteAll collects all promises to
PromiseCombiner instance which sets listener to each given promise throwing
IllegalStateException on VoidChannelPromise which breaks while loop
and "reports" operation as failed (when in fact part of writes might be
actually written).
Modifications:
Check if the promise is not void before adding it to the PromiseCombiner
instance.
Result:
PendingWriteQueue.removeAndWriteAll succesfully writes all pendings
even in case void promise was used.
Motivation:
We currently not check if the buffer has a memory address and just assume this is the case if the nioBufferCount() == 1.
Modifications:
- Check hasMemoryAddress() before trying to access it.
- Add unit case.
Result:
More correct and robust code. Related to [#7752].
Motivation:
We should allow to access the memoryAddress / array of the FixedCompositeByteBuf when it only wraps a single ByteBuf. We do the same for CompositeByteBuf.
Modifications:
- Check how many buffers FixedCompositeByteBuf wraps and depending on it delegate the access to the memoryAddress / array
- Add unit tests.
Result:
Fixes [#7752].
Motivation:
We recently introduced ObjectCleaner which can be used to ensure some cleanup action is done once an object becomes weakable reachable. We should use this in Recycler.WeakOrderQueue to reduce the overhead of using a finalizer() (which will cause the GC to process it two times).
Modifications:
Replace finalizer() usage with ObjectCleaner
Result:
Fixes [#7343]
Motivation:
When using the JdkSslEngine, the ALPN class is used keep a reference
to the engine. In the event that the TCP connection fails, the
SSLEngine is not removed from the map, creating a memory leak.
Modification:
Always close the SSLEngine regardless of if the channel became
active. Also, record the SSLEngine was closed in all places.
Result:
Fixes: https://github.com/grpc/grpc-java/issues/3080
Motivation:
Android 5.0 sometimes not correctly update the bytesConsumed of the SSLEngineResult when consuming data from the input ByteBuffer. This will lead to handshake failures.
Modifications:
Add a workaround for Android 5.0
Result:
Be able to use netty on Android 5.0 by fixing https://github.com/netty/netty/issues/7758 .
Motivation:
We dont protect from overflow and so the timer may fire too early if a large timeout is used.
Modifications:
Add overflow guard and a test.
Result:
Fixes https://github.com/netty/netty/issues/7760.
Motivation:
The flush task is currently using flush() which will have the affect of have the flush traverse the whole ChannelPipeline and also flush messages that were written since we gave up flushing. This is not really correct as we should only continue to flush messages that were flushed at the point in time when the flush task was submitted for execution if the user not explicit call flush() by him/herself.
Modification:
Call *Unsafe.flush0() via the flush task which will only continue flushing messages that were marked as flushed before.
Result:
More correct behaviour when the flush task is used.
Motivation:
If someone invoke writeByte(), markWriterIndex(), readByte() in order first, and then invoke resetWriterIndex() should be throw a IndexOutOfBoundsException to obey the rule that the buffer declared "0 <= readerIndex <= writerIndex <= capacity".
Modification:
Changed the code writerIndex = markedWriterIndex; into writerIndex(markedWriterIndex); to make the check affect
Result:
Throw IndexOutOfBoundsException if any invalid happened in resetWriterIndex.
Motivation:
Read-only heap ByteBuffer doesn't expose array: the existent method to perform copies to direct ByteBuf involves the creation of a (maybe pooled) additional heap ByteBuf instance and copy
Modifications:
To avoid stressing the allocator with additional (and stealth) heap ByteBuf allocations is provided a method to perform copies using the (pooled) internal NIO buffer
Result:
Copies from read-only heap ByteBuffer to direct ByteBuf won't create any intermediate ByteBuf
Motivation:
We called the wrong super method in the test and also had a few unused imports.
Modifications:
Fix super method call and cleanup.
Result:
More correct test and cleanup.
Motivation:
When the response is very small, compression will inflate the response.
Modifications:
Add filed io.netty.handler.codec.http.HttpContentCompressor#compressThreshold that control whether the HTTP response should be compressed.
Result:
Fixes#7660.
Motivation:
KQueue implementations current have inconsistent behavior with Epoll implementations with respect to asynchronous sockets and connecting. In the Epoll transport we attempt to connect, if the connect call does not synchornously fail/succeed we set the EPOLLOUT which will be triggered by the kernel if the connection attempt succeeds or an error occurs. The connect API provides no way to asynchronously communicate an error so the Epoll implementation fires a EPOLLOUT event and puts the connect status in getsockopt(SO_ERROR). KQueue provides the same APIs but different behavior. If the EVFILT_WRITE is not enabled and the EVFILT_READ is enabled before connect is called, and there is an error the kernel may fire the EVFILT_READ filter and provide the Connection Refused error via read(). This is even true if we set the EVFILT_WRITE filter after calling connect because connect didn't synchornously complete. After the error has been delievered via read() a call to getsockopt(SO_ERROR) will return 0 indicating there is no error. This means we cannot rely upon the KQueue based kernel to deliver connection errors via the EVFILT_WRITE filter in the same way that the linux kernel does with the EPOLLOUT flag.
ce241bd introduced a change which depends upon the behavior of the EVFILT_WRITE being set and may prematurely stop writing to the OS as a result, becaues we assume the OS will notify us when the socket is writable. However the current work around for the above described behavior is to initialize the EVFILT_WRITE to true for connection oriented protocols. This leads to prematurely exiting from the flush() which may lead to deadlock.
Modifications:
- KQueue should check when an error is obtained from read() if the connectPromise has not yet been completed, and if not complete it with a ConnectException
Result:
No more deadlock in KQueue due to asynchronous connect workaround.
Motivation:
When we do DNS queries we need to ensure we always release the AddressEnvelope.
Modifications:
Also release the AddressEnvelope if the original resolution was done in the meantime and we did not cancel the extra query yet.
Result:
Should fix [#7713]
Motivation:
The Snappy decoder was failing on valid inputs containing literals
with 2-byte lengths > 0x8000 or copies with 2-byte offsets >= 0x8000.
The decoder was also enforcing an artificially low offset limit of
0x7FFF, something the Snappy format description advises against,
and which prevents decoding valid inputs generated by other encoders.
Modifications:
Interpret 2-byte literal lengths and 2-byte copy offsets as unsigned
shorts, in accordance with the format description and reference
implementation.
Allow any positive offset value. Throw an appropriate exception
for negative values (which can theoretically occur due to arithmetic
overflow on 4-byte offsets, but are unlikely to occur in the wild).
Result:
The Snappy decoder can handle valid inputs that previously caused
it to throw exceptions.
Motivation:
b215794de3 recently introduced a change in behavior where writeSpinCount provided a limit for how many write operations were attempted per flush operation. However when the write quantum was meet the selector write flag was not cleared, and the channel unsafe flush0 method has an optimization which prematurely exits if the write flag is set. This may lead to no write progress being made under the following scenario:
- flush is called, but the socket can't accept all data, we set the write flag
- the selector wakes us up because the socket is writable, we write data and use the writeSpinCount quantum
- we then schedule a flush() on the EventLoop to execute later, however it the flush0 optimization prematurely exits because the write flag is still set
In this scenario the socket is still writable so the EventLoop may never notify us that the socket is writable, and therefore we may never attempt to flush data to the OS.
Modifications:
- When the writeSpinCount quantum is exceeded we should clear the selector write flag
Result:
Fixes https://github.com/netty/netty/issues/7729
Motivation:
CharSequenceValueConverter#convertToBoolean has a few manual conditionals which can be removed if we use AsciiString.contentEqualsIgnoreCase. Also by comparing an AsciiString to a String we will incur conversions to char that can be avoided if we compare against AsciiString.
Modifications:
- Use AsciiString.contentEqualsIgnoreCase
- Compare against a AsciiString
Result:
Simplified CharSequenceValueConverter#convertToBoolean which favors AsciiString comparison.
Motivation:
It is not clear why Unsafe is unavailable when it is explicitly
disabled, or when Netty thinks it is running on Android.
Modification:
Change the "has" fields and methods to be causes. A null cause
means Unsafe is present. This catches all possible reason why
Unsafe might not be available.
Result:
Easier to debug Netty start up when logging cannot be turned on.
Motivation:
When SSL handshake fails, the connection should be closed. This is not true anymore after 978a46c.
Modifications:
- Ensure we always flush and close the channel on handshake failure.
- Add testcase.
Result:
Fixes [#7724].
