Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

method io.netty.util.concurrent.DefaultPromise#cancel/isDone violates contract? #7712

Open
laosijikaichele opened this issue Feb 13, 2018 · 4 comments · Fixed by #11647
Open

Comments

@laosijikaichele
Copy link
Contributor

laosijikaichele commented Feb 13, 2018

Expected behavior

According the contract from method java.util.concurrent.Future#cancel :

After this method returns, subsequent calls to isDone will always return true.

the console should print true

Actual behavior

console print false

Steps to reproduce

please use following reproducer code

Minimal yet complete reproducer code (or URL to code)

import io.netty.util.concurrent.GlobalEventExecutor;
import io.netty.util.concurrent.Promise;

public class DefaultPromiseIsDoneTest {

    private final Promise<?> defaultPromise = GlobalEventExecutor.INSTANCE.newPromise();

    public static void main(String args[]) {
        DefaultPromiseIsDoneTest main = new DefaultPromiseIsDoneTest();
        main.isDoneTest();
    }

    private void isDoneTest() {
        defaultPromise.setUncancellable();
        defaultPromise.cancel(false);
        boolean isDone = defaultPromise.isDone();
        System.out.println(isDone);
    }
}

Netty version

4.1.21

JVM version (e.g. java -version)

1.8

OS version (e.g. uname -a)

MacOS

Does Netty change the contract inherited from java.util.concurrent.Future#cancel?
There is a discussion about this in stackoverflow:
https://stackoverflow.com/questions/30140463/future-cancel-method-documentation

@laosijikaichele laosijikaichele changed the title method io.netty.util.concurrent.DefaultPromise#cancel violates contrast? method io.netty.util.concurrent.DefaultPromise#cancel violates contract? Feb 13, 2018
@laosijikaichele
Copy link
Contributor Author

The following methods also violate the contract:

io.netty.channel.group.VoidChannelGroupFuture#isDone
io.netty.channel.VoidChannelPromise#isDone

@laosijikaichele laosijikaichele changed the title method io.netty.util.concurrent.DefaultPromise#cancel violates contract? method io.netty.util.concurrent.DefaultPromise#cancel/isDone violates contract? Feb 13, 2018
@laosijikaichele
Copy link
Contributor Author

friendly pinging...
@normanmaurer @trustin @Scottmitch

@normanmaurer
Copy link
Member

@laosijikaichele yes I think you are right but there is not much we can do without breaking users until we do a new major release. Let me add it to the todo list.

@normanmaurer
Copy link
Member

#5430 (comment)

@chrisvest chrisvest linked a pull request Sep 2, 2021 that will close this issue
chrisvest added a commit to chrisvest/netty that referenced this issue Sep 6, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants