Netty Future no longer extends JDK Future #11647
Merged
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
This was
linked to
issues
Sep 2, 2021
normanmaurer
requested changes
Sep 2, 2021
common/src/main/java/io/netty/util/concurrent/AbstractEventExecutor.java
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/EventExecutorGroup.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/RunnableScheduledFutureAdapter.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/UnorderedThreadPoolEventExecutor.java
Outdated
Show resolved
Hide resolved
NiteshKant
reviewed
Sep 2, 2021
common/src/main/java/io/netty/util/concurrent/AbstractEventExecutorGroup.java
Outdated
Show resolved
Hide resolved
|
@chrisvest can you please rebase ? |
Motivation: It is important to avoid blocking method calls in an event loop thread, since that can stall the system. Netty's Future interface was extending the JDK Future interface, which included a number of blocking methods of questionable use in Netty. We wish to reduce the number of blocking methods on the Future API in order to discourage their use a little. Further more, the Netty Future specification of the behaviour of the cancel() and isDone() methods are inconsistent with those of the JDK Future. If Netty's Future stop extending the JDK Future interface, it will also no longer be bound by its specification. Modification: Make Netty's Future no longer extend the JDK Future interface. Change the EvenExecutorGroup interface to no longer extend ScheduledExecutorService. The EventExecutorGroup still extends Executor, because Executor does not dictate any return type of the `execute()` method — this is also useful in the DefaultFutureCompletionStage implementation. The Netty ScheduledFuture interface has been removed since it provided no additional features that were actually used. Numerous changes to use sites that previously relied on the JDK types. Remove the `Future.cancel()` method that took a boolean argument — this argument was always ignored in our implementations, which was another spec deviation. Various `invoke*` and `shutdown*` methods have been removed from the EvenExecutorGroup API since it no longer extends ScheduledExecutorService — these were either not used anywhere, or deprecated with better alternatives available. Result: Cleaner code, leaner API. Fixes netty#7712, netty#8520
chrisvest
force-pushed
the
5x-no-jdk-future
branch
from
3e467bd
to
55cc686
Compare
|
@normanmaurer @NiteshKant Updated, please take a look. |
normanmaurer
requested changes
Sep 6, 2021
common/src/main/java/io/netty/util/concurrent/EventExecutorGroup.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public <T> Future<T> submit(Runnable task, T result) { | ||
| return (Future<T>) super.submit(task, result); | ||
| return (Future<T>) executor.submit(task, result); |
There was a problem hiding this comment.
As Future.executor() exists I think we need to take special action to ensure the returned Future actually return UnorderedThreadPoolEventExecutor and not the "inner" executor. Same is true for all the other methods.
There was a problem hiding this comment.
This is already the case because of how the inner executor decorates tasks. I have added a test for this.
|
@normanmaurer Comments addressed, please take a look. |
normanmaurer
previously requested changes
Sep 7, 2021
common/src/test/java/io/netty/util/concurrent/UnorderedThreadPoolEventExecutorTest.java
Show resolved
Hide resolved
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
NiteshKant
approved these changes
Sep 7, 2021
| * {@linkplain Thread#interrupt() interrupted}. | ||
| * @throws ExecutionException If the task failed, either by throwing an exception, or through cancellation. | ||
| * @throws TimeoutException If the future did not complete within the specified timeout. | ||
| */ | ||
| default V get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { |
There was a problem hiding this comment.
Not in this PR but wondering if we should remove blocking methods from the Future. If at all blocking is required which I believe would be for test/examples then asJdkFuture() can be used to get to blocking methods.
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:
It is important to avoid blocking method calls in an event loop thread, since that can stall the system.
Netty's Future interface was extending the JDK Future interface, which included a number of blocking methods of questionable use in Netty.
We wish to reduce the number of blocking methods on the Future API in order to discourage their use a little.
Further more, the Netty Future specification of the behaviour of the cancel() and isDone() methods are inconsistent with those of the JDK Future.
If Netty's Future stop extending the JDK Future interface, it will also no longer be bound by its specification.
Modification:
Make Netty's Future no longer extend the JDK Future interface.
Change the EvenExecutorGroup interface to no longer extend ScheduledExecutorService.
The EventExecutorGroup still extends Executor, because Executor does not dictate any return type of the
execute()method — this is also useful in the DefaultFutureCompletionStage implementation.The Netty ScheduledFuture interface has been removed since it provided no additional features that were actually used.
Numerous changes to use sites that previously relied on the JDK types.
Remove the
Future.cancel()method that took a boolean argument — this argument was always ignored in our implementations, which was another spec deviation.Various
invoke*andshutdown*methods have been removed from the EvenExecutorGroup API since it no longer extends ScheduledExecutorService — these were either not used anywhere, or deprecated with better alternatives available.Result:
Cleaner code, leaner API.
Fixes #7712, #8520