Motivation:
We missed to check if the dst is ready only before using unsafe to copy data into it which lead to data-corruption. We need to ensure we respect ready only ByteBuffer.
Modifications:
- Correctly check if the dst is ready only before copy data into it in UnsafeByteBufUtil
- Also make it work for buffers that are not direct and not have an array
Result:
No more data corruption possible if the dst buffer is readonly and unsafe buffer implementation is used.
Motivation:
We currently not supported using KeyManagerFactory with OpenSslServerContext and so should throw an exception if the user tries to do so. This will at least not give suprising and hard to debug problems later.
Modifications:
Throw exception if a user tries to construct a OpenSslServerContext with a KeyManagerFactory
Result:
Fail fast if the user tries to use something that is not supported.
Related: #4572
Motivation:
- A user might want to extend Http2ConnectionHandler and define his/her
own static inner Builder class that extends
Http2ConnectionHandler.BuilderBase. This introduces potential
confusion because there's already Http2ConnectionHandler.Builder. Your
IDE will warn about this name duplication as well.
- BuilderBase exposes all setters with public modifier. A user's Builder
might not want to expose them to enforce it to certain configuration.
There's no way to hide them because it's public already and they are
final.
- BuilderBase.build(Http2ConnectionDecoder, Http2ConnectionEncoder)
ignores most properties exposed by BuilderBase, such as
validateHeaders, frameLogger and encoderEnforceMaxConcurrentStreams.
If any build() method ignores the properties exposed by the builder,
there's something wrong.
- A user's Builder that extends BuilderBase might want to require more
parameters in build(). There's no way to do that cleanly because
build() is public and final already.
Modifications:
- Make BuilderBase and Builder top-level so that there's no duplicate
name issue anymore.
- Add AbstractHttp2ConnectionHandlerBuilder
- Add Http2ConnectionHandlerBuilder
- Add HttpToHttp2ConnectionHandlerBuilder
- Make all builder methods in AbstractHttp2ConnectionHandlerBuilder
protected so that a subclass can choose which methods to expose
- Provide only a single build() method
- Add connection() and codec() so that a user can still specify
Http2Connection or Http2Connection(En|De)coder explicitly
- Implement proper state validation mechanism so that it is prevented
to invoke conflicting setters
Result:
Less confusing yet flexible builder API
Motivation:
We need to ensure we consume all pending data in the BIO on error to correctly send the close notify for the remote peer.
Modifications:
Correctly force the user to call wrap(...) if there is something left in the BIO.
Result:
close_notify is not lost.
Motivation:
When ClientAuth is set via SslContextBuilder we pass it into the OpenSslEngine constructor. Due a bug we missed to call the correct native methods and so never enabled ClientAuth in this case.
Modifications:
Correctly call setClientAuth(...) in the constructor if needed.
Result:
client auth also works when configured via the SslContextBuilder and OPENSSL is used.
Motivation:
The KObjectHashMapTest is in a directory called "io.netty.util.collection" rather than "io/netty/util/collection". This causes the generated tests to be created in the wrong directory as well.
Modifications:
Moved the file.
Result:
Fixes#4546
Motivation:
As discussed in #4529, NameResolver design shouldn't be resolving SocketAddresses (or String name + port) and return InetSocketAddresses. It should resolve String names and return InetAddresses.
This SocketAddress to InetSocketAddresses resolution is actually a different concern, used by Bootstrap.
Modifications:
Extract SocketAddress to InetSocketAddresses resolution concern to a new class hierarchy named AddressResolver.
These AddressResolvers delegate to NameResolvers.
Result:
Better separation of concerns.
Note that new AddressResolvers generate a bit more allocations because of the intermediate Promise and List<InetAddress>.
Motivation:
We need to remove all registered events for a Channel from the EventLoop before doing the actual close to ensure we not produce a cpu spin when the actual close operation is delayed or executed outside of the EventLoop.
Modifications:
Deregister for events for NIO and EPOLL socket implementations when SO_LINGER is used.
Result:
No more cpu spin.
Motivation:
Related to issue #4564.
AsciiString.contentEqualsIgnoreCase fails when comparing two AsciiStrings of the same length
Modifications:
Compare the values of the first AsciiString to the second AsciiString
Result:
AsciiString.contentEqualsIgnoreCase works as expected
Motivation:
Initialisation of the ByteBufUtil class, a class frequently used is
delayed because a significant number of String operations is performed to
fill a HEXDUMP_ROWPREFIXES array. This array also sticks to the Strings
forever.
It is quite likely that applications never use the hexdump facility.
Modification:
Moved the static initialisation and references to a static inner class.
This delays initialisation (and memory usage) until actually needed.
The API is kept as is.
Result:
Faster startup time, less memory usage for most netty using applications.
Motivation:
DefaultPromiseTest has dead code which was left over from a code restructure. Shared code between 2 tests was moved into a common method, but some code which was not cleaned up in each of these methods after the code was moved.
Modifications:
- Delete dead code in DefaultPromiseTest
Result:
Less dead code
Motivation:
AbstractFuture currently wraps CancellationException in a ExecutionException. However the interface of Future says that this exception should be directly thrown.
Modifications:
- Throw CancellationException from AbstractFuture.get
Result:
Interface contract for CancellationException is honored in AbstractFuture.
Motivation:
We should retain the original hostname when connect to a remote peer so the user can still query the origin hostname if getHostString() is used.
Modifications:
Compute a InetSocketAddress from the original remote address and the one returned by the Os.
Result:
Same behavior when using epoll transport and nio transport.
Motivation:
Some times the user wants to set a Http2HeaderEncoder.SensitivityDetector when building a Http2ConnectionHandler.
Modifications:
Allow to set Http2HeaderEncoder.SensitivityDetector via builder.
Result:
More flexible building of Http2ConnectionHandler possible.
Motivation:
If a uri contains whitespaces we need to ensure we correctly escape these when creating the request for the handshake.
Modifications:
- Correctly encode path for uri
- Add tests
Result:
Correctly handle whitespaces when doing websocket upgrade requests.
Motivation:
We already provide a NEVER_SENSITIVE instance,we should add ALWAYS_SENSITIVE as well.
Modifications:
Add ALWAYS_SENSITIVE instance which will always return true when check for sesitive.
Result:
User can reuse code.
Motivation:
We recently added methods to ByteBuf to directly write and read LE values. We should use these in the Snappy implementation and so reduce duplication.
Modifications:
Replace manually swapping of values with LE write and read methods of ByteBuf.
Result:
Cleaner code with less duplication.
Motivation:
We missed to remove a method in SslContext while refactored the implementation. We should remove the method to keep things clean.
Modifications:
Remove unused method.
Result:
Code cleanup.
Motivation:
There's no way to override the default settings of the DnsNameResolvers
created by DnsNameResolverGroup because DnsNameResolverGroup is final.
Modifications:
- Make DnsNameResolverGroup non-final
- Add a new overridable protected method 'newResolver()' so that a user
can override it to create an alternative DnsNameResolver instance or
set the non-default properties
Result:
A user can configure the DnsNameResolver.
Motivation:
Each server should be checked for every record type. Currently, if there
are only two configured servers and the first is down, it is impossible
to query for IPv4 addresses because the second server is only ever
queried for type AAAA.
Modifications:
Do not cycle DNS servers while cycling DNS record types (A and AAAA)
Result:
Name resolution is less fragile when the number of available DNS servers
is 2.
Motivation:
Use new / non-deprecated APIs for creating SSL Context
in tests, in order to be able to implement OpenSsl
tests with maximum code reuse.
Modifications:
Use `SslContextBuilder.(forServer|forClient)` instead
of deprecated `JdkSslServerContext` constructor.
Use `ApplicationProtocolConfig` instead of Protocol
Negotiator.
Use custom exception type for skipping tests to avoid
swallowing exceptions arising from tests.
Result:
Exceptions from tests aren't swallowed.
Using new APIs allows reusing same test code for
OpenSsl tests.
Motivation:
HttpClientUpgradeHandler uses HttpHeaderNames.UPGRADE as the value of
the 'Connection' header, which is incorrect. It should use
HttpHeaderValues.UPGRADE instead (note Names vs Values.)
Also, HttpHeaderValues.UPGRADE should be 'upgrade' rather than
'Upgrade', as defined in:
- https://tools.ietf.org/html/rfc7230#section-6.7
Modifications:
- Use HttpHeaderValues.UPGRADE for a 'Connection' header
- Lowercase the value of HttpHeaderValues.UPGRADE
Result:
- Fixes#4508
- Correct behavior
Motivation:
On a successful protocol upgrade in HTTP, HttpClientUpgradeHandler calls
HttpClientCodec.upgradeFrom(), which removed both the HTTP encoder and
decoder from the pipeline immediately.
However, because the decoder is in the middle of the decode loop,
removing it from the pipeline immediately will cause the cumulation
buffer to be released prematurely.
This often leads to an IllegalReferenceCountException or missing first
response after the upgrade response.
Modifications:
- Remove the decoder *after* the decode loop is done
Result:
Fixes#4504
Motivation:
HttpClientUpgradeHandler currently throws an IllegalStateException when
the server sends a '101 Switching Protocols' response that has no
'Upgrade' header.
Some servers do not send the 'Upgrade' header on a successful protocol
upgrade and we could safely assume that the server accepted the
requested protocol upgrade in such a case, looking from the response
status code (101)
Modifications:
- Do not throw an IllegalStateException when the server responded 101
without a 'Upgrade' header
- Note that we still check the equality of the 'Upgrade' header when it
is present.
Result:
- Fixes#4523
- Better interoperability
Motivation:
Fix a race-condition when closing NioSocketChannel or EpollSocketChannel while try to detect if a close executor should be used and the underlying socket was already closed. This could lead to an exception that then leave the channel / in an invalid state and so could lead to side-effects like heavy CPU usage.
Modifications:
Catch possible socket exception while try to get the SO_LINGER options from the underlying socket.
Result:
No more race-condition when closing the channel is possible with bad side-effects.
Motivation:
- On the client, cookies should be sorted in decreasing order of path
length. From RFC 6265:
5.4.2. The user agent SHOULD sort the cookie-list in the following
order:
* Cookies with longer paths are listed before cookies with
shorter paths.
* Among cookies that have equal-length path fields, cookies with
earlier creation-times are listed before cookies with later
creation-times.
NOTE: Not all user agents sort the cookie-list in this order, but
this order reflects common practice when this document was
written, and, historically, there have been servers that
(erroneously) depended on this order.
Note that the RFC does not define the path length of cookies without a
path. We sort pathless cookies before cookies with the longest path,
since pathless cookies inherit the request path (and setting a path
that is longer than the request path is of limited use, since it cannot
be read from the context in which it is written).
- On the server, if there are multiple cookies of the same name, only one
of them should be encoded. RFC 6265 says:
Servers SHOULD NOT include more than one Set-Cookie header field in
the same response with the same cookie-name.
Note that the RFC does not define which cookie should be set in the case
of multiple cookies with the same name; we arbitrarily pick the last one.
Modifications:
- Changed the visibility of the 'strict' field to 'protected' in
CookieEncoder.
- Modified ClientCookieEncoder to sort cookies in decreasing order of path
length when in strict mode.
- Modified ServerCookieEncoder to return only the last cookie of a given
name when in strict mode.
- Added a fast path for both strict mode in both client and server code
for cases with only one cookie, in order avoid the overhead of sorting
and memory allocation.
- Added unit tests for the new cases.
Result:
- Cookie generation on client and server is now more conformant to RFC 6265.
As discussed in #3209, this PR adds Little Endian accessors
to ByteBuf and descendants.
Corresponding accessors were added to UnsafeByteBufUtil,
HeapByteBufferUtil to avoid calling `reverseBytes`.
Deprecate `order()`, `order(buf)` and `SwappedByteBuf`.
Motivation:
ChannelMetadata has a field minMaxMessagesPerRead which can be confusing. There are also some cases where static instances are used and the default value for channel type is not being applied.
Modifications:
- use a default value which is set unconditionally to simplify
- make sure static instances of MaxMessagesRecvByteBufAllocator are not used if the intention is that the default maxMessagesPerRead should be derived from the channel type.
Result:
Less confusing interfaces in ChannelMetadata and ChannelConfig. Default maxMessagesPerRead is correctly applied.
Motivation:
Because we flow control HEADERS frames, it's possible that an intermediate error can result in a RST_STREAM frame being sent for a frame that the other endpoint is not yet aware of. This is a violation of the spec and will either result in spammy logs at the other endpoint or broken connections.
Modifications:
Modified the HTTP/2 handler so that it only sends RST_STREAM if it has sent at least one HEADERS frame to the remote endpoint for the stream.
Result:
Fixes#4465
Motivation:
The CoalescingBufferQueueTest is somewhat relaxed with its releasing of test buffers, using safeRelease to generically deal with tests that may or may not release the buffers. SafeRelease generates logs, however, when the release fails.
Modifications:
Tightened up the individual test methods to verify that the buffers are released properly.
Result:
Fixes#4497
Motivation:
The javadocs for ChannelOption.AUTO_CLOSE say the default is false, but the default is currently true.
Modifications:
- Make javadocs consistent with code
Result:
Less confusing docs.
Motivation:
The encoder is currently responsible for chunking frames when writing in order to conform to max frame size. The frame writer would be a better place for this since it could perform a reuse the same promise aggregator for all the write and could also perform a single allocation for all of the frame headers.
Modifications:
Modified `DefaultHttp2FrameWriter` to perform the chunking and modified the contract in the `Http2FrameWriter` interface. Modified `DefaultHttp2ConnectionEncoder` to send give all allocated bytes to the writer.
Result:
Fixes#3966
Motivation:
The UniformStreamByteDistributor currently processes all zero-length frames, regardless of add order. This means that we would always send HEADERS for all streams, possibly taking away bandwidth for streams that actually have data.
Modifications:
Empty frames are now treated the same as any other frame except that the algorithm will pop off the any empty frames at the head of the queue.
Result:
Empty frames require no extra processing.
Motivation:
DefaultHeaders creates an array of size 16 for all headers. This may waste a good deal of memory if applications only have a small number of headers. This memory may be critical when the number of connections grows large.
Modifications:
- Make the size of the array for DefaultHeaders configurable
Result:
Applications can control the size of the DefaultHeaders array and save memory.
Motiviation:
According to jetty docs the alpn-api should use the provided scope.
Modificaitons:
- change scope to provided for alpn-api
- update for new jdk
Result:
Users of Netty don't run into alpn version conflicts.
Fixes https://github.com/netty/netty/issues/4480
Motivation:
exceptionCaught(...) will only handle inbound exceptions which means it makes not much sense to have it also on ChannelOutboundHandler. Because of this we should move it to ChannelInboundHandler.
Modifications:
Add @deprecated annotation to ChannelHandler.exceptionCaught(...).
Result:
Preapre to cleanup the API in later release.
Motivation:
6.7 is the latest stable release in RHEL/CentOS 6 line. Given that most
RHEL/CentOS users have upgraded to 6.7 via yum upgrade, we should bump
our requirement.
Modification:
s/6.6/6.7/g
Result:
'mvn release:*' must be run on RHEL/CentOS 6.7 instead of 6.6.
Motivation:
AbstractEpollStreamChannel has a queue which collects splice events. Splice is assumed not to be the most common use case of this class and thus the splice queue could be initialized in a lazy fashion to save memory. This becomes more significant when the number of connections grows.
Modifications:
- AbstractEpollStreamChannel.spliceQueue will be initialized in a lazy fashion
Result:
Less memory consumption for most use cases
Motivation:
2a2059d976 was backported from master, and included an overriden method which does not exist in 4.1.
Modifications:
- Remove the invoker method from NoPriorityByteDistributionBenchmark
Result:
No more build error
Motivation:
We should use OneTimeTask where possible to reduce object creation.
Modifications:
Replace Runnable with OneTimeTask
Result:
Less object creation
Motivation:
There is a notification ordering issue in DefaultPromise when the lateListener collection is in use. The ordering issue can be observed in situations where a late listener is added to a Future returned from a write operation. It is possible that this future will run after a read operation scheduled on the I/O thread, even if the late listener is added on the I/O thread. This can lead to unexpected ordering where a listener for a write operation which must complete in order for the read operation to happen is notified after the read operation is done.
Modifications:
- If the lateListener collection becomes empty, it should be treated as though it was null when checking if lateListeners can be notified immediatley (instead of executing a task on the executor)
Result:
Ordering is more natural and will not be perceived as being out of order relative to other tasks on the same executor.
Motivation:
We not need to store another reference to AbstractChannel as we can access it through DefaultChannelHandlerContext.
Modifications:
Remove reference.
Result:
Cleaner code.
Motivation:
If you start to have 1M+ concurrent connections memory footprint can be come a big issue. We should try to reduce it as much as possible in the core of netty.
Modifications:
- Remove HashMap that was used to store name to ctx mapping. This was only used for validation and access a handler by name. As a pipeline is not expected to be very long (like 100+ handlers) we can just walk the linked list structure to find the ctx with a given name.
Result:
Less memory footprint of the DefaultChannelPipeline.
Motivation:
If we have a lot of writes going on we currently need to lookup the IovArray for each Channel that does writes. This can have quite some perf overhead. We should not need to do this and just store a reference of the IovArray on the EpollEventLoop itself.
Modifications:
- Remove IoArrayThreadLocal
- Store the IoArray in the EventLoop itself
Result:
Less FastThreadLocal lookups
Motivation:
The current priority algorithm can yield poor per-stream goodput when either the number of streams is high or the connection window is small. When all priorities are the same (i.e. priority is disabled), we should be able to do better.
Modifications:
Added a new UniformStreamByteDistributor that ignores priority entirely and manages a queue of streams. Each stream is allocated a minimum of 1KiB on each iteration.
Result:
Improved goodput when priority is not used.
Motivation:
If ChannelOption.ALLOW_HALF_CLOSURE is true and the shutdown input operation fails we should not propagate this exception, and instead consider this socket's read as half closed.
Modifications:
- AbstractEpollChannel.shutdownInput should not propagate exceptions when attempting to shutdown the input, but instead should just close the socket
Result:
Users expecting a ChannelInputShutdownEvent will get this event even if the socket is already shutdown, and the shutdown operation fails.
Motivation:
HttpHeaders already has specific methods for such popular and simple headers like "Host", but if I need to convert POST raw body to string I need to parse complex ContentType header in my code.
Modifications:
Add getCharset and getCharsetAsString methods to parse charset from Content-Length header.
Result:
Easy to use utility method.
Motivation:
The `DefaultHttp2RemoteFlowController` does not correctly determine `hasFrame` when updating the stream state for the distributor. Adding a check to enforce `hasFrame` when `streamableBytes > 0` causes several test failures.
Modifications:
Modified `DefaultHttp2RemoteFlowController` to simplify the writing logic and to correct the bookkeeping for `hasFrame`.
Result:
The distributors are always called with valid arguments.
