Motivation:
Magic numbers seem hard to read or understand.
Modification:
Replace several magic numbers with named constants.
Result:
Improve readability and make it easier to maintain.
**Motivation:**
When I was previously working on a project using Netty's HTTP/2 support, I used the newer frames approach but I struggled to find any good examples or documentation online. I did, however, see a few people ask the same (or similar) questions as me on StackOverflow and a couple of older Netty Github issues.
Reading issue [9733](https://github.com/netty/netty/issues/9733) therefore prompted me to pull together a few bits of code into this HTTP/2 frame client example.
**Modification:**
Populated the previously-empty `example/src/main/java/io/netty/example/http2/helloworld/frame/client/` folder with a HTTP/2 frame client example.
**Result:**
Gives a clear example of how the newer HTTP/2 support can be used for Netty clients.
Motivation:
ThrowableUtil.stackTraceToString is an expensive method call. So I think a log level check before this logging statement is quite needed especially in a environment with the warning log disabled.
Modification:
Add log level check simply before logging.
Result:
Improve performance in a environment with the warning log disabled.
Motivation:
939e928312 introduced MacOSDnsServerAddressStreamProvider which will ensure the right nameservers are selected when running on MacOS. To ensure this is done automatically on MacOS we should use it by default on these platforms.
Modifications:
Try to use MacOSDnsServerAddressStreamProvider when on MacOS via reflection and fallback if not possible
Result:
Ensure the right nameservers are used on MacOS even when a VPN (for example) is used.
Motivation:
I am receiving a mutlipart/form_data upload from postman. The filename contains Chinese, and so some invalid chars. We should ensure all of these are removed before trying to decode.
Modification:
Ensure all invalid characters are removed
Result:
Fixes#10087
Co-authored-by: liming.zhang <liming.zhang@luckincoffee.com>
Motivation:
In general, we will close the debug log in a product environment. However, logging without external level check may still affect performance as varargs will need to allocate an array.
Modification:
Add log level check simply before logging.
Result:
Improve performance slightly in a product environment.
Motivation:
Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.
Modifications:
- Ensure we flush if we exit the `doBeginRead()` method.
- Account for the Http2FrameCodec always synchronously finishing writes
of window update frames.
Result:
Fixes#10072
Motivation:
Some JVMs (like OpenJDK 8 on Windows) use a low resolution timer in System.nanoTime() and may return the same value more than once. This triggered an assertion failure when deadlineNanos was equal to nanoTime and AbstractScheduledEventExecutor#pollScheduledTask called #setConsumed.
Modifications:
With this change the assertion checks exactly the same condition as AbstractScheduledEventExecutor#pollScheduledTask, and will no longer fail under these circumstances.
Result:
Fixes#10070.
Motivation:
When the HttpContentCompressor is put on an alternate EventExecutor, the order of events should be
preserved so that the compressed content is correctly created.
Modifications:
- checking that the executor in the ChannelHandlerContext is the same executor as the current executor when evaluating if the handler should be skipped.
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/10067
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
PromiseTask.isCancelled performs an unsynchronized read and so may trigger race-detectors. As this was just done for a small optimization which should not really make a lot of difference we should just remove it.
Modifications:
Remove unsynchronized read optimization
Result:
Fixes https://github.com/netty/netty/issues/10026.
Motivation:
The Snappy crc32c checksum produced by SnappyFrameEncoder maybe failed to be validated on other languages snappy decoder, such as golang/snappy.
Modification:
- make the 4-byte cast later after the mask operation. Because whether retaining the higher 4-7 bytes in a long java type will make difference in (checksum >> 15 | checksum << 17) + 0xa282ead8 result.
Result:
Checksum correctly calculated
Motivation:
Often it is useful to be able to detect different sorts of SSL errors that cause the handshake to fail. To make this easier we should throw and explicit exception type for handshake timeouts.
Modifications:
- Add SslHandshakeTimeoutException (which extends SSLHandshakeException) and use it for handshake timeouts
- Adjust testcases
Result:
Easier to detect that handshake failed because of a timeout
Motivation:
We did not correctly account for produced bytes when SSL_write(...) returns -1 in all cases. This could lead to lost data and so a corrupt SSL connection.
Modifications:
- Always ensure we calculate the produced bytes correctly
- Add unit tests
Result:
Fixes https://github.com/netty/netty/issues/10041
Motivation:
ImmediateExecutor needs more test cases.
Modification:
Add several test cases for ImmediateExecutor.
Result:
Improve test coverage slightly.
Motivation:
Our parsing of the initial line / http headers did treat some characters as separators which should better trigger an exception during parsing.
Modifications:
- Tighten up parsing of the inital line by follow recommentation of RFC7230
- Restrict separators to OWS for http headers
- Add unit test
Result:
Stricter parsing of HTTP1
Motivation:
java.io.File.listFiles() may return null and cause a unexpected NPE.
Modification:
Add a null check for variable files. And if variable files is null, the compressHeapDumps method will just return after logging a error message.
Result:
Fix the potential NPE.
Motivation:
NetworkInterface.getByInetAddress() may return null on Android. This is incorrect by the API but still happens. To help our users we should provide a workaround
Modifications:
Just return an empty Enumeration when null is returned.
Result:
Fixes https://github.com/netty/netty/issues/10045
Motivation:
The type IntObjectMap with the key of primitive int can help lower the cost of boxing and unboxing, and improve performance. I think it's more suitable for fragments field in SctpMessageCompletionHandler.java.
Modification:
Just replace the type of fragments field with IntObjectMap.
Result:
Improve performance slightly while decoding sctp message.
Motivation:
We should not include the number of ServerChannel that are part of the DefaultChannelGroup when specify the initial size of the LinkedHashMap
Modifications:
Only use the number of the non ServerChannel
Result:
Reduce memory-footprint
Motivation:
Having only the ROOT label (".") as the name is valid, but Android 9 and prior does not correctly handle the case. We should add a workaround for it.
Modifications:
Detect if we are on Android and if so check if the name is the ROOT label and if so just return the name without trying to convert it
Result:
Fixes https://github.com/netty/netty/issues/10034
Motivation:
In SniHandlerTest we depended on implementation details of the SSLEngine. We should better not doing this
Modifications:
Just release all outbound data
Result:
Dont depend on implementation details
Motivation:
java.io.File.listFiles() may return null and cause a unexpected NPE.
Modification:
Extract a local variable from the return value of File.listFiles() and do a null check.
Result:
Fix the potential NPE.
Motivation:
The method body of fillInStackTrace method of IllegalStateException class in SimpleChannelPool.java is so simple (just return this) that there is no need to be marked as synchronized.
Modification:
Remove the synchronized flag of fillInStackTrace method.
Result:
It can improve performance slightly while creating a IllegalStateException instance.
Motivation:
The method body of fillInStackTrace method of TimeoutException class in FixedChannelPool.java is so simple (just return this) that there is no need to be marked as synchronized.
Modification:
Remove the synchronized flag of fillInStackTrace method.
Result:
It can improve performance slightly while throwing a TimeoutException.
Motivation:
Due incorrectly handling of reference count of the clientHello ByteBuf we may overrelease the buffer. This did show up in the log of a test:
11:55:16.595 [main] DEBUG i.n.h.ssl.SslClientHelloHandler - Unexpected client hello packet: 16030100bd010000b90303a74225676d1814ba57faff3b3663656ed05ee9dbb2a4dbb1bb1c32d2ea5fc39e0000000100008c0000001700150000164348415434e380824c45414e434c4f5544e38082434e000b000403000102000a00340032000e000d0019000b000c00180009000a00160017000800060007001400150004000500120013000100020003000f0010001100230000000d0020001e060106020603050105020503040104020403030103020303020102020203000f00010133740000
io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1
at io.netty.util.internal.ReferenceCountUpdater.toLiveRealRefCnt(ReferenceCountUpdater.java:74)
at io.netty.util.internal.ReferenceCountUpdater.release(ReferenceCountUpdater.java:138)
at io.netty.buffer.AbstractReferenceCountedByteBuf.release(AbstractReferenceCountedByteBuf.java:100)
at io.netty.handler.ssl.SslClientHelloHandler.releaseIfNotNull(SslClientHelloHandler.java:181)
at io.netty.handler.ssl.SslClientHelloHandler.select(SslClientHelloHandler.java:225)
at io.netty.handler.ssl.SslClientHelloHandler.decode(SslClientHelloHandler.java:149)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:355)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:377)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:363)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.embedded.EmbeddedChannel.writeInbound(EmbeddedChannel.java:343)
at io.netty.handler.ssl.SniHandlerTest.testNonAsciiServerNameParsing(SniHandlerTest.java:297)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:19)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runners.Suite.runChild(Suite.java:128)
at org.junit.runners.Suite.runChild(Suite.java:27)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Modifications:
Correctly transfer lifetime of buffer and so not over-release it.
Result:
Correctly handle buffer lifecycle and so not swallow the original exception
Motivation:
Current pipeline handler replace tests are replacing handler with same name.
we need test that test handler can renamed, old handlers are removed from pipeline.
Modification:
There is coverage missing related to renaming of handlers
Result:
Adds missing tests
Co-authored-by: phani254 <phani254@yahoo.com>
Motivation:
BufferedReader.readLine() may return null and cause a NPE.
Modification:
Simply add a null check.
Result:
If BufferedReader.readLine() returns null, the sysctlGetInt will just return null rather than cause NPE.
Motivation:
Modifications:
- Wrap the code and execute with an AccessController
- Ignore SecurityException (by just logging it)
- Add some more debug logging
Result:
Fixes https://github.com/netty/netty/issues/10017
Motivation:
Even if it was stored in the string constant pool, I thought it was safe to compare it through the Equals() method.
Modification:
So, I changed "==" comparison to equals() comparison
Result:
It has become safer to compare String values with different references and with the same values.
Motivation:
We did had some System.out.println(...) call in a test which seems to be some left-over from debugging.
Modifications:
Remove System.out.println(...)
Result:
Code cleanup
Motivation:
GlobalEventExecutor/SingleThreadEventExecutor#taskQueue is BlockingQueue.
Modifications:
Add allowBlockingCallsInside configuration for GlobalEventExecutor/SingleThreadEventExecutor#takeTask.
Result:
Fixes#9984
When BlockHound is installed, GlobalEventExecutor/SingleThreadEventExecutor#takeTask is not reported as a blocking call.
Motivation:
We need to carefully manage flushes to ensure we not discard these by mistake due wrongly implemented consolidation of flushes.
Modifications:
- Ensure we reset flag before we actually call flush0(...)
- Add unit test
Result:
Fixes https://github.com/netty/netty/issues/10015
Motivation:
NioEventLoopTest.testChannelsRegistered sometimes fails due a race which is related to how SelectionKey and Selector is implemented in the JDK. In the current implementation it will "lazy" remove SelectionKeys from the Set which means we may still have these included sometimes when we use size() to get the number of SelectionKeys.
Modifications:
Just retry to read the number of registered Channels if we still see 2
Result:
Fixes https://github.com/netty/netty/issues/9895
Motivation:
We should keep the scope of the synchronized block as small as possible.
Modifications:
Reduce scope by copy to an array first before iterate through it
Result:
Smaller scope of synchronized
Motivation
As part of a recent commit for issue
https://github.com/netty/netty/issues/9861 the HttpObjectDecoder was
changed to throw an IllegalArgumentException (and produce a failed
decoder result) when decoding a message with both "Transfer-Encoding:
chunked" and "Content-Length".
While it seems correct for Netty to try to sanitize these types of
messages, the spec explicitly mentions that the Content-Length header
should be *removed* in this scenario.
Both Nginx 1.15.9 and Tomcat 9.0.31 also opt to remove the header:
b693d7c198/java/org/apache/coyote/http11/Http11Processor.java (L747-L755)0ad4393e30/src/http/ngx_http_request.c (L1946-L1953)
Modifications
* Change the default behavior from throwing an IllegalArgumentException
to removing the "Content-Length" header
* Extract the behavior to a new protected method,
handleChunkedEncodingWithContentLength(), that can be overridden to
change this behavior (or capture metrics)
Result
Messages of this nature will now be successfully decoded and have their
"Content-Length" header removed, rather than creating invalid messages
(decoder result failures). Users will be allowed to override and
configure this behavior.
Motivation:
Users of the DefaultHttp2ConnectionDecodcer are notified of inbound GoAwayFrames
after the connection has already closed any ignored streams, potentially
losing the signal that some streams may have been ignored by the peer and
are thus retryable.
Modifications:
Reorder the notifications of the frame and connection listeners to
propagate the frame first, giving the frame listeners the opportunity to
clean up ignored streams in their own way.
Result:
Fixes#9986
Motivation:
When an `AddressResolverGroup` closes, it can leave unwanted listeners attached to its `EventExecutor`'s termination future.
Modifications:
- Keep track of listeners attached to termination futures
- Clear listeners if the `AddressResolverGroup` closes before its associated executor(s)
Result:
Unwanted listeners no longer remain in memory after an `AddressResolverGroup` closes before its associated executor(s).
Motivation:
FixedCompositeByteBuf.isDirect() should return the same value as EMPTY_BUFFER.isDirect() when constructed via an empty array
Modifications:
- Return correct value when constructed via empty array.
- Add unit test
Result:
FixedCompositeByteBuf.isDirect() returns correct value
Motivation:
ClearTextHttp2ServerUpgradeHandler is currently more complex then needed. We can simplify it by directly implement the prior-knowledge logic as part of the handler.
Modifications:
Merge inner PriorKnowledgeHandler logic into ClearTextHttp2ServerUpgradeHandler by extending ByteToMessageDecoder directly
Result:
Cleaner code and less pipeline operations
