Motivation:
We used ERR_get_error() to detect errors and missed to handle different errors. Also we missed to clear the error queue for a thread before invoke SSL operations,
this could lead to detecting errors on different OpenSslEngines then the one in which the error actual happened.
Modifications:
Explicit handle errors via SSL.get_error and clear the error code before SSL operations.
Result:
Correctly handle errors and no false-positives in different OpenSslEngines then the one which detected an error.
Motivation:
Coalescing many small writes into a larger DATA frame reduces framing overheads on the wire and reduces the number of calls to Http2FrameListeners on the remote side.
Delaying the write of WINDOW_UPDATE until flush allows for more consumed bytes to be returned as the aggregate of consumed bytes is returned and not the amount consumed when the threshold was crossed.
Modifications:
- Remote flow controller no longer immediately writes bytes when a flow-controlled payload is enqueued. Sequential data payloads are now merged into a single CompositeByteBuf which are written when 'writePendingBytes' is called.
- Listener added to remote flow-controller which observes written bytes per stream.
- Local flow-controller no longer immediately writes WINDOW_UPDATE when the ratio threshold is crossed. Now an explicit call to 'writeWindowUpdates' triggers the WINDOW_UPDATE for all streams who's ratio is exceeded at that time. This results in
fewer window updates being sent and more bytes being returned.
- Http2ConnectionHandler.flush triggers 'writeWindowUpdates' on the local flow-controller followed by 'writePendingBytes' on the remote flow-controller so WINDOW_UPDATES preceed DATA frames on the wire.
Result:
- Better throughput for writing many small DATA chunks followed by a flush, saving 9-bytes per coalesced frame.
- Fewer WINDOW_UPDATES being written and more flow-control bytes returned to remote side more quickly, thereby improving throughput.
Motivation:
Bootstrap of the HTTP/2 can take a lot of paths and a lot of things can go wrong in the initial handshakes leading up to establishment of HTTP/2 between client and server. There have been many times where handshakes have failed silently, leading to very cryptic errors that are hard to debug.
Modifications:
Changed the HTTP/2 handler and decoder to ensure that the very first data on the wire (WRT HTTP/2) is SETTINGS/preface+SETTINGS. When this is not the case, a connection error is thrown with the bytes that were found instead.
Result:
Fixes#3880
Motivation:
Only one of the three FixedChannelPool constructors checks for the constructor
arguments. Therfore it was possible to create a pool with zero maxConnections.
This change chains all constructors together, so that the last one
in the chain always checks the validity of the arguments, regardless of the
constructor used.
Result:
It is no longer possible to create a FixedChannelPool instance with invalid
maxConnections or maxPendingAcquires parameters.
Motivation:
At the moment we use 1 * cores as default mimimum for pool arenas. This can easily lead to conditions as we use 2 * cores as default for EventLoop's when using NIO or EPOLL. If we choose a smaller number we will run into hotspots as allocation and deallocation needs to be synchronized on the PoolArena.
Modifications:
Change the default number of arenas to 2 * cores.
Result:
Less conditions when using the default settings.
Motivation:
According to the javadocs of SSLSession.getPeerPrincipal should be returning the identity of the peer, while we return the identity of the issuer.
Modifications:
Return the correct indentity.
Result:
Behavior match the documentation.
Motivation:
FixedChannelPool should enforce a number of maximal used channels, but due a bug we fail to correctly enforce this.
Modifications:
Change check to correctly only acquire channel if we not hit the limit yet.
Result:
Correct limiting.
Motivation:
To avoid buffering too much it would be useful to get an estimate of how many bytes can be written to a Channel before it becomes unwritable.
Modifications:
- Update the Channel interface to support 2 new methods. 1 to give how many bytes before unwritable. 1 to give how many bytes before writable.
- Update the AbstractChannel implementation to delegate to the ChannelOutboundBuffer.
Result:
The Channel interface supports 2 new methods which provide more visibility into writability.
Proposal to fix issue #3636
Motivations:
Currently, while adding the next buffers to the decoder
(`decoder.offer()`), there is no way to access to the current HTTP
object being decoded since it can only be available currently once fully
decoded by `decoder.hasNext()`.
Some could want to know the progression on the overall transfer but also
per HTTP object.
While overall progression could be done using (if available) the global
Content-Length of the request and taking into account each HttpContent
size, the per HttpData object progression is unknown.
Modifications:
1) For HTTP object, `AbstractHttpData` has 2 protected properties named
`definedSize` and `size`, respectively the supposely final size and the
current (decoded until now) size.
This provides a new method `definedSize()` to get the current value for
`definedSize`. The `size` attribute is reachable by the `length()`
method.
Note however there are 2 different ways that currently managed the
`definedSize`:
a) `Attribute`: it is reset each time the value is less than actual
(when a buffer is added, the value is increased) since the final length
is not known (no Content-Length)
b) `FileUpload`: it is set at startup from the lengh provided
So these differences could lead in wrong perception;
a) `Attribute`: definedSize = size always
b) `FileUpload`: definedSize >= size always
Therefore the comment tries to explain clearly the different behaviors.
2) In the InterfaceHttpPostRequestDecoder (and the derived classes), I
add a new method: `decoder.currentPartialHttpData()` which will return a
`InterfaceHttpData` (if any) as the current `Attribute` or `FileUpload`
(the 2 generic types), which will allow then the programmer to check
according to the real type (instance of) the 2 methods `definedSize()`
and `length()`.
This method check if currentFileUpload or currentAttribute are null and
returns the one (only one could be not null) that is not null.
Note that if this method returns null, it might mean 2 situations:
a) the last `HttpData` (whatever attribute or file upload) is already
finished and therefore accessible through `next()`
b) there is not yet any `HttpData` in decoding (body not yet parsed for
instance)
Result:
The developper has more access and therefore control on the current
upload.
The coding from developper side could looks like in the example in
HttpUloadServerHandler.
Motivation:
It's useful to be able to be notified once all Channels that are part of the ChannelGroup are notified. This can for example be useful if you want to do a graceful shutdown.
Modifications:
- Add ChannelGroup.newCloseFuture(...) which will be notified once all Channels are notified that are part of the ChannelGroup at the time of calling.
Result:
Easier to be notified once all Channels within a ChannelGroup are closed.
Motiviation:
There are currently no accessors which provide visbility into how many bytes must be written in order for a writability change to occur. This feature would be useful for codecs which intent to control how many bytes are queued at any given time.
Modifications:
- add bytesBeforeUnWritable() which will give the number of bytes before the buffer (and associated channel) transitions to not writable
- add bytesBeforeWritable() which will give the number of bytes that must be drained from the queue until the channel becomes writable.
Result:
More visibility into writability for the ChannelOutboundBuffer.
Motivation:
Due a copy and paste error we incorrectly skipped the first cert in the keyCertChainFile when using OpenSslClientContext.
Modifications:
Correctly not skip the first cert.
Result:
The certificate chain is correctly setup when using OpenSslClientContext.
Related: #3814
Motivation:
To implement the support for an upgrade from cleartext HTTP/1.1
connection to cleartext HTTP/2 (h2c) connection, a user usually uses
HttpServerUpgradeHandler.
It does its job, but it requires a user to instantiate the UpgradeCodecs
for all supported protocols upfront. It means redundancy for the
connections that are not upgraded.
Modifications:
- Change the constructor of HttpServerUpgradeHandler
- Accept UpgraceCodecFactory instead of UpgradeCodecs
- The default constructor of HttpServerUpgradeHandler sets the
maxContentLength to 0 now, which shouldn't be a problem because a
usual upgrade request is a GET.
- Update the examples accordingly
Result:
A user can instantiate Http2ServerUpgradeCodec and its related objects
(Http2Connection, Http2FrameReader/Writer, Http2FrameListener, etc) only
when necessary.
Motivation:
A user sometimes just want the aggregated message has no content at
all. (e.g. A user only wants HTTP GET requests.)
Modifications:
- Do not raise IllegalArgumentException even if a user specified
the maxContentLength of 0
Result:
A user can disallow a message with non-empty content.
Motivation:
Our HTTP/2 implementation sometimes uses hard-coded handler names when
adding/removing a handler to/from a pipeline. It's not really a good
idea because it can easily result in name clashes. Unless there is a
good reason, we need to use the reference to the handlers
Modifications:
- Allow null as a handler name for Http2Client/ServerUpgradeCodec
- Use null as the default upgrade handler name
- Do not use handler name strings in some test cases and examples
Result:
Fixes#3815
Related: #3871
Motivation:
StreamBufferingEncoderTest does not release when writeGoAway() is
called.
Modifications:
Release the buffer in mock object arguments
Result:
No buffer leak
Motivation:
Dumping the content of a ByteBuf in a hex format is very useful.
Modifications:
Move code into ByteBufUtil so its easy to reuse.
Result:
Easy to reuse dumping code.
Motiviation:
https://github.com/netty/netty/pull/3865 was merged from a machine with old code. A test case that was updates was not merged.
Modifications:
- Merge the missing test case updates
Result:
Test case no longer fails.
Motiviation:
The connection handler stream close operation is unconditionally adding a listener object to a future. We may not have to add a listener at all because the future has already been completed.
Modifications:
- If the future is done, directly invoke the logic without creating/adding a new listener.
Result:
No need to create/add listener if the future is already done in close logic.
Motivation:
We need to ensure we never allow to have null values set on headers, otherwise we will see a NPE during encoding them.
Modifications:
Add unit test that shows we correctly handle null values.
Result:
Verify correct implementation.
Motivation:
The current dumping code does not respect the readerIndex and so logs incorrect.
Modifications:
Respect readerIndex of ByteBuf
Result:
Correctly log content of buffer.
Motivation:
If the handlerAdded(...) callback was not called, the checkDeadLock() of the handshakeFuture will produce an IllegalStateException.
This was first reported at https://github.com/impossibl/pgjdbc-ng/issues/168 .
Modifications:
Pass deadlock check if ctx is null
Result:
No more race and so IllegalStateException.
Motivation:
Some glibc/kernel versions will trigger an EPOLLERR event to notify
about failed connect and not an EPOLLOUT. Also EPOLLERR may be triggered
when a connection is broke.
Modification:
React on EPOLLERR like if an EPOLLOUT / EPOLLIN was received, this will work in
all cases as we handle errors in EPOLLOUT / EPOLLIN anyway.
Result:
Correctly detect errors.
Motivation:
gRPC's BufferingHttp2ConnectionEncoder is a generic utility that simplifies client-side applications that want to allow stream creation without worrying about violating the SETTINGS_MAX_CONCURRENT_STREAMS limit. Since it's not gRPC-specific it makes sense to move it into Netty proper.
Modifications:
Adding the BufferingHttp2ConnectionEncoder and it's unit test.
Result:
Netty now supports buffering stream creation.
Motivation:
For advanced use-cases it an be helpful to be able to directly access the SSL_CTX and SSL pointers of the underlying openssl objects. This for example allows to register custom C callbacks.
Modifications:
- Expose the SSL_CTX and SSL pointers
- Cleanup the shutdown code
Result:
It's now possible to obtain the c pointes and set native callbacks.
Motivation:
SpdyOrHttpChooser and Http2OrHttpChooser duplicate fair amount code with each other.
Modification:
- Replace SpdyOrHttpChooser and Http2OrHttpChooser with ApplicationProtocolNegotiationHandler
- Add ApplicationProtocolNames to define the known application-level protocol names
Result:
- Less code duplication
- A user can perform dynamic pipeline configuration that follows ALPN/NPN for any protocols.
Related: #3641 and #3813
Motivation:
When setting up an HTTP/1 or HTTP/2 (or SPDY) pipeline, a user usually
ends up with adding arbitrary set of handlers.
Http2OrHttpChooser and SpdyOrHttpChooser have two abstract methods
(create*Handler()) that expect a user to return a single handler, and
also have add*Handlers() methods that add the handler returned by
create*Handler() to the pipeline as well as the pre-defined set of
handlers.
The problem is, some users (read: I) don't need all of them or the
user wants to add more than one handler. For example, take a look at
io.netty.example.http2.tiles.Http2OrHttpHandler, which works around
this issue by overriding addHttp2Handlers() and making
createHttp2RequestHandler() a no-op.
Modifications:
- Replace add*Handlers() and create*Handler() with configure*()
- Rename getProtocol() to selectProtocol() to make what it does clear
- Provide the default implementation of selectProtocol()
- Remove SelectedProtocol.UNKNOWN and use null instead, because
'UNKNOWN' is not a protocol
- Proper exception handling in the *OrHttpChooser so that the
exception is logged and the connection is closed when failed to
select a protocol
- Make SpdyClient example always use SSL. It was always using SSL
anyway.
- Implement SslHandshakeCompletionEvent.toString() for debuggability
- Remove an orphaned class: JettyNpnSslSession
- Add SslHandler.applicationProtocol() to get the name of the
application protocol
- SSLSession.getProtocol() now returns transport-layer protocol name
only, so that it conforms to its contract.
Result:
- *OrHttpChooser have better API.
- *OrHttpChooser handle protocol selection failure properly.
- SSLSession.getProtocol() now conforms to its contract.
- SpdyClient example works with SpdyServer example out of the box
Motivation:
The logic in the current websocket example is confusing and misleading
Modifications:
Remove occurrences of "http" and "https" and replace them with "ws" and "wss"
Result:
The example code is now coherent and is easier to understand for a new user.
Motivation:
the ByteBuffer[] that we keep in the ThreadLocal are never nulled out which can lead to have ByteBuffer instances sit there forever.
This is even a bigger problem if nioBuffer() of ByteBuffer returns a new ByteBuffer that can not be destroyed by ByteBuffer.release().
Modifications:
Null out ByteBuffer array after processing.
Result:
No more dangling references after done.
Related: #3797
Motivation:
There is a race condition where DnsNameResolver.query() can attempt to
increase the reference count of the DNS response which was released
already by other thread.
Modifications:
- Make DnsCacheEntry a top-level class for clear access control
- Use 'synchronized' to avoid the race condition
- Add DnsCacheEntry.retainedResponse() to make sure that the response
is never released while it is retained
- Make retainedResponse() return null when the response has been
released already, so that DnsNameResolver.query() knows that the
cached entry has been released
Result:
The forementioned race condition has been fixed.
Motivation:
The unit tests should not fail due to using a channel option which is not supported by the underlying kernel.
Modifications:
- Ignore RuntimeExceptions which are thrown by JNI code when setsockopt or getsockopt fails.
Result:
Unit tests pass if socket option is not supported by kernel.
Motiviation:
TCP_NOTSENT_LOWAT is only supported in linux kernel 3.12 or newer. The addition of this socket option prevents older kernels from building.
Modifications:
- Conditionally define TCP_NOTSENT_LOWAT if it is not defined
Result:
Kernels older than 3.12 can still compile the EPOLL module.
Motiviation:
Linux provides the TCP_NOTSENT_LOWAT socket option. This can be used to control how much unsent data is queued in the tcp kernel buffers. This can be important when application level protocols (SPDY, HTTP/2) have their own priority mechanism and don't want data queued in the kernel.
Modifications:
- The epoll module will have an additional socket option TCP_NOTSENT_LOWAT
- There will be JNI methods to control the underlying linux socket option mechanism
Result:
Linux EPOLL module exposes the TCP_NOTSENT_LOWAT socket option.
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:
SingleThreadEventLoopTest.testScheduleTaskAtFixedRate() fails often due to:
- too little tolerance
- incorrect assertion (it compares only with the previous timestamp)
Modifications:
- Increase the timestamp difference tolerance from 10ms to 20ms
- Improve the timestamp assertion so that the comparison is performed against the first recorded timestamp
- Misc: Fix broken Javadoc tag
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:
the JNI function ThrowNew won't release any allocated memory.
The method exceptionMessage is allocating a new string concatenating 2 constant strings
What is creating a small leak in case of these exceptions are happening.
Modifications:
Added new methods that will use exceptionMessage and free resources accordingly.
I am also removing the inline definition on these methods as they could be reused by
other added modules (e.g. libaio which should be coming soon)
Result:
No more leaks in case of failures.
Motivation:
When a faulty never-ending test keeps producing a lot of garbage doing
nothing but generating CPU load, our CI fails to detect the stalled
build, because it determines the 'inactivity time' from console
activity and GC keeps producing console output.
Modifications:
Remove the -verbose:gc flag from pom.xml
Result:
Stalled builds are terminated by our CI server.
Motivation:
PoolThreadCache did only cache allocations if the allocation and deallocation Thread were the same. This is not optimal as often people write from differen thread then the actual EventLoop thread.
Modification:
- Add MpscArrayQueue which was forked from jctools and lightly modified.
- Use MpscArrayQueue for caches and always add buffer back to the cache that belongs to the allocation thread.
Result:
ThreadPoolCache is now also usable and so gives performance improvements when allocation and deallocation thread are different.
Performance when using same thread for allocation and deallocation is noticable worse then before.
Motivation:
Calling System.nanoTime() for each channelRead(...) is very expensive. See [#3808] for more detailed description.
Also we always do extra work for each write and read even if read or write idle states should not be handled.
Modifications:
- Move System.nanoTime() call to channelReadComplete(...).
- Reuse ChannelFutureListener for writes
- Only add ChannelFutureListener to writes if write and all idle states should be handled.
- Only call System.nanoTime() for reads if idle state events for read and all states should be handled.
Result:
Less overhead when using the IdleStateHandler.
Motivation:
We called TrustManagerFactory.init(...) even when the trustCertChainFile is null. This could lead to exceptions during the handshake.
Modifications:
Correctly only call TurstManagerFactory.init() if trustCertcChainFail is not null.
Result:
Correct behavior.
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:
Currently we hold a lock on the PoolArena when we allocate / free PoolSubpages, which is wasteful as this also affects "normal" allocations. The same is true vice-verse.
Modifications:
Ensure we synchronize on the head of the PoolSubPages pool. This is done per size and so it is possible to concurrently allocate / deallocate PoolSubPages with different sizes, and also normal allocations.
Result:
Less condition and so faster allocation/deallocation.
Before this commit:
xxx:~/wrk $ ./wrk -H 'Connection: keep-alive' -d 120 -c 256 -t 16 -s scripts/pipeline-many.lua http://xxx:8080/plaintext
Running 2m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 17.61ms 29.52ms 689.73ms 97.27%
Req/Sec 278.93k 41.97k 351.04k 84.83%
530527460 requests in 2.00m, 71.64GB read
Requests/sec: 4422226.13
Transfer/sec: 611.52MB
After this commit:
xxx:~/wrk $ ./wrk -H 'Connection: keep-alive' -d 120 -c 256 -t 16 -s scripts/pipeline-many.lua http://xxx:8080/plaintext
Running 2m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 15.85ms 24.50ms 681.61ms 97.42%
Req/Sec 287.14k 38.39k 360.33k 85.88%
547902773 requests in 2.00m, 73.99GB read
Requests/sec: 4567066.11
Transfer/sec: 631.55MB
This is reproducable every time.
