4.1 #1
Open
4.1 #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation:
Using the latest jython release fixes some noise that is produced by an exception that is thrown when jython is terminated.
Exception in thread "Jython-Netty-Client-4" Exception in thread "Jython-Netty-Client-7" Exception in thread "Jython-Netty-Client-5" java.lang.NoClassDefFoundError: org/python/netty/util/concurrent/DefaultPromise$2
at org.python.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:589)
at org.python.netty.util.concurrent.DefaultPromise.setSuccess(DefaultPromise.java:397)
at org.python.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:151)
at java.lang.Thread.run(Thread.java:748)
Exception in thread "Jython-Netty-Client-8" java.lang.NoClassDefFoundError: org/python/netty/util/concurrent/DefaultPromise$2
at org.python.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:589)
at org.python.netty.util.concurrent.DefaultPromise.setSuccess(DefaultPromise.java:397)
at org.python.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:151)
at java.lang.Thread.run(Thread.java:748)
Exception in thread "Jython-Netty-Client-3" java.lang.NoClassDefFoundError: org/python/netty/util/concurrent/DefaultPromise$2
at org.python.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:589)
at org.python.netty.util.concurrent.DefaultPromise.setSuccess(DefaultPromise.java:397)%
Modification:
Update to latest stable release.
Result:
Less noise during build.
Motivation: A new version was released that fixes a few test-cases to allow more close codes. Modifications: Upgrade to 0.1.5 Result: More compliant testing of websockets.
Motivation: RFC 6455 doesn't define close status codes 1012, 1013 and 1014. Yet, since then, IANA has defined them and web browsers support them. From https://www.iana.org/assignments/websocket/websocket.xhtml: * 1012: Service Restart * 1013: Try Again Later * 1014: The server was acting as a gateway or proxy and received an invalid response from the upstream server. This is similar to 502 HTTP Status Code. Modification: Make status codes 1012, 1013 and 1014 legit. Result: WebSocket status codes as defined by IANA are supported.
Motivation: A new EA build was released for Java 12. Modifications: Update to OpenJDK 12 EA24 Result: Use latest OpenJDK 12 build when building with Java 12
…etty#8665) Motivation: How we did the mapping from native code to AbstractKQueueChannel was not safe and could lead to heap corruption. This then sometimes produced ClassCastExceptions or could also lead to crashes. This happened sometimes when running the testsuite. Modifications: Use a Map for the mapping (just as we do in the native epoll transport). Result: No more heap corruption / crashes.
… th… (netty#8649) Motivation: During some other work I noticed we do not have any tests to ensure we correctly use SSLSessionBindingEvent. We should add some testing. Modifications: - Added unit test to verify we correctly implement it. - Ignore the test when using Conscrypt as it not correctly implements it. Result: More tests for custom SSL impl.
…meDecoder that they are only compatible with UTF-8 encoded streams. (netty#8651) Motivation: LineBasedFrameDecoder, JsonObjectDecoder and XmlFrameDecoder upon investigation of the sourcecode appeared to only support ASCII or UTF-8 input. It is an important characteristic and ont reflected in any documentation. This could lead to improper usage and bugs. Modifications: Javadoc comment is addedd to all three classes to state that implementation is only compatible with UTF-8 or ASCII input streams and brifly touches on implementaion details. Result: The end user of the netty library would not have to study sorcecode to deterime character encoding limitations for given classes.
netty#8666) Motivation: We should try removing all FastThreadLocals for the Thread before we notify the termination. future. The user may block on the future and once it unblocks the JVM may terminate and start unloading classes. Modifications: Remove all FastThreadLocals for the Thread before notify termination future. Result: Fixes netty#6596.
Motivation: Users who want to construct a `FlushConsolidationHandler` with a default `explicitFlushAfterFlushes` but non-default `consolidateWhenNoReadInProgress` may benefit from having an easy way to get the default "flush after flushes" count. Modifications: - Moved default `explicitFlushAfterFlushes` value to a public constant. - Adjusted Javadoc accordingly. Result: Default `explicitFlushAfterFlushes` is accessible to callers.
…ty#8646) Motiviation: Http2FrameCodecTest and Http2MultiplexCodecTest were quite fragile and often not went through the whole pipeline which made testing sometimes hard and error-prone. Modification: - Refactor tests to have data flow through the whole pipeline and so made the test more robust (by testing the while implementation). Result: Easier to write tests for the codecs in the future and more robust testing in general. Beside this it also fixes netty#6036.
Motivation: Java uses camel-case by convention. Modification: Consistently use camel-case. Result: More consistent code styling.
Motivation: Javadocs contained some spelling errors, we should fix these. Modification: Fix spelling Result: Javadoc cleanup.
Motivation: We do not correctly detect loops when follow CNAMEs and so may try to follow it without any success. Modifications: - Correctly detect CNAME loops - Do not cache CNAME entries which point to itself - Add unit test. Result: Fixes netty#8687.
…adding the handler to the pipeline. (netty#8684) Motivation: Due a race in DefaultChannelPipeline / AbstractChannelHandlerContext it was possible to have only handlerRemoved(...) called during tearing down the pipeline, even when handlerAdded(...) was never called. We need to ensure we either call both of none to guarantee a proper lifecycle of the handler. Modifications: - Enforce handlerAdded(...) / handlerRemoved(...) semantics / ordering - Add unit test. Result: Fixes netty#8676 / netty#6536 .
Motivation: In Http2FrameCodec we made the incorrect assumption that we can only have 1 buffered outboundstream as maximum. This is not correct and we need to account for multiple buffered streams. Modifications: - Use a map to allow buffer multiple streams - Add unit test. Result: Fixes netty#8692.
Motivation: A new EA release was done for OpenJDK12. Modifications: Use OpenJDK12 EA 27 when running CI jobs for JDK 12. Result: Test against latest OpenJDK 12 EA build.
Motivation: The javadocs stating `IndexOutOfBoundsException` is thrown were different from what `ByteBuf` actually did. We want to ensure the Javadocs represent reality. Modifications: Updated javadocs on `write*`, `ensureWriteable`, `capacity`, and `maxCapacity` methods. Results: Javadocs more closely match actual behaviour.
Motivation: Incrementing two variables in sync is not necessary when only one will do. Modifications: - Remove `j` from `for` loop and replace with `i`. - Add more unit testing scenarios to cover changed code. Results: Unnecessary variable removed.
…eChannelFactory. (netty#8718) Motivation: We should access the Constructor of the passed in class in the Constructor of ReflectiveChannelFactory only to reduce the overhead but also fail-fast. Modifications: Access the Constructor early. Result: Fails fast and less performance overhead.
Motivation: Clean up code to increase readability. Modification: Extract duplicate code blocks into method. Result: Less code duplication
Motivation:
Avoid unnecessary iteration and `ArrayList` allocation.
Modification:
```
for (AbstractEpollChannel channel: channels.values()) {
array.add(channel);
}
```
replaced with
`array.addAll(channels.values())`
and
```
Collection<AbstractEpollChannel> array = new ArrayList<AbstractEpollChannel>(channels.size());
array.addAll(channels.values())
```
replaced with:
`AbstractEpollChannel[] localChannels = channels.values().toArray(new AbstractEpollChannel[0]);`
Result:
Simpler code in `EpollEventLoop.closeAll();`
Motivation: ChunkedWriteHandler should report write operation as failed in case *any* chunked was not written. Right now this is not true for the last chunk. Modifications: * Check if the appropriate write operation was succesfull when reporting the last chunk * Skip writing chunks if the write operation was already marked as "done" * Test cases to cover write failures when dealing with chunked input Result: Fix netty#8700
Motivation: There are the first EA bulds for OpenJDK 13. We should support to build with it and run builds on the CI. Modifications: - Add profile for JDK 13 - Add docker config to run with JDK 13. Result: Building and testing with OpenJDK 13 is possible.
Motivation: The CorruptedFrameException from the finish() method of the Utf8Validator gets propagated to other handlers while the connection is still open. Modification: Override exceptionCaught method of the Utf8FrameValidator and close the connection if it is a CorruptedFrameException. Result: The CorruptedFrameException gets propagated to other handlers only after properly closing the connection.
Motivation: We should always build with the latest JDK releases. Modifications: Update JDK8 and JDK11 versions to the latest. Result: Run CI jobs on the latest JDK release.
Motivation: Clean up code to increase readability. Modification: Extract duplicate code and remove unnecessary throws Result: Share more code.
Motivation: Since the updating to OpenJDK 11.0.2 the OSGI testsuite fails. We should dissable it until there is a version of the used plugins that works with this OpenJDK version. Modifications: Skip osgi testsuite when using JDK11. Result: Build pass again with JDK11.
…other nameservers are left. (netty#8731) Motivation: When using multiple nameservers and a nameserver respond with NXDOMAIN we should only fail the query if the nameserver in question is authoritive or no nameservers are left to try. Modifications: - Try next nameserver if NXDOMAIN was returned but the nameserver is not authoritive - Adjust testcase to respect correct behaviour. Result: Fixes netty#8261
netty#8726) Motivation: When a write error happens during writing of flowcontrolled data frames we miss to correctly detect this in the write loop which may result in an infinite loop as we will never detect that the frame should be removed from the queue. Modifications: - When we fail a flowcontrolled data frame we ensure that the next frame.write(...) call will signal back that the whole frame was handled and so can be removed. - Add unit test. Result: Fixes netty#8707.
Motivation: testWriteTaskRejected was racy as we did not ensure we dispatched all events to the executor before shutting it down. Modifications: Add a latch to ensure we dispatched everything. Result: Fix racy test that failed sometimes before.
) Motivation: f17bfd0 removed the usage of static exception instances to reduce the risk of OOME due addSupressed calls. We should do the same for exceptions used to signal handshake timeouts. Modifications: Do not use static instances Result: No risk of OOME due addSuppressed calls
… block or loopback-mode-disabled operations). Motivation: Provide epoll/native multicast to support high load multicast users (we are using it for a high load telecomm app at my day job). Modification: Added support for source specific and any source multicast for epoll transport. Some caveats: no support for disabling loop back mode, retrieval of interface and block operation, all of which tend to be less frequently used. Result: Provides epoll transport multicast for common use cases. Co-authored-by: Norman Maurer <norman_maurer@apple.com>
…created with existing FileDescriptor (netty#9185) Motivation: When EpollDatagramChannel is created with an existing FileDescriptor we should detect the correct InternetProtocolFamily. Modifications: Obtain the InternetProtocolFamily from the given FD Result: Use correct InternetProtocolFamily when EpollDatagramChannel is created via existing FileDescriptor
…impls (netty#9119) Motivation There are a few minor inconsistencies / redundant operations in the ByteBuf implementations which would be good to fix. Modifications - Unnecessary ByteBuffer.duplicate() performed in CompositeByteBuf.nioBuffer(int,int) - Add missing checkIndex(...) check to ReadOnlyByteBufferBuf.nioBuffer(int,int) - Remove duplicate bounds check in ReadOnlyByteBufferBuf.getBytes(int,byte[],int,int) - Omit redundant bounds check in UnpooledHeapByteBuf.getBytes(int,ByteBuffer) Result More consistency and slightly less overhead
… upgrade request (netty#9177) Motivation: The io.netty.example.http2.helloworld.client.Http2Client example should work in the h2c (HTTP2 cleartext - non-TLS) mode, which is the default for this example unless you set a -Dssl VM param. As we do not set the HOST header some servers do reject the upgrade request. Modifications: Set the HOST header Result: Fixes netty#9115.
…etty#9081) Motivation A Semaphore is currently dedicated to this purpose but a simple CountDownLatch will do. Modification Remove private threadLock Semaphore from SingleThreadEventExecutor and just use a CountDownLatch. Also eliminate use of PlatformDependent.throwException() in startThread method, and combine some nested if clauses. Result Cleaner EventLoop termination notification.
…ePayload (netty#9202) Motivation: The toString() methods of MqttSubscribePayload and MqttUnsubscribePayload are causing exceptions when no topics are set. Modification: The toString() methods will not throw Excpetions anymore. Result: Fixes netty#9197
Motivation: We do not need to issue a read on timerfd and eventfd when the EventLoop wakes up if we register these as Edge-Triggered. This removes the overhead of 2 syscalls and so helps to reduce latency. Modifications: - Ensure we register the timerfd and eventfd with EPOLLET flag - If eventfd_write fails with EAGAIN, call eventfd_read and try eventfd_write again as we only use it as wake-up mechanism. Result: Less syscalls and so reducing overhead. Co-authored-by: Carl Mastrangelo <carl@carlmastrangelo.com>
Motivation: 1. Users will be able to use an optimized version of `UnpooledHeapByteBuf` and override behavior of methods if required. 2. Consistency with `UnpooledDirectByteBuf`, `UnpooledHeapByteBuf`, and `UnpooledUnsafeDirectByteBuf`. Modifications: - Add `public` access modifier to `UnpooledUnsafeHeapByteBuf` class and ctor; Result: Public access for optimized version of `UnpooledHeapByteBuf`.
…ReadComplete() if fireChannelRead(...) was called before (netty#9211) Motivation: At the moment ByteToMessageDecoder always calls fireChannelReadComplete() when the handler is removed from the pipeline and the cumulation buffer is not null. We should only call it when we also call fireChannelRead(...), which only happens if the cumulation buffer is not null and readable. Modifications: Only call fireChannelReadComplete() if fireChannelRead(...) is called before during removal of the handler. Result: More correct semantics
Motivation: It is valid to use null as sender so we should support it when DatagramPacketEncoder checks if it supports the message. Modifications: - Add null check - Add unit test Result: Fixes netty#9199.
…#9085) Motivation While digging around looking at something else I noticed that these share a lot of logic and it would be nice to reduce that duplication. Modifications Have UnpooledUnsafeDirectByteBuf extend UnpooledDirectByteBuf and make adjustments to ensure existing behaviour remains unchanged. The most significant addition needed to UnpooledUnsafeDirectByteBuf was re-overriding the getPrimitive/setPrimitive methods to revert back to the AbstractByteBuf versions which include bounds checks (UnpooledDirectByteBuf excludes these as an optimization, relying on those done by underlying ByteBuffer). Result ~200 fewer lines, less duplicate logic.
Motivation: RoundRobinDnsAddressResolverGroup ultimately opens UDP ports for DNS resolution. Callers likely expect that RoundRobinDnsAddressResolverGroup#close() will close those ports, but that is not currently true (see netty#9212). Modifications: Overrode RoundRobinInetAddressResolver#close() to close the delegate name resolver, which in turn closes any UDP ports used for name resolution. Result: RoundRobinDnsAddressResolverGroup#close() closes UDP ports as expected. This fixes netty#9212.
…s. (netty#9219) Motivation: This resolves a TODO from the initial transport-native-kqueue implementation, supplying the user with the pid of the local peer client/server process. Modification: Inside netty_kqueue_bsdsocket_getPeerCredentials, Call getsockopt with LOCAL_PEERPID and pass it to PeerCredentials constructor. Add a test case in KQueueSocketTest. Result: PeerCredentials now have pid field set. Fixes netty#9213
Motivation: The wakeup logic in EpollEventLoop is overly complex Modification: * Simplify the race to wakeup the loop * Dont let the event loop wake up itself (it's already awake!) * Make event loop check if there are any more tasks after preparing to sleep. There is small window where the non-eventloop writers can issue eventfd writes here, but that is okay. Result: Cleaner wakeup logic. Benchmarks: ``` BEFORE Benchmark Mode Cnt Score Error Units EpollSocketChannelBenchmark.executeMulti thrpt 20 408381.411 ± 2857.498 ops/s EpollSocketChannelBenchmark.executeSingle thrpt 20 157022.360 ± 1240.573 ops/s EpollSocketChannelBenchmark.pingPong thrpt 20 60571.704 ± 331.125 ops/s Benchmark Mode Cnt Score Error Units EpollSocketChannelBenchmark.executeMulti thrpt 20 440546.953 ± 1652.823 ops/s EpollSocketChannelBenchmark.executeSingle thrpt 20 168114.751 ± 1176.609 ops/s EpollSocketChannelBenchmark.pingPong thrpt 20 61231.878 ± 520.108 ops/s ```
Motivation: b4e3c12 introduced code to avoid coupling close() to graceful close. It also added some code which attempted to infer when a graceful close was being done in writing of a GOAWAY to preserve the "connection is closed when all streams are closed behavior" for the child channel API. However the implementation was too overzealous and may preemptively close the connection if there are not currently any open streams (and close if there are any frames which create streams in flight). Modifications: - Decouple writing a GOAWAY from trying to infer if a graceful close is being done and closing the connection. Even if we could enhance this logic (e.g. wait to close until the second GOAWAY with no error) it is possible the user doesn't want the connection to be closed yet. We can add a means for the codec to orchestrate the graceful close in the future (e.g. write some special "close the connection when all streams are closed") but for now we can just let the application handle this. Result: Fixes netty#9207
Motivation: When Netty is run through ProGuard, seemingly unused methods are removed. This breaks reflection, making the Handler skipping throw a reflective error. Modification: If a method is seemingly absent, just disable the optimization. Result: Dealing with ProGuard sucks infinitesimally less.
…hen using epoll native transport with multicast (netty#9218) Motivation: We did not have support for enable / disable loopback mode in our native epoll transport and also missed the implemention to access the configured interface. Modifications: Add implementation and adjust test to cover it Result: More complete multicast support with native epoll transport
delete Other "Content-" MIME Header Fields exception Motivation: RFC7578 4.8. Other "Content-" Header Fields The multipart/form-data media type does not support any MIME header fields in parts other than Content-Type, Content-Disposition, and (in limited circumstances) Content-Transfer-Encoding. Other header fields MUST NOT be included and MUST be ignored. Modification: Ignore other Content types. Result: Other "Content-" Header Fields should be ignored no exception
…9205 (netty#9206) Motivation: When connecting through an HTTP proxy over clear HTTP, user agents must send requests with an absolute url. This hold true for WebSocket Upgrade request. WebSocketClientHandshaker and subclasses currently always send requests with a relative url, which causes proxies to crash as request is malformed. Modification: Introduce a new parameter `absoluteUpgradeUrl` and expose it in constructors and WebSocketClientHandshakerFactory. Result: It's now possible to configure WebSocketClientHandshaker so it works properly with HTTP proxies over clear HTTP.
…9230) Motivation: Sometimes it is beneficial to be able to set a parent Channel in EmbeddedChannel if the handler that should be tested depend on the parent. Modifications: - Add another constructor which allows to specify a parent - Add unit tests Result: Fixes netty#9228.
Motivation: In the current implementation, the synchronous close() method for FixedChannelPool returns after scheduling the channels to close via a single threaded executor asynchronously. Closing a channel requires event loop group, however, there might be a scenario when the application has closed the event loop group after the sync close() completes. In this scenario an exception is thrown (event loop rejected the execution) when the single threaded executor tries to close the channel. Modifications: Complete the close function only after all the channels have been close and introduce closeAsync() method for cases when the current/existing behaviour is desired. Result: Close function would completely when the channels have been closed
Motivation: Because of a simple bug in ByteBufChecksum#updateByteBuffer(Checksum), ReflectiveByteBufChecksum is never used for CRC32 and Adler32, resulting in direct ByteBuffers being checksummed byte by byte, which is undesriable. Modification: Fix ByteBufChecksum#updateByteBuffer(Checksum) method to pass the correct argument to Method#invoke(Checksum, ByteBuffer). Result: ReflectiveByteBufChecksum will now be used for Adler32 and CRC32 on Java8+ and direct ByteBuffers will no longer be checksummed on slow byte-by-byte basis.
…ite fails (netty#9240) Motivation: SslHandler must generate control data as part of the TLS protocol, for example to do handshakes. SslHandler doesn't capture the status of the future corresponding to the writes when writing this control (aka non-application data). If there is another handler before the SslHandler that wants to fail these writes the SslHandler will not detect the failure and we must wait until the handshake timeout to detect a failure. Modifications: - SslHandler should detect if non application writes fail, tear down the channel, and clean up any pending state. Result: SslHandler detects non application write failures and cleans up immediately.
Motivation: ReflectiveByteBufChecksum#update(buf, off, len) ignores provided offset and length arguments when operating on direct buffers, leading to wrong byte sequences being checksummed and ultimately incorrect checksum values (unless checksumming the entire buffer). Modifications: Use the provided offset and length arguments to get the correct nio buffer to checksum; add test coverage exercising the four meaningfully different offset and length combinations. Result: Offset and length are respected and a correct checksum gets calculated; simple unit test should prevent regressions in the future.
Motivation: Lz4FrameEncoder and Lz4FrameDecoder in their default configuration use an extremely inefficient way to checksum direct byte buffers. In particular, for every byte checksummed, a single-element byte array is being allocated and a JNI cal is made, which in some internal testing makes a 25x difference in total throughput and allocates *a lot* of garbage. Modifications: Lz4XXHash32, an implementation of ByteBufChecksum specifically for use by Lz4FrameEncoder and Lz4FrameDecoder, is introduced. It utilises xxHash32 block API which provides a hash() method that accepts a ByteBuffer as an argument. Lz4FrameEncoder and Lz4FrameDecoder are modified to use this implementation by default. Result: Lz4FrameEncoder and Lz4FrameDecoder perform well again when operating on direct byte buffers with default checksum configuration; a public implementation is provided for those who need to override the seed.
Motivation: Traffic shaping needs more accurate execution than scheduled one. So the use of FixedRate instead. Moreover the current implementation tends to create as many threads as channels use a ChannelTrafficShapingHandlern, which is unnecessary. Modifications: Change the executor.schedule to executor.scheduleAtFixedRate in the start and remove the reschedule call from run monitor thread since it will be restarted by the Fixed rate executor. Also fix a minor bug where restart was only doing start() without stop() before. Result: Threads are more stable in number of cached and precision of traffic shaping is enhanced.
…e flow-controller (netty#9235) Motivation: We should decouple the writability state of the http2 child channels from the flow-controller and just tie it to its own pending bytes counter that is decremented by the parent Channel once the bytes were written. Modifications: - Decouple writability state of child channels from flow-contoller - Update tests Result: Less coupling and more correct behavior. Fixes netty#8148.
Motivation: Incorrect WebSockets closure affects our production system. Enforced 'close socket on any protocol violation' prevents our custom termination sequence from execution. Huge number of parameters is a nightmare both in usage and in support (decoders configuration). Modification: Fix violations handling - send proper response codes. Fix for messages leak. Introduce decoder's option to disable default behavior (send close frame) on protocol violations. Encapsulate WebSocket response codes - WebSocketCloseStatus. Encapsulate decoder's configuration into a separate class - WebSocketDecoderConfig. Result: Fixes netty#8295.
…mes writable (netty#9254) Motivation: f945a07 decoupled the writability state from the flow controller but could lead to the situation of a lot of writability updates events were propagated to the child channels. This change ensure we only take into account if the parent channel becomes writable again before we try to set the child channels to writable. Modifications: Only listen for channel writability changes for if the parent channel becomes writable again. Result: Less writability updates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Explain here the context, and why you're making that change.
What is the problem you're trying to solve.
Modification:
Describe the modifications you've done.
Result:
Fixes #.
If there is no issue then describe the changes introduced by this PR.