Motivation:
https://github.com/netty/netty/pull/6042 only addressed PlatformDependent#getSystemClassLoader but getClassLoader is also called in an optional manner in some common code paths but fails to catch a general enough exception to continue working.
Modifications:
- Calls to getClassLoader which can continue if results fail should catch Throwable
Result:
More resilient code in the presense of restrictive class loaders.
Fixes https://github.com/netty/netty/issues/6246.
Motivation:
We not warned about not-supported ChannelOptions when set the options for the ServerChannel.
Modifications:
- Share code for setting ChannelOptions during bootstrap
Result:
Warning is logged when a ChannelOption is used that is not supported during bootstrap a Channel. See also [#6192]
Motivation:
The comment on AbstractChannelHandlerContext.invokeHandler() is incorrect and missleading. See [#6177]
Modifications:
Change true to false to correct the comment.
Result:
Fix missleading and incorrect comment.
Motivation:
`SimpleChannelPool` subclasses are likely to override the `connectChannel` method, and are likely to clobber the cloned `Bootstrap` handler in the process. To allow subclasses to properly notify the pool listener of new connections, we should expose (at least) the `handler` property of the pool to subclasses.
Modifications:
Expose `SimpleChannelPool` properties to subclasses via `protected` getters.
Result:
Subclasses can now use the bootstrap, handler, health checker, and health-check-on-release preoperties from their superclass.
Motivation:
DefaultChannelId provides a regular expression which validates if a user provided MAC address is valid. This regular expression may allow invalid MAC addresses and also not allow valid MAC addresses.
Modifications:
- Introduce a MacAddressUtil#parseMac method which can parse and validate the MAC address at the same time. The regular expression check before hand is additional overhead if we have to parse the MAC address.
Result:
Fixes https://github.com/netty/netty/issues/6132.
Motivation:
On some platforms the PID my be bigger then 4194304 so we should not limit it to 4194304.
Modifications:
Only check that the PID is a valid Integer
Result:
No more warnings on systems where the PID is bigger then 4194304.
Motivation:
In later Java8 versions our Atomic*FieldUpdater are slower then the JDK implementations so we should not use ours anymore. Even worse the JDK implementations provide for example an optimized version of addAndGet(...) using intrinsics which makes it a lot faster for this use-case.
Modifications:
- Remove methods that return our own Atomic*FieldUpdaters.
- Use the JDK implementations everywhere.
Result:
Faster code.
Motivation:
e102a008b6 changed a conditional where previously the NIO ServerChannel would not be closed in the event of an exception.
Modifications:
- Restore the logic prior to e102a008b6 which does not automatically close ServerChannels for IOExceptions
Result:
NIO ServerChannel doesn't close automatically for an IOException.
Motivation:
We should not catch ConcurrentModificationException as this can never happen because things are executed on the EventLoop thread.
Modifications:
Remove try / catch
Result:
Cleaner code.
Modifications:
LocalChannel#releaseInboundBuffers should always clear/release the queue and set readInProgress to false
Result:
LocalChannel queue is more reliably cleaned up.
Motivation:
LocalChannel attempts to close its peer socket when ever it is closed. However if the channels are on different EventLoops we may attempt to process events for the peer channel on the wrong EventLoop.
Modifications:
- Ensure the close process ensures we are on the correct thread before accessing data
Result:
More correct LocalChannel close code.
Motivation:
PlatformDependent#getSystemClassLoader may throw a wide variety of exceptions based upon the environment. We should handle all exceptions and continue initializing the slow path if an exception occurs.
Modifications:
- Catch Throwable in cases where PlatformDependent#getSystemClassLoader is used
Result:
Fixes https://github.com/netty/netty/issues/6038
Motivation:
Netty provides a adaptor from ByteBuf to Java's InputStream interface. The JDK Stream interfaces have an explicit lifetime because they implement the Closable interface. This lifetime may be differnt than the ByteBuf which is wrapped, and controlled by the interface which accepts the JDK Stream. However Netty's ByteBufInputStream currently does not take reference count ownership of the underlying ByteBuf. There may be no way for existing classes which only accept the InputStream interface to communicate when they are done with the stream, other than calling close(). This means that when the stream is closed it may be appropriate to release the underlying ByteBuf, as the ownership of the underlying ByteBuf resource may be transferred to the Java Stream.
Motivation:
- ByteBufInputStream.close() supports taking reference count ownership of the underyling ByteBuf
Result:
ByteBufInputStream can assume reference count ownership so the underlying ByteBuf can be cleaned up when the stream is closed.
Motivation:
To guard against the case that a user will enqueue a lot of empty or small buffers and so raise an OOME we need to also take the overhead of the ChannelOutboundBuffer / PendingWriteQueue into account when detect if a Channel is writable or not. This is related to #5856.
Modifications:
When calculate the memory for an message that is enqueued also add some extra bytes depending on the implementation.
Result:
Better guard against OOME.
Motivation
It's possible to extend LocalChannel as well as LocalServerChannel but the LocalServerChannel's serve(peer) method is hardcoded to create only instances of LocalChannel.
Modifications
Add a protected factory method that returns by default new LocalChannel(...) but users may override it to customize it.
Result
It's possible to customize the LocalChannel instance on either end of the virtual connection.
Motivation:
Some unit tests in SingleThreadEventLoopTest rely upon Thread.sleep for sequencing events between threads. This can be unreliable and result in spurious test failures if thread scheduling does not occur in a fair predictable manner.
Modifications:
- Reduce the reliance on Thread.sleep in SingleThreadEventLoopTest
Result:
Fixes https://github.com/netty/netty/issues/5851
Motivation:
The local transport is used to communicate in the same JVM so we should use heap buffers.
Modifications:
Use heapbuffers by default if not requested otherwise.
Result:
No allocating of direct buffers by default when using local transport
Motivation:
When using java.nio.DatagramChannel we should not close the channel when a SocketException was thrown as we can still use the channel.
Modifications:
Not close the Channel when SocketException is thrown
Result:
More robust and correct handling of exceptions when using NioDatagramChannel.
Motivation:
If an exception is thrown while processing the ready channels in the EventLoop we should still run all tasks as this may allow to recover. For example a OutOfMemoryError may be thrown and runAllTasks() will free up memory again. Beside this we should also ensure we always allow to shutdown even if an exception was thrown.
Modifications:
- Call runAllTasks() in a finally block
- Ensure shutdown is always handles.
Result:
More robust EventLoop implementations for NIO and Epoll.
Motivation:
We should better first process OP_WRITE before OP_READ as this may allow us to free memory in a faster fashion for previous queued writes.
Modifications:
Process OP_WRITE before OP_READ
Result:
Free memory faster for queued writes.
Motivation:
the build doesnt seem to enforce this, so they piled up
Modifications:
removed unused import lines
Result:
less unused imports
Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
the implicit #fireChannelReadComplete() in EmbeddedChannel#writeInbound().
Motivation
We use EmbeddedChannels to implement a ProxyChannel of some sorts that shovels
messages between a source and a destination Channel. The latter are real network
channels (such as Epoll) and they may or may not be managed in a ChannelPool. We
could fuse both ends directly together but the EmbeddedChannel provides a nice
disposable section of a ChannelPipeline that can be used to instrument the messages
that are passing through the proxy portion.
The ideal flow looks abount like this:
source#channelRead() -> proxy#writeOutbound() -> destination#write()
source#channelReadComplete() -> proxy#flushOutbound() -> destination#flush()
destination#channelRead() -> proxy#writeInbound() -> source#write()
destination#channelReadComplete() -> proxy#flushInbound() -> source#flush()
The problem is that #writeOutbound() and #writeInbound() emit surplus #flush()
and #fireChannelReadComplete() events which in turn yield to surplus #flush()
calls on both ends of the pipeline.
Modifications
Introduce a new set of write methods that reain the same sematics as the #write()
method and #flushOutbound() and #flushInbound().
Result
It's possible to implement the above ideal flow.
Fix for EmbeddedChannel#ensureOpen() and Unit Tests for it
Some PR stuff.
Motivation:
To make it easier to debug why notification of a promise failed we should log extra info and make it consistent.
Modifications:
- Create a new PromiseNotificationUtil that has static methods that can be used to try notify a promise and log.
- Reuse this in AbstractChannelHandlerContext, ChannelOutboundBuffer and PromiseNotifier
Result:
Easier to debug why a promise could not be notified.
Motivation:
RFC7871 defines an extension which allows to request responses for a given subset.
Modifications:
- Add DnsOptPseudoRrRecord which can act as base class for extensions based on EDNS(0) as defined in RFC6891
- Add DnsOptEcsRecord to support the Client Subnet in DNS Queries extension
- Add tests
Result:
Client Subnet in DNS Queries extension is now supported.
Motivation:
For use cases that demand frequent updates of the write watermarks, an
API that requires immutable WriteWaterMark objects is not ideal, as it
implies a lot of object allocation.
For example, the HTTP/2 child channel API uses write watermarks for outbound
flow control and updates the write watermarks on every DATA frame write.
Modifications:
Remote @Deprecated tag from primitive getters and setters, however the corresponding
channel options remain deprecated.
Result:
Primitive getters and setters for write watermarks are no longer marked @Deprecated.
Motivation:
The JDK implementation of SocketChannel has an internal state that is tracked for its operations. Because of this we need to ensure we call finishConnect() before try to call read(...) / write(...) as otherwise it may produce a NotYetConnectedException.
Modifications:
First process OP_CONNECT flag.
Result:
No more possibility of NotYetConnectedException because OP_CONNECT is handled not early enough when processing interestedOps for a Channel.
Motivation:
The DefaultEventLoopGroup class extends MultithreadEventExecutorGroup but doesn't expose the ctor variants that accept a custom Executor like NioEventLoopGroup and EpollEventLoopGroup do.
Modifications:
Add missing constructor.
Result:
Be able to use custom Executor with DefaultEventLoopGroup.
Motivation:
When attempting to set the selectedKeys fields on the selector
implementation, JDK 9 can throw an inaccessible object exception.
Modications:
Catch and log this exception as an possible course of action if the
sun.nio.ch package is not exported from java.base.
Result:
The selector replacement will fail gracefully as an expected course of
action if the sun.nio.ch package is not exported from java.base.
Motivation:
The NIO transport used an IllegalStateException if a user tried to issue another connect(...) while the connect was still in process. For this case the JDK specified a ConnectPendingException which we should use. The same issues exists in the EPOLL transport. Beside this the EPOLL transport also does not throw the right exceptions for ENETUNREACH and EISCONN errno codes.
Modifications:
- Replace IllegalStateException with ConnectPendingException in NIO and EPOLL transport
- throw correct exceptions for ENETUNREACH and EISCONN in EPOLL transport
- Add test case
Result:
More correct error handling for connect attempts when using NIO and EPOLL transport
Motivation:
The API documentation in ChannelConfig states that a a channel is writable,
if the number of pending bytes is below the low watermark and a
channel is not writable, if the number of pending bytes exceeds the high
watermark.
Therefore, we should use < operators instead of <= as well as > instead of >=.
Using <= and >= is also problematic, if the low watermark is equal to the high watermark,
as then a channel could be both writable and unwritable with the same number of pending
bytes (depending on whether remove() or addMessage() is called first).
The use of <= and >= was introduced in PR https://github.com/netty/netty/pull/3036, but
I don't understand why, as there doesn't seem to have been any discussion around that.
Modifications:
Use < and > operators instead of <= and >=.
Result:
High and low watermarks are treated as stated in the API docs.
Motivation:
We need to ensure we also call fireChannelActive() if the Channel is directly closed in a ChannelFutureListener that is belongs to the promise for the connect. Otherwise we will see missing active events.
Modifications:
Ensure we always call fireChannelActive() if the Channel was active.
Result:
No missing events.
Motivation:
We use often javachannel().socket().* in NIO as these methods exists in java6. The problem is that these will throw often very general Exceptions (Like SocketException) while it is more expected to throw the Exceptions listed in the nio interfaces. When possible we should use the new methods available in java7+ which throw the correct exceptions.
Modifications:
Check for java version and depending on it using the socket or the javachannel.
Result:
Throw expected Exceptions.
Motivation:
To make it easier to debug connect exceptions we create new exceptions which also contain the remote address. For this we basically created a new instance and call setStackTrace(...). When doing this we pay an extra penality because it calls fillInStackTrace() when calling the super constructor.
Modifications:
Create special sub-classes of Exceptions that override the fillInStackTrace() method and so eliminate the overhead.
Result:
Less overhead when "annotate" connect exceptions.
Motivation:
Comments stating that AUTO_CLOSE will be removed in Netty 5.0 are wrong,
as there is no Netty 5.0.
Modifications:
Removed comment.
Result:
No more references to Netty 5.0
Motivation:
PendingWriteQueue should guard against re-entrant writes once removeAndWriteAll() is run.
Modifications:
Continue writing until queue is empty.
Result:
Correctly guard against re-entrance.
Motivation:
Instrumenting the NIO selector implementation requires special
permissions. Yet, the code for performing this instrumentation is
executed in a manner that would require all code leading up to the
initialization to have the requisite permissions. In a restrictive
environment (e.g., under a security policy that only grants the
requisite permissions the Netty transport jar but not to application
code triggering the Netty initialization), then instrumeting the
selector will not succeed even if the security policy would otherwise
permit it.
Modifications:
This commit marks the necessary blocks as privileged. This enables
access to the necessary resources for instrumenting the selector. The
idea is that we are saying the Netty code is trusted, and as long as the
Netty code has been granted the necessary permissions, then we will
allow the caller access to these resources even though the caller itself
might not have the requisite permissions.
Result:
The selector can be instrumented in a restrictive security environment.
Motivation:
Writing to a system property requires permissions. Yet the code for
setting sun.nio.ch.bugLevel is not marked as privileged. In a
restrictive environment (e.g., under a security policy that only grants
the requisite permissions the Netty transport jar but not to application
code triggering the Netty initialization), writing to this system
property will not succeed even if the security policy would otherwise
permit it.
Modifications:
This commt marks the necessary code block as privileged. This enables
writing to this system property. The idea is that we are saying the
Netty code is trusted, and as long as the Netty code has been granted
the necessary permissions, then we will allow the caller access to these
resources even though the caller itself might not have the requisite
permissions.
Result:
The system property sun.nio.ch.bugLevel can be written to in a
restrictive security environment.
Motivation:
If the user uses 0 as quiet period we should shutdown without any delay if possible.
Modifications:
Ensure we not introduce extra delay when a shutdown quit period of 0 is used.
Result:
EventLoop shutdown as fast as expected.
Motivation:
At the moment we call initChannel(...) in the channelRegistered(...) method which has the effect that if another ChannelInitializer is added within the initChannel(...) method the ordering of the added handlers is not correct and surprising. This is as the whole initChannel(...) method block is executed before the initChannel(...) block of the added ChannelInitializer is handled.
Modifications:
Call initChannel(...) from within handlerAdded(...) if the Channel is registered already. This is true in all cases for our DefaultChannelPipeline implementation. This way the ordering is always as expected. We still keep the old behaviour as well to not break code for other ChannelPipeline implementations (if someone ever wrote one).
Result:
Correct and expected ordering of ChannelHandlers.
Motivation:
When we try to close the Channel due a timeout we need to ensure we not log if the notification of the promise fails as it may be completed in the meantime.
Modifications:
Add another constructor to ChannelPromiseNotifier and PromiseNotifier which allows to log on notification failure.
Result:
No more miss-leading logs.
Motivation:
I received a report the its not possible to add another ChannelInitialiter in the initChannel(...) method, so we should add a test case for it.
Modifications:
Added testcase.
Result:
Validate that all works as expected.
Motivation:
When a ChannelInitializer is used via ServerBootstrap.handler(...) the users handlers may be added after the internal ServerBootstrapAcceptor. This should not happen.
Modifications:
Delay the adding of the ServerBootstrapAcceptor until the initChannel(....) method returns.
Result:
Correct order of handlers in the ServerChannels ChannelPipeline.
Motivation:
We used Promise.setFailure(...) when fail a Promise in SimpleChannelPool. As this happens in multiple levels this can result in stackoverflow as setFailure(...) may throw an IllegalStateException which then again is propergated.
Modifications:
Use tryFailure(...)
Result:
No more possibility to cause a stack overflow when failing the promise.
Motivation:
The SimpleChannelPool#notifyConnect() method will leak Channels if the user cancelled the Promise in between.
Modifications:
Release the channel if the Promise was complete before.
Result:
No more channel leaks.
Motiviation:
DefaultChannelId attempts to acquire a default process ID by determining
the process PID. However, to do this it attempts to punch through to the
system classloader, a permission that in the face of a restrictive
security manager is unlikely to be granted. Looking past this, it then
attempts to load a declared method off a reflectively loaded class,
another permission that is not likely to be granted in the face of a
restrictive security manager. However, neither of these permissions are
necessary as the punching through to the system security manager is
completely unneeded, and there is no need to load a public method as a
declared method.
Modifications:
Instead of punching through to the system classloader requiring
restricted permissions, we can just use current classloader. To address
the access declared method permission, we instead just reflectively
obtain the desired public method via Class#getMethod.
Result:
Acquiring the default process ID from the PID will succeed without
requiring the runtime permissions "getClassLoader" and
"accessDeclaredMembers".
