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

StackOverflowError when adding listener to DefaultPromise w/ ImmediateEventExecutor #4395

Closed
maxwindiff opened this issue Oct 26, 2015 · 6 comments
Assignees
Milestone

Comments

@maxwindiff
Copy link

This will produce a StackOverflowError:

    @Test
    public void testName() throws Exception {
        DefaultPromise<Object> promise = new DefaultPromise<>(ImmediateEventExecutor.INSTANCE);
        promise.addListener(f1 -> {
            promise.addListener(f2 -> System.out.println("done"));
        });
        promise.tryFailure(new Exception());
    }

The error may not show up in console but if you add an exception breakpoint in IntelliJ / Eclipse you'll see it.

Reproduces on 4.1.0.Beta7

@Scottmitch
Copy link
Member

thanks for reporting @maxwindiff!

@Scottmitch Scottmitch self-assigned this Oct 27, 2015
@Scottmitch Scottmitch added this to the 4.0.33.Final milestone Oct 27, 2015
Scottmitch added a commit to Scottmitch/netty that referenced this issue Oct 29, 2015
Motivation:
When the ImmediateEventExecutor is in use it is possible to get a StackOverFlowException if when a promise completes a new listener is added to that promise.

Modifications:
- Protect against the case where LateListeners.run() smashes the stack.

Result:
Fixes netty#4395
Scottmitch added a commit that referenced this issue Oct 29, 2015
Motivation:
When the ImmediateEventExecutor is in use it is possible to get a StackOverFlowException if when a promise completes a new listener is added to that promise.

Modifications:
- Protect against the case where LateListeners.run() smashes the stack.

Result:
Fixes #4395
Scottmitch added a commit that referenced this issue Oct 29, 2015
Motivation:
When the ImmediateEventExecutor is in use it is possible to get a StackOverFlowException if when a promise completes a new listener is added to that promise.

Modifications:
- Protect against the case where LateListeners.run() smashes the stack.

Result:
Fixes #4395
@buchgr
Copy link
Contributor

buchgr commented Oct 29, 2015

@Scottmitch Does this also fix #3450?

@normanmaurer
Copy link
Member

@buchgr yes I think so.

@Scottmitch
Copy link
Member

@buchgr - Based upon your description (same scenario that is described in this issue), I would think this should take care of #3450. Is it possible to verify?

@buchgr
Copy link
Contributor

buchgr commented Nov 3, 2015

@Scottmitch I don't have the code anymore, but yeah it looks like it fixes both issues. I think it's a perfectly valid fix as it makes it work with all Netty provided Executors. However, it doesn't work with Guava's DirectExecutor or Developer Joes SelfBakedDirectExecutor :). I couldn't think of fix for those scenarios myself either ...

@Scottmitch
Copy link
Member

@buchgr - I also couldn't think of a good way to generalize the behavior of the executor will always (or on the next execute call) use the same thread to execute tasks. ... and just punted for the near term :(

pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
Motivation:
When the ImmediateEventExecutor is in use it is possible to get a StackOverFlowException if when a promise completes a new listener is added to that promise.

Modifications:
- Protect against the case where LateListeners.run() smashes the stack.

Result:
Fixes netty#4395
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants