Motivation:
SonarQube (clinker.netty.io/sonar) reported a resource which may not have been properly closed in all situations in AbstractDiskHttpData.
Modifications:
- Ensure file channels are closed in the presence of exceptions.
- Correct instances where local channels were created but potentially not closed.
Result:
Less leaks. Less SonarQube vulnerabilities.
Motivation:
SonarQube (clinker.netty.io/sonar) reported a few 'critical' issues related to the OpenSslEngine.
Modifications:
- Remove potential for dereference of null variable.
- Remove duplicate null check and TODO cleanup.
Results:
Less potential for null dereference, cleaner code, and 1 less TODO.
Motivation:
`HttpResponseDecoder` and `HttpRequestDecoder` in the event when the max configured sizes for HTTP initial line, headers or content is breached, sends a `DefaultHttpResponse` and `DefaultHttpRequest` respectively. After this `HttpObjectDecoder` gets into `BAD_MESSAGE` state and ignores any other data received on this connection.
The combination of the above two behaviors, means that the decoded response/request are not complete (absence of sending `LastHTTPContent`). So, any code, waiting for a complete message will have to additionally check for decoder result to follow the correct semantics of HTTP.
If `HttpResponseDecoder` and `HttpRequestDecoder` creates a Full* invalid message then the request/response is a complete HTTP message and hence obeys the HTTP contract.
Modification:
Modified `HttpRequestDecoder`, `HttpResponseDecoder`, `RtspRequestDecoder` and `RtspResponseDecoder` to return Full* messages from `createInvalidMessage()`
Result:
Fixes the wrong behavior of sending incomplete messages from these codecs
In testEncodingSingleCookieV0():
Let's assume we encoded a cookie with MaxAge=50 when currentTimeMillis
is 10999.
Because the encoder will not encode the millisecond part for Expires,
the timeMillis value of the encoded Expires field will be 60000. (If we
did not dropped the millisecond part, it would be 60999.)
Encoding a cookie will take some time, so currentTimeMillis will
increase slightly, such as to 11001.
diff = (60000 - 11001) / 1000 = 48999 / 1000 = 48
maxAge - diff = 50 - 48 = 2
Due to losing millisecond part twice, we end up with the precision
problem illustrated above, and thus we should increase the tolerance
from 1 second to 2 seconds.
/cc @slandelle
Motivation:
Previously flow-controller had to know the implementation details of each frame type in order to write it correctly. That concern is more correctly handled by the encoder. By encapsulating the payload types to be flow-controlled it will be easier to add support for extension types later. This change also fixes#3353.
Modifications:
Add interface FlowControlled which is now delivered to flow-controller.
Implement this interface for HEADERS and DATA
Refactor and improve tests for flow-control.
Result:
Flow control semantics are more cleanly separated for data encoding and implementation is simpler overall.
Motivation:
SslHandler adds a pending write with an empty buffer and a VoidChannelPromise when a user flush and not pending writes are currently stored. This may produce an IllegalStateException later if the user try to add a ChannelFutureListener to the promise in the next ChannelOutboundHandler.
Modifications:
Replace ctx.voidPromise() with ctx.newPromise()
Result:
No more IllegalStateException possible
Motivation:
At the moment the max number of events that can be handled per epoll wakup was set during construction.
Modifications:
- Automatically increase the max number of events to handle
Result:
Better performance when a lot of events need to be handled without adjusting the code.
Motivation:
The current way how the guard against overflow when generating the nextId() is pretty slow once an overflow happened.
Modifications:
Once a possible overflow is detected all ids used by the EpollEventLoop are scrubed and re-assigned to the registered Channels. This way we only need to do extra work each time an overflow is detected.
Result:
More consistent performance even after the first overflow was detected.
Motivation:
SSLEngine specifies that IllegalArgumentException must be thrown if a null argument is given when using wrap(...) or unwrap(...).
Modifications:
Replace NullPointerException with IllegalArgumentException to match the javadocs.
Result:
Match the javadocs.
Motivation:
We failed to correctly calculate the endOffset when wrap multiple ByteBuffer and so not wrapped everything when an offset > 0 is used.
Modifications:
Correctly calculate endOffset.
Result:
All ByteBuffers are correctly wrapped when offset > 0.
Motivation:
As the ByteBuf is not set to null after release it we may try to release it again in handleReadException()
Modifications:
- set ByteBuf to null to avoid another byteBuf.release() to be called in handleReadException()
Result:
No IllegalReferenceCountException anymore
Motivation:
On Linux, you can gather various metrics using getsockopt(..., TCP_INFO,
...).
Modifications:
Add EpollSocketChannel.tcpInfo() which returns EpollTcpInfo that exposes
all metrics exposed via getsockopt(..., TCP_INFO, ...)
Result:
TCP_INFO support implemented
Motivation:
The JdkZlibDecoder and JZlibDecoder call isReadable and readableBytes in the same method. There is an opportunity to reduce the number of methods calls to just use readableBytes. JdkZlibDecoder reads from a ByteBuf with an absolute index instead of using readerIndex()
Modifications:
- Use readableBytes where isReadable was used
- Correct absolute ByteBuf index to be relative to readerIndex()
Result:
Less method calls duplicating work and preventing an index out of bounds exception.
Motivation:
In the native transport we use getpeername to obtain the remote address from the file descriptor. This may fail for various reasons in which case NULL is returned.
Modifications:
- Check for null when try to obtain remote / local address
Result:
No more NPE
Motivation:
Internet Explorer doesn't honor Set-Cookie header Max-Age attribute. It only honors the Expires one.
Modification:
Always generate an Expires attribute along the Max-Age one.
Result:
Internet Explorer compatible expiring cookies. Close#1466.
Motivation:
HttpContentDecoder had the following issues:
- For chunked content, the decoder set invalid "Content-Length" header
with length of the first decoded chunk.
- Decoding of FullHttpRequests put both the original conent and decoded
content into output. As result, using HttpObjectAggregator before the
decoder lead to errors.
- Requests with "Expect: 100-continue" header were not acknowleged:
the decoder didn't pass the header message down the handler's chain
until content is received. If client expected "100 Continue" response,
deadlock happened.
Modification:
- Invalid "Content-Length" header is removed; handlers down the chain can either
rely on LastHttpContent message or ask HttpObjectAggregator to add the header.
- FullHttpRequest is split into HttpRequest and HttpContent (decoded) parts.
- Header (HttpRequest) part of request is sent down the chain as soon as it's received.
Result:
The issues are fixed, unittest is added.
Motivation:
Pull request for RFC6265 support had some unused flag first in ClientCookieDecoder.
Modification:
Remove unused flag first.
Result:
Cleaner code.
Motivation:
A downstream consumer of Netty failed as emitting zero-length http2 data frames in a unit test resulted in assertion errors in Http2LocalFlowController. Since zero-length frames are valid, an assertion that http2 data frame length must be positive is invalid.
Modifications:
Assertions of data length in Http2LocalFlowController now permit zero.
Result:
Those running netty with assertions on can now emit zero length http2 data frames.
Motivation:
There are two member variables (addAllVisitor, setAllVisitor) which are likely not to be used in the majority of use cases.
Modifications:
Remove these member variables and rely on a method to return a new object when needed.
Result:
Two less member variables for each DefaultHeaders instance.
Motivation:
The Headers interface had two member variables (addAllVisitor, setAllVisitor) which are not necessarily always needed but are always instantiated. This may result in excess memory being used.
Modifications:
- addAllVisitor will be accessed via a method addAllVisitor() which will use lazy initialization.
- setAllVisitor will be accessed via a method addAllVisitor() which will use lazy initialization.
Result:
Potential memory savings by using lazy initialization.
Motivation:
Rfc6265Client/ServerCookieEncoder is a better replacement of the old
Client/ServerCookieEncoder, and thus there's no point of keeping both.
Modifications:
- Remove the old Client/ServerCookieEncoder
- Remove the 'Rfc6265' prefix from the new cookie encoder/decoder
classes
- Deprecate CookieDecoder
Result:
We have much better cookie encoder/decoder implementation now.
Motivation:
Currently Netty supports a weird implementation of RFC 2965.
First, this RFC has been deprecated by RFC 6265 and nobody on the
internet use this format.
Then, there's a confusion between client side and server side encoding
and decoding.
Typically, clients should only send name=value pairs.
This PR introduces RFC 6265 support, but keeps on supporting RFC 2965 in
the sense that old unused fields are simply ignored, and Cookie fields
won't be populated. Deprecated fields are comment, commentUrl, version,
discard and ports.
It also provides a mechanism for safe server-client-server roundtrip, as
User-Agents are not supposed to interpret cookie values but return them
as-is (e.g. if Set-Cookie contained a quoted value, it should be sent
back in the Cookie header in quoted form too).
Also, there are performance gains to be obtained by not allocating the
attribute name Strings, as we only want to match them to find which POJO
field to populate.
Modifications:
- New RFC6265ClientCookieEncoder/Decoder and
RFC6265ServerCookieEncoder/Decoder pairs that live alongside old
CookieEncoder/Decoder pair to not break backward compatibility.
- New Cookie.rawValue field, used for lossless server-client-server
roundtrip.
Result:
RFC 6265 support.
Clean separation of client and server side.
Decoder performance gain:
Benchmark Mode Samples Score Error
Units
parseOldClientDecoder thrpt 20 2070169,228 ± 105044,970
ops/s
parseRFC6265ClientDecoder thrpt 20 2954015,476 ± 126670,633
ops/s
This commit closes#3221 and #1406.
Motivation:
HttpPostMultipartRequestDecoder threw an ArrayIndexOutOfBoundsException
when trying to decode Content-Disposition header with filename
containing ';' or protected \\".
See issue #3326 and #3327.
Modifications:
Added splitMultipartHeaderValues method which cares about quotes, and
use it in splitMultipartHeader method, instead of StringUtils.split.
Result:
Filenames can contain semicolons and protected \\".
Motivation:
When SslHandler.unwrap() copies SSL records into a heap buffer, it does
not update the start offset, causing IndexOutOfBoundsException.
Modifications:
- Copy to a heap buffer before calling unwrap() for simplicity
- Do not copy an empty buffer to a heap buffer.
- unwrap(... EMPTY_BUFFER ...) never involves copying now.
- Use better parameter names for unwrap()
- Clean-up log messages
Result:
- Bugs fixed
- Cleaner code
Motivation:
When using OpenSslEngine with the SslHandler it is possible to reduce memory copies by unwrap(...) multiple ByteBuffers at the same time. This way we can eliminate a memory copy that is needed otherwise to cumulate partial received data.
Modifications:
- Add OpenSslEngine.unwrap(ByteBuffer[],...) method that can be used to unwrap multiple src ByteBuffer a the same time
- Use a CompositeByteBuffer in SslHandler for inbound data so we not need to memory copy
- Add OpenSslEngine.unwrap(ByteBuffer[],...) in SslHandler if OpenSslEngine is used and the inbound ByteBuf is backed by more then one ByteBuffer
- Reduce object allocation
Result:
SslHandler is faster when using OpenSslEngine and produce less GC
Motivation:
Decompression handlers contain heavy use of switch-case statements. We
use compact indentation style for 'case' so that we utilize our screen
real-estate more efficiently.
Also, the following decompression handlers do not need to run a loop,
because ByteToMessageDecoder already runs a loop for them:
- FastLzFrameDecoder
- Lz4FrameDecoder
- LzfDecoder
Modifications:
- Fix indentations
- Do not wrap the decoding logic with a for loop when unnecessary
- Handle the case where a FastLz/Lzf frame contains no data properly so
that the buffer does not leak and less garbage is produced.
Result:
- Efficiency
- Compact source code
- No buffer leak
Motivation:
HttpResponseStaus, HttpMethod and HttpVersion have methods that return
AsciiString. There's no need for object-to-string conversion.
Modifications:
Use codeAsText(), name(), text() instead of setInt() and setObject()
Result:
Efficiency
Motivation:
The SpdyHttpDecoder was modified to support pushed resources that are
divided into multiple frames. The decoder accepts a pushed
SpdySynStreamFrame containing the request headers, followed by a
SpdyHeadersFrame containing the response headers.
Modifications:
This commit modifies the SpdyHttpEncoder so that it encodes pushed
resources in a format that the SpdyHttpDecoder can decode. The encoder
will accept an HttpRequest object containing the request headers,
followed by an HttpResponse object containing the response headers.
Result:
The SpdyHttpEncoder will create a SpdySynStreamFrame followed by a
SpdyHeadersFrame when sending pushed resources.
Motivation:
NioUdtMessageRendezvoudChannelTest.basicEcho() is flakey on Linux and
failing on Windows.
Modifications:
Disable the problematic test until it's fixed.
Result:
Less annoyance
Motivation:
Currently when there are bytes left in the cumulation buffer we do a byte copy to produce the input buffer for the decode method. This can put quite some overhead on the impl.
Modification:
- Use a CompositeByteBuf to eliminate the byte copy.
- Allow to specify if a CompositeBytebug should be used or not as some handlers can only act on one ByteBuffer in an efficient way (like SslHandler :( ).
Result:
Performance improvement as shown in the following benchmark.
Without this patch:
[xxx@xxx ~]$ ./wrk-benchmark
Running 5m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 20.19ms 38.34ms 1.02s 98.70%
Req/Sec 241.10k 26.50k 303.45k 93.46%
1153994119 requests in 5.00m, 155.84GB read
Requests/sec: 3846702.44
Transfer/sec: 531.93MB
With the patch:
[xxx@xxx ~]$ ./wrk-benchmark
Running 5m test @ http://xxx:8080/plaintext
16 threads and 256 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 17.34ms 27.14ms 877.62ms 98.26%
Req/Sec 252.55k 23.77k 329.50k 87.71%
1209772221 requests in 5.00m, 163.37GB read
Requests/sec: 4032584.22
Transfer/sec: 557.64MB
Motivation:
When a user sees an error message, sometimes he or she does not know
what exactly he or she has to do to fix the problem.
Modifications:
Log the URL of the wiki pages that might help the user troubleshoot.
Result:
We are more friendly.
Motivation:
When a user deliberatively omitted netty-tcnative from classpath, he or
she will see an ugly stack trace of ClassNotFoundException.
Modifications:
Log more briefly when netty-tcnative is not in classpath.
Result:
Better-looking log at DEBUG level
Motivations:
It seems that slicing a buffer and using this slice to write to CTX will
decrease the initial refCnt to 0, while the original buffer is not yet
fully used (not empty).
Modifications:
As suggested in the ticket and tested, when the currentBuffer is sliced
since it will still be used later on, the currentBuffer is retained.
Add a test case for this issue.
Result:
The currentBuffer still has its correct refCnt when reaching the last
write (not sliced) of 1 and therefore will be released correctly.
The exception does no more occur.
This fix should be applied to all branches >= 4.0.
Motivation:
The latest stable RHEL version of 6.x is now 6.6.
Modification:
Update pom.xml's validation configuration
Result:
Can release on the latest stable RHEL version in 6.x
Motivation:
- There's no point of pre-population.
- Waste of memory and time because they are going to be cached lazily
- Some pre-populated cipher suites are ancient and will be unused
Modification:
- Remove cache pre-population
Result:
Sanity restored
Motivation
----------
The performance tests for utf8 also used the getBytes on ASCII,
which is incorrect and also provides different performance numbers.
Modifications
-------------
Use CharsetUtil.UTF_8 instead of US_ASCII for the getBytes calls.
Result
------
Accurate and semantically correct benchmarking results on utf8
comparisons.
When handling an oversized message, HttpObjectAggregator does not wait
until the last chunk is received to produce the failed message, making
AggregatedFullHttpMessage.trailingHeaders() return null.
Related: #3019
Motivation:
We have multiple (Full)HttpRequest/Response implementations and only
some of them implements toString() properly.
Modifications:
- Add the reusable string converter for HttpMessages to HttpMessageUtil
- Implement toString() of (Full)HttpRequest/Response implementations
properly using HttpMessageUtil
Result:
Prettier string representation is returned by HttpMessage
implementations.
Related: #3274
Motivation:
channelReadComplete() event is not triggered after reading successfully
in EpollDatagramChannel.
Modifications:
- Trigger exceptionCaught() event for read failure only once for less
noise
- Trigger channelReadComplete() event at the end of the read.
Result:
Fix#3274
Motivation:
Calling JNI methods is pretty expensive, so we should only do if needed.
Modifications:
Lazy call methods if needed.
Result:
Better performance.
