Fix issue #10508 where PARANOID mode slow down about 1000 times compared to ADVANCED.
Also fix a rare issue when internal buffer was growing over a limit, it was partially discarded
using `discardReadBytes()` which causes bad changes within previously discovered HttpData.
Reasons were:
Too many `readByte()` method calls while other ways exist (such as keep in memory the last scan position when trying to find a delimiter or using `bytesBefore(firstByte)` instead of looping externally).
Changes done:
- major change on way buffer are parsed: instead of read byte per byte until found delimiter, try to find the delimiter using `bytesBefore()` and keep the last unfound position to skeep already parsed parts (algorithms are the same but implementation of scan are different)
- Change the condition to discard read bytes when refCnt is at most 1.
Observations using Async-Profiler:
==================================
1) Without optimizations, most of the time (more than 95%) is through `readByte()` method within `loadDataMultipartStandard` method.
2) With using `bytesBefore(byte)` instead of `readByte()` to find various delimiter, the `loadDataMultipartStandard` method is going down to 19 to 33% depending on the test used. the `readByte()` method or equivalent `getByte(pos)` method are going down to 15% (from 95%).
Times are confirming those profiling:
- With optimizations, in SIMPLE mode about 82% better, in ADVANCED mode about 79% better and in PARANOID mode about 99% better (most of the duplicate read accesses are removed or make internally through `bytesBefore(byte)` method)
A benchmark is added to show the behavior of the various cases (one big item, such as File upload, and many items) and various level of detection (Disabled, Simple, Advanced, Paranoid). This benchmark is intend to alert if new implementations make too many differences (such as the previous version where about PARANOID gives about 1000 times slower than other levels, while it is now about at most 10 times).
Extract of Benchmark run:
=========================
Run complete. Total time: 00:13:27
Benchmark Mode Cnt Score Error Units
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel thrpt 6 2,248 ± 0,198 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel thrpt 6 2,067 ± 1,219 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel thrpt 6 1,109 ± 0,038 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel thrpt 6 2,326 ± 0,314 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel thrpt 6 1,444 ± 0,226 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel thrpt 6 1,462 ± 0,642 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel thrpt 6 0,159 ± 0,003 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel thrpt 6 1,522 ± 0,049 ops/ms
Motivation:
`DelegatingDecompressorFrameListener#initDecompressor` has multiple dots `.` in comments. However, it should not have that.
Modification:
Removed multiple dots.
Result:
Clean comment
Motivation:
Passing a null value of byte[] to the `Unsafe.copyMemory(xxx)` would cause the JVM crash
Modification:
Add null checking before calling `PlatformDependent.copyMemory(src, xxx)`
Result:
Fixes#10791 .
Motivation:
https in xmlns URIs does not work and will let the maven release plugin fail:
```
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.779 s
[INFO] Finished at: 2020-11-10T07:45:21Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.5.3:prepare (default-cli) on project netty-parent: Execution default-cli of goal org.apache.maven.plugins:maven-release-plugin:2.5.3:prepare failed: The namespace xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" could not be added as a namespace to "project": The namespace prefix "xsi" collides with an additional namespace declared by the element -> [Help 1]
[ERROR]
```
See also https://issues.apache.org/jira/browse/HBASE-24014.
Modifications:
Use http for xmlns
Result:
Be able to use maven release plugin
Motivation:
`HttpConversionUtil#toFullHttpResponse` and `HttpConversionUtil#toFullHttpRequest` has `ByteBufAllocator` which is used for building `FullHttpMessage` and then data can be appended with `FullHttpMessage#content`. However, there can be cases when we already have `ByteBuf` ready with data. So we need a parameter to add `ByteBuf` directly into `FullHttpMessage` while creating it.
Modification:
Added `ByteBuf` parameter,
Result:
More functionality for handling `FullHttpMessage` content.
Motivation:
Sometimes it would be helpful to easily detect if an operation failed due the SSLEngine already be closed.
Modifications:
Add special exception that is used when the engine was closed
Result:
Easier to detect a failure caused by a closed exception
Motivation:
Printing download progress in the build log makes it harder to see what's wrong when the build fails.
Modification:
Change the maven command to not show transfer progress, also enable batch mode so Maven don't print in colors that we can't see anyway.
Result:
More concise code analysis build logs.
Motivation:
We recently released a new netty-build version and changed the artifact name
Modifications:
Update version and artifact name
Result:
Use latest version
Motivation:
We should have a method to add `HttpScheme` if `HttpRequest` does not contain `x-http2-scheme` then we should use add it if `HttpToHttp2ConnectionHandler` is build using specified `HttpScheme`.
Modification:
Added `HttpScheme` in `HttpToHttp2ConnectionHandlerBuilder`.
Result:
Automatically add `HttpScheme` if missing in `HttpRequest`.
Motivation:
When parsing HEADERS, connection errors can occur (e.g., too large of
headers, such that we don't want to HPACK decode them). These trigger a
GOAWAY with a last-stream-id telling the client which streams haven't
been processed.
Unfortunately that last-stream-id didn't include the stream for the
HEADERS that triggered the error. Since clients are free to silently
retry streams not included in last-stream-id, the client is free to
retransmit the request on a new connection, which will fail the
connection with the wrong last-stream-id, and the client is still free
to retransmit the request.
Modifications:
Have fatal connection errors (those that hard-cut the connection)
include all streams in last-stream-id, which guarantees the HEADERS'
stream is included and thus should not be silently retried by the HTTP/2
client.
This modification is heavy-handed, as it will cause racing streams to
also fail, but alternatives that provide precise last-stream-id tracking
are much more invasive. Hard-cutting the connection is already
heavy-handed and so is rare.
Result:
Fixes#10670
Motivation:
We received a [bug report](https://bugs.chromium.org/p/chromium/issues/detail?id=1143320) from the Chrome team at Google, their canary builds are failing [HTTP/2 GREASE](https://tools.ietf.org/html/draft-bishop-httpbis-grease-00) testing to netflix.com.
The reason it's failing is that Netty can't handle unknown frames without an active stream created. Let me know if you'd like more info, such as stack traces or repro steps.
Modification:
The change is minor and simply ignores unknown frames on the connection stream, similarly to `onWindowUpdateRead`.
Result:
I figured I would just submit a PR rather than filing an issue, but let me know if you want me to do that for tracking purposes.
Motivation:
JUnit 5 is the new hotness. It's more expressive, extensible, and composable in many ways, and it's better able to run tests in parallel. But most importantly, it's able to directly run JUnit 4 tests.
This means we can update and start using JUnit 5 without touching any of our existing tests.
I'm also introducing a dependency on assertj-core, which is like hamcrest, but arguably has a nicer and more discoverable API.
Modification:
Add the JUnit 5 and assertj-core dependencies, without converting any tests at time time.
Result:
All our tests are now executed through the JUnit 5 Vintage Engine.
Also, the JUnit 5 test APIs are available, and any JUnit 5 tests that are added from now on will also be executed.
Motivation:
Allowing null handlers allows for more convenient idioms in
conditionally adding handlers, e.g.,
ch.pipeline().addLast(
new FooHandler(),
condition ? new BarHandler() : null,
new BazHandler()
);
Modifications:
* Change addFirst(..) and addLast(..) to skip null handlers, rather than
break or short-circuit.
* Add new unit tests.
Result:
* Makes addFirst(..) and addLast(..) behavior more consistent
* Resolves https://github.com/netty/netty/issues/10728
Motivation:
PoolChunk maintains multiple PriorityQueue<Long> collections. The usage
of PoolChunk#removeAvailRun unboxes the Long values to long, and then
this method uses queue.remove(..) which will auto box the value back to
Long. This creates unnecessary allocations via Long.valueOf(long).
Modifications:
- Adjust method signature and usage of PoolChunk#removeAvailRun to avoid
boxing
Result:
Less allocations as a result of PoolChunk#removeAvailRun.
Motivation:
`HttpConversionUtil#toHttpResponse` translates `Http2Headers` to `HttpResponse`. It uses `#addHttp2ToHttpHeaders(..., boolean isRequest)` to do so. However, `isRequest` field is set to `true` instead of `false`. It should be set to `false` because we're doing conversion of Response not Request.
Modification:
Changed `true` to `false`.
Result:
Correctly translates `Http2Headers` to `HttpResponse`.
Motivation:
We should have the `toString` method in `DefaultHttp2WindowUpdateFrame` because it makes debugging a little easy.
Modification:
Added `toString` method.
Result:
`toString` method to help in debugging.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
`Http2Frame` has extra empty line after `String name();`. However, it should not be there.
Modification:
Removed extra empty line.
Result:
Empty-line code style now matching with other classes.
Motivation:
`Http2FrameCodec#write(...)` has typo in comment.
`// In the event of manual SETTINGS ACK is is assumed the encoder will apply the earliest received but not`.
The typo is `is is`. However, it should be `it is`.
Modification:
Changed `is is` to `it is`.
Result:
Correct comment without typos.
Motivation:
`Http2DataFrame#isEndStream()` JavaDoc says `Returns {@code true} if the END_STREAM flag ist set.`. The typo is `ist` word. However, it should be `is`.
Modification:
Changed `ist` to `is`.
Result:
Better JavaDoc by fixing the typo.
Motivation:
`Http2HeadersFrame#isEndStream()` JavaDoc says `Returns {@code true} if the END_STREAM flag ist set.`. The typo is `ist` word. However, it should be `is`.
Modification:
Changed `ist` to `is`.
Result:
Better JavaDoc by fixing the typo.
Motivation:
`DefaultChannelId` uses reflection to access the JMX runtime. This needs some extra config for GraalVM.
Modification:
Add config for GraalVM
Result:
Works when using GraalVM native image
Motivation:
Heart-beat is a functionality of STOMP enabling clients and servers to know the healthiness of the connection. The current decoder didn't allow for heart-beat messages to be forwarded to the decoder and were simply swallowed as part of the frame decoding.
Modifications:
Adding support for heartbeat message parsing by introducing a new HEARTBEAT command (not a real STOMP command).
Heartbeat received on the channel will trigger a StompFrame with the command set to HEARTBEAT.
Sending heartbeat on the channel is achieved by creating a StompFrame with the command set to HEARTBEAT.
Result:
Heartbeat can now be received/sent and acted upon to determine the healthiness of the connection and terminate it if needed.
Motivation:
HttpServerUpgradeHandler takes a list of protocols from an incoming
request and uses them for building a response.
Although the class does some validation while parsing the list,
it then disables HTTP header validation when it builds a responst.
The disabled validation may potentially allow
HTTP response splitting attacks.
Modifications:
- Enabled HTTP header validation in HttpServerUpgradeHandler
as a defense-in-depth measure to prevent possible
HTTP response splitting attacks.
- Added a new constructor that allows disabling the validation.
Result:
HttpServerUpgradeHandler validates incoming protocols
before including them into a response.
That should prevent possible HTTP response splitting attacks.
Motivation:
Subscription ID property of the PUBLISH message may be repeated multiple times, which wasn't taken into account when developing `MqttProperties` API.
Modification:
Store Subscription ID properties separately from others - in `MqttProperties.subscriptionIds`.
Add `MqttProperties.getProperties` method to retrieve properties that may be repeated.
Change internal representation of User Properties for uniformity with Subscription ID - now they're stored in `MqttProperties.userProperties` rather than the common hash map.
Result:
Multiple Subscription ID properties can be set or retrieved.
Motivation:
a63faa4fa1 missed to update the macos specific c files for the resolver.
Modifications:
Fix up c files so it compiles again on macos
Result:
Compiles on macos again
Motivation:
We had a lot of duplication in our jni code which was mostly due macros but also related to how we support shading. By using netty-jni-util we can share all the code between netty and netty-tcnative ( and possible other jni based netty projects in the future).
Modifications:
- Use netty-jni-util and re-use its macros / functions
- Remove duplicated code
- Adjust build files
Result:
Less code duplication for JNI
Motivation:
`addHttp2ToHttpHeaders(int streamId, Http2Headers sourceHeaders, FullHttpMessage destinationMessage, boolean addToTrailer)`
should match
`addHttp2ToHttpHeaders(int streamId, Http2Headers inputHeaders, HttpHeaders outputHeaders, HttpVersion httpVersion, boolean isTrailer, boolean isRequest)`.
However, the `Http2Headers` variable name is different.
Modification:
Changed `sourceHeaders` to `inputHeaders`.
Result:
Variable and JavaDoc naming now correct.
Motivation:
Http2ToHttpHeaderTranslator is a private class but translateHeaders(Iterable<Entry<CharSequence, CharSequence>>) is public but it should be package-private.
Modification:
Removed public.
Result:
Correct access modifer.
Motivation:
Some buffers implement ByteBuf#order(order) by wrapping themselves in a SwappedByteBuf.
The SwappedByteBuf is then responsible for swapping the byte order on accesses.
The explicitly little-endian accessor methods, however, should not be swapped to big-endian, but instead remain explicitly little-endian.
Modification:
The SwappedByteBuf was passing through calls to e.g. writeIntLE, to the big-endian equivalent, e.g. writeInt.
This has been changed so that these calls delegate to their explicitly little-endian counterpart.
Result:
This makes all buffers that make use of SwappedByteBuf for their endian-ness configuration, consistent with all the buffers that use other implementation strategies.
In the end, all buffers now behave exactly the same, when using their explicitly little-endian accessor methods.
Motivation:
ddebc1027d missed to make Errors.throwConnectException(...) public
Modifications:
Make method public
Result:
Be able to use Errors.throwConnectException(...) from other module
Motivation:
03aafb9cff did ensure we fail while loading a natibe library which is not compatible. While this is great it is still sometimes hard for people to understand what NoSuchMethodError means in this context.
Modifications:
If possible rethrow the NoSuchMethodError and provide some more hints about multiple versions of the shared library
Result:
Easier to understand for people why loading fails
Motivation:
ddebc10 added a few adjustments that are needed by io_uring that we will add as an incubator repository. Unfortunally we missed the changed in PlatformDependent*.
Modifications:
Add missing methods
Result:
Be able to compile io_uring code against core netty
Motivation:
During the last few month we did develop an io_uring based transport which shows very promising performance numbers. To give it more time to bake we will develop it outside of netty in an "incubator" module which will make it clear to users what to expect and also allow us to seperate its release cycle. While the implementation of it is very self contained there are few small adjustments that need to be made in netty itself to allow us to reuse code.
Modifications:
- AbstractChannel: Add method which can be used when a write fails and remove final from one method
- IovArray: Allow to create an IovArray from a ByteBuf instance
- FileDescriptor: Allow to reuse mark close logic via sub-class
Result:
Be able to reuse netty core classes in io_uring incubator repository
Motivation:
03aafb9cff did ensure we unregister all native methods when we unload / or fail during load of the native library. Unfortunally it missed to unregister in one case for kqueue.
Modifications:
Add unregister calls to the unload function
Result:
Correctly unregister in all cases
Motivation:
Conscrypt 2.5.1 is available and it's a good idea to upgrade to the latest version.
Modification:
Upgraded Conscrypt 2.4.0 to 2.5.1
Result:
Newer Conscrypt version.
Motivation:
FingerprintTrustManagerFactory can only use SHA-1 that is considered
insecure.
Modifications:
- Updated FingerprintTrustManagerFactory to accept a stronger hash algorithm.
- Deprecated the constructors that still use SHA-1.
- Added a test for FingerprintTrustManagerFactory.
Result:
A user can now configure FingerprintTrustManagerFactory to use a
stronger hash algorithm.
The compiler shows a warning if the code still uses one of the
unsafe constructors.
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
It's important to unload all previous registered native methods when there is a failure during loading the native lib. Failing to do so may lead to an "invalid state" and so may segfault the JVM when trying to call a native method that was previous loaded.
This was observed when two versions of netty-tcnative were on the classpath which had different requirements in terms of linking.
Something like this was reported in he hs log:
```
Instructions: (pc=0x0000000116413bf0)
0x0000000116413bd0:
[error occurred during error reporting (printing registers, top of stack, instructions near pc), id 0xb]
Register to memory mapping:
RAX=0x0000000116413bf0 is an unknown value
RBX={method} {0x000000011422e708} 'aprMajorVersion' '()I' in 'io/netty/internal/tcnative/Library'
RCX=0x000000000000000a is an unknown value
RDX=0x000000000000000a is an unknown value
```
Modifications:
- Unregister previous registered native methods on failure
- Unregister previous registered native methods on on unload of the native lib
Result:
No more segfault caused by invalid state when loading of the native lib fails in between. In this case the user will receive an error now like:
Motivation:
SLF4J 1.7.30 is the latest version in 1.7.x and we should upgrade to it from 1.7.21.
Modification:
Changed 1.7.21 to 1.7.30
Result:
Newer version of SLF4J
Motivation:
We should preferable always release the message before we notify the promise. Thhis has a few advantages:
- Release memory as soon as possible
- Listeners observe the "more correct" reference count
Modifications:
Release message before fail the promises
Result:
Faster releasing of resources. This came up in https://github.com/netty/netty/issues/10723
