Repository: netty/netty
Issue: StackOverflowError when adding listener to DefaultPromise w/ ImmediateEventExecutor
Labels: duplicate

This will produce a `StackOverflowError`:

``` java
    @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


Issue comments:
thanks for reporting @maxwindiff!

@Scottmitch Does this also fix https://github.com/netty/netty/issues/3450?

@buchgr yes I think so.

@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?

@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 ... 

@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 :(


Pull request review comments:
Static?

done

just a nit but do we want to also prefix the new methods with "test" as the others in the class ?

done


Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java
index 4aec5b53061..142b88d5573 100644
--- a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java
+++ b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java
@@ -836,7 +836,8 @@ private final class LateListeners extends ArrayDeque<GenericFutureListener<?>> i
 
         @Override
         public void run() {
-            if (listeners == null) {
+            final EventExecutor executor = executor();
+            if (listeners == null || executor == ImmediateEventExecutor.INSTANCE) {
                 for (;;) {
                     GenericFutureListener<?> l = poll();
                     if (l == null) {
@@ -847,7 +848,7 @@ public void run() {
             } else {
                 // Reschedule until the initial notification is done to avoid the race condition
                 // where the notification is made in an incorrect order.
-                execute(executor(), this);
+                execute(executor, this);
             }
         }
     }


Test patch:
diff --git a/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java b/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java
index 26f304b09f3..ea7a56e6545 100644
--- a/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java
+++ b/common/src/test/java/io/netty/util/concurrent/DefaultPromiseTest.java
@@ -143,6 +143,38 @@ public void testListenerNotifyLater() throws Exception {
         testListenerNotifyLater(2);
     }
 
+    @Test(timeout = 2000)
+    public void testPromiseListenerAddWhenCompleteFailure() throws Exception {
+        testPromiseListenerAddWhenComplete(new RuntimeException());
+    }
+
+    @Test(timeout = 2000)
+    public void testPromiseListenerAddWhenCompleteSuccess() throws Exception {
+        testPromiseListenerAddWhenComplete(null);
+    }
+
+    private static void testPromiseListenerAddWhenComplete(Throwable cause) throws InterruptedException {
+        final CountDownLatch latch = new CountDownLatch(1);
+        final Promise<Void> promise = new DefaultPromise<Void>(ImmediateEventExecutor.INSTANCE);
+        promise.addListener(new FutureListener<Void>() {
+            @Override
+            public void operationComplete(Future<Void> future) throws Exception {
+                promise.addListener(new FutureListener<Void>() {
+                    @Override
+                    public void operationComplete(Future<Void> future) throws Exception {
+                        latch.countDown();
+                    }
+                });
+            }
+        });
+        if (cause == null) {
+            promise.setSuccess(null);
+        } else {
+            promise.setFailure(cause);
+        }
+        latch.await();
+    }
+
     private static void testListenerNotifyLater(final int numListenersBefore) throws Exception {
         EventExecutor executor = new TestEventExecutor();
         int expectedCount = numListenersBefore + 2;