Motivation:
The DefaultHttp2ConnectionDecoder class is calling verifyPrefaceReceived() for almost every frame event at all times.
The Http2ConnectionHandler class is calling readClientPrefaceString() on every decode event.
Modifications:
- DefaultHttp2ConnectionDecoder should not have to continuously call verifyPrefaceReceived() because it transitions boolean state 1 time for each connection.
- Http2ConnectionHandler should not have to continuously call readClientPrefaceString() because it transitions boolean state 1 time for each connection.
Result:
- Less conditional checks for the mainstream usage of the connection.
Motivation:
The backport of a6c729bdf8 failed.
Modifications:
- Make sure the interfaces are correctly implemented when backporting.
Result:
Microbenchmark compiles and runs on 4.1 branch.
Motivation:
The current DefaultHttp2RemoteFlowController's writePendingBytes currently operates in 2 passes. The first allocates bytes and optionally writes some frames. The second pass just loops across all active streams and writes all remaining bytes.
If streams can be removed/added as a side effect of writing (EOS or error) then we need to take more care when the write actually occurs. Moving all of the writes to the second loop (across active streams) is simpler since we can just make a copy of the list and not worry about any restructuring of the priority tree that may result.
Modifications:
Modified DefaultHttp2RemoteFlowController.writePendingBytes to only allocate bytes on the first pass and then write any allocated bytes on the second pass.
Result:
Side effects resulting from writing should no longer impact the flow control algorithm.
Motivation:
A microbenchmark will be useful to get a baseline for performance.
Modifications:
- Introduce a new microbenchmark which tests the Http2DefaultFrameWriter.
- Allow benchmarks to run without thread context switching between JMH and Netty.
Result:
Microbenchmark exists to test performance.
Motivation:
The Http2ConnectionHandler writeRstStream method allows RST_STREAM frames to be sent when we do not know about the stream and after a RST_STREAM frame has already been sent. This may lead to sending frames when we should not according to the HTTP/2 spec. There is also the potential to notify the closeListener multiple times if the closeStream method is called multiple times.
Modifications:
- Prevent RST_STREAM from being sent if we don't know about the stream, or if we already sent the RST_STREAM.
- Prevent the closeListener from being notified multiple times.
Result:
More robust writeRstStream logic in boundary conditions.
Motivation:
There was a new draft for HTTP/2. We should support the new draft.
Modifications:
- Review the HTTP/2 draft 17 specification, and update code to reflect changes.
Result:
Support for HTTP/2 draft 17.
Motivation:
There are new versions of the ALPN and NPN dependencies. There was also some backport misses in the pom file related to ALPN/NPN.
Modifications:
- Add new versions for ALPN/NPN dependencies.
- Backport missed pieces from pom.xml.
Result:
Updated version of ALPN/NPN versions.
Motivation:
The codec-http2 module was excluded from the all/pom.xml.
Modifications:
Include the codec-http2 dependency in the all/pom.xml.
Results:
Projects including the all dependency get codec-http2.
Motivation:
The example script has some changes that have not yet been backported. These changes should be backported.
Modifications:
run-example.sh changes related to http/2 are backported
Result:
HTTP/2 examples can be run on the 4.1 branch.
Motivation:
- In FlowState.write(...) we are currently swalloing an exception.
- In my previous commit I introduced a compiler warning by not making
a local variabe final.
Modifications:
- Have FlowState.cancel() take a Throwable.
- Make the variable final.
Result:
No more swallowed exceptions and warnings.
Motivation:
- The encoder and decoder should be closed right after the handler releases its resources.
- The clientPrefaceString is allocated in the constructor but releases in handlerRemoved.
If the handler is never added to the pipeline, the clientPrefaceString will never be
released.
Modifications:
- Call encoder.close() and decoder.close() on channelInactive.
- Release the clientPrefaceString on handlerRemoved.
Result:
- The encoder and decoder get closed right after the handler's resources are freed.
- It's easier to verify that the clientPrefaceString will also get released.
Motivation:
Current AbstractMemcacheObjectEncoder does unnecessary message type checking if the message is MemcacheMessage type.
Modifications:
Returns after encoding MemcacheMessage message.
Result:
Small performance improvement for this encoder.
(Ported @luciferous's changes against 3.10)
Motivation:
The current implementation of the encoder writes each character of the
String as a single byte to the buffer, however not all characters are
mappable to a single byte.
Modifications:
If a character is outside the ASCII range, it's converted to '?'.
Result:
A safer encoder for String to ASCII, which substitutes unmappable
characters with'?'.
Motivation:
The Http2FrameLogger is currently using the internal logging classes. We should change this so that it's using the public classes and then converts internally.
Modifications:
Modified Http2FrameLogger and the examples to use the public LogLevel class.
Result:
Fixes#2512
Related: #3504
Motivation:
There are two places in the SocksAuthRequest constructor where an
IllegalArgumentException is thrown with a password as part of the
exception message.
This constitutes mishandling of confidential information, which can
compromise user privacy and is flagged as critical by security scanners.
Modifications:
Mask the password in the exception messages
Result:
No unexpected password leak
Motivation:
With the current implementation the client protocol preference list
takes precedence over the one of the server, since the select method
will return the first item, in the client list, that matches any of the
protocols supported by the server. This violates the recommendation of
http://tools.ietf.org/html/rfc7301#section-3.2.
It will also fail with the current implementation of Chrome, which
sends back Extension application_layer_protocol_negotiation, protocols:
[http/1.1, spdy/3.1, h2-14]
Modifications:
Changed the protocol negotiator to prefer server’s list. Added a test
case that demonstrates the issue and that is fixed with the
modifications of this commit.
Result:
Server’s preference list is used.
Motivation:
The current documentation for Endpoint methods referring to concurrent streams and the SETTINGS_MAX_CONCURRENT_STREAMS setting are a bit confusing.
Modifications:
Renamed a few of the methods and added more clear documentation.
Result:
Fixes#3451
Motivation:
There are two writeRstStream methods in the DefaultHttp2ConnectionEncoder.
One of the two is neither used nor part of the Http2FrameWriter interface.
Modifications:
Delete the method.
Result:
Fewer lines of dead code.
Motivation:
For every write of a flow controlled frame (HEADERS, DATA) we are allocating
a Frame object that is not necessary anymore as it does not maintain any
state, besides the payload.
Modifications:
Remove the Frame class and directly add the payload to the pending write queue.
Result:
One few object allocation per write of a flow controlled frame.
Motivation:
While the LengthFieldBasedFrameDecoder supports a byte order the LengthFieldPrepender does not.
That means that I can simply add a LengthFieldBasedFrameDecoder with ByteOrder.LITTLE_ENDIAN to my pipeline
but have to write my own Encoder to write length fields in little endian byte order.
Modifications:
Added a constructor that takes a byte order and all other parameters.
All other constructors delegate to this one with ByteOrder.BIG_ENDIAN.
LengthFieldPrepender.encode() uses this byte order to write the length field.
Result:
LengthFieldPrepender will write the length field in the defined byte order.
Motivation:
When an error happens during loading the native library it may try to generate a new RuntimeException before the RuntimeException is loaded.
Modifications:
- Load RuntimeException as first
Result:
No more segfaults possible
Motivation:
The logger was always performing a hex dump of the ByteBufs regarless whether or not the log would take place.
Modifications:
Fixed the logger to avoid serializing the ByteBufs and calling the varargs method if logging is not enabled.
Result:
The loggers should run MUCH faster when disabled.
Motivation:
On a system where ipv4 and ipv6 are supported a user may want to use -Djava.net.preferIPv4Stack=true to restrict it to use ipv4 only.
This is currently ignored with the epoll transport.
Modifications:
Respect java.net.preferIPv4Stack system property.
Result:
-Djava.net.preferIPv4Stack=true will have the effect the user is looking for.
Motivation:
If HEADERS or DATA frames are pending due to a too small flow control
window, a frame with the END_STREAM flag set will wrongfully cancel
all pending frames (including itself).
Also see grpc/grpc-java#145
Modifications:
The transition of the stream state to CLOSE / HALF_CLOSE due to a
set END_STREAM flag is delayed until the frame with the flag is
actually written to the Channel.
Result:
Flow control works correctly. Frames with END_STREAM flag will no
longer cancel their preceding frames.
Motivation:
When remove0() is called multiple times for an DefaultAttribute it can cause corruption of the internal linked-list structure.
Modifications:
- Ensure remove0() can not cause corruption by null out prev and next references.
Result:
No more corruption possible
Related: #3478
Motivation:
DefaultNameResolver uses InetSocketAddress.getHostString() instead of
getHostName(). Because Netty uses the DefaultNameResolver by default and
getHostString() is available only since Java 7, a user cannot use Netty
on Java 6 anymore.
Modifications:
Use InetSocketAddress.getHostName() which is practically same and also
is available in Java 6.
Result:
Netty 4.1 runs on Java 6 again.
Related: #3476
Motivation:
Some users use TrafficCounter for other uses than we originally
intended, such as implementing their own traffic shaper. In such a
case, a user does not want to specify an AbstractTrafficShapingHandler.
Modifications:
- Add a new constructor that does not require an
AbstractTrafficShapingHandler, so that a user can use it without it.
- Simplify TrafficMonitoringTask
- Javadoc cleanup
Result:
We open the possibility of using TrafficCounter for other purposes than
just using it with AbstractTrafficShapingHandler. Eventually, we could
generalize it a little bit more, so that we can potentially use it for
other uses.
Motivation:
Not knowing which unit is returned by the maxContentLength() of the Messageggregator when reading the Javadoc is annoying and can be a source of bugs.
Modifications:
Added the mention "in bytes"
Result:
Javadoc is clear.
Motivation:
Not knowing which unit is used for the maxContentLength of the HttpObjectAggregator when reading the Javadoc is annoying and can be a source of bugs.
Modifications:
Added the mention "in bytes"
Result:
Javadoc is clear.
Related: #3464
Motivation:
When a connection attempt is failed,
NioSocketChannelUnsafe.closeExecutor() triggers a SocketException,
suppressing the channelUnregistered() event.
Modification:
Do not attempt to get SO_LINGER value when a socket is not open yet.
Result:
One less bug
Motivation:
Due a a regression that was introduced by b898bdd we failed to set the localAddress if the connect did not success directly.
Modifications:
Correct set localAddress in doConnect(...)
Result:
Be able to get the localAddress in all cases.
Related: #3445
Motivation:
HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line. If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.
Modifications:
- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.
Result:
One less bug
Motivation:
`Unpooled` javadoc's mentioned the generation of hex dump and swapping an integer's byte order,
which are actually provided by `ByteBufUtil`.
Modifications:
Sentence moved to `ByteBufUtil` javadoc.
Result:
`Unpooled` javadoc is correct.
Motivation:
During 6b941e9bdb I introduced a regression that could cause an IllegalStateException.
A non-proper fix was commited as part of #3443. This commit add a proper fix.
Modifications:
Remove FileDescriptor.INVALID and add FileDescriptor.isOpen() as replacement. Once FileDescriptor.close() is called isOpen() will return false.
Result:
No more IllegalStateException caused by a close channel.
Motivation:
EpollDragramChannel never calls fireChannelActive after connect() which is a bug.
Modifications:
Correctly call fireChannelActive if needed
Result:
Correct behaviour
Motivation:
Before struct's were passed per value and not pointer. This did enforce a memory copy which is not needed.
Modifications:
- Use "const struct....*" as replacement
Result:
No more unnecessary memory copies
Motivation:
When create address from filedescriptor we may use incorrect byte order and so end up with an incorrect InetAddress.
Modification:
Not manually shift bytes
Result:
Correct address in all cases.
Motivation:
Because of a regression sometimes accept could produce an IllegalArgumentException
Modifications:
Correctly respect offset when decode port and scope id.
Result:
No more IllegalArgumentException
Motivation:
Twitter hpack has upgraded to 0.10.1 to fix a parsing bug.
Modifications:
Updated the parent pom to specify the dependency version.
Result:
HTTP/2 updated to the latest hpack release.
Motivation:
This is a regression that was introduced as part of 6b941e9bdb. The regression could produce an "infinity" triggering of IllegalStateException if a channel goes inactive while process the events for it.
Modifications:
Correctly check if the channel is still active before trigger the callbacks.
Result:
No more IllegalStateException
Motivation:
There is a small race in the native transport where an accept(...) may success but a later try to obtain the remote address from the fd may fail is the fd is already closed.
Modifications:
Let accept(...) directly set the remote address.
Result:
No more race possible.
