From f6f75646021167e8451220da78485def998cf398 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Mon, 4 Feb 2019 19:07:42 +0100
Subject: [PATCH] Don't update state of PromiseCombiner when finish(null) is
 called (#8843)

Motivation:

When we fail a call to PromiseCombiner.finish(...) because of a null argument we must not update the internal state before throwing.

Modifications:

- First do the null check and only after we validated that the argument is not null update the internal state
- Add test case.

Modifications:

Do not mess up internal state of PromiseCombiner when finish(...) is called with a null argument.

Result:

After your change, what will change.
---
 .../io/netty/util/concurrent/PromiseCombiner.java   |  3 ++-
 .../netty/util/concurrent/PromiseCombinerTest.java  | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/common/src/main/java/io/netty/util/concurrent/PromiseCombiner.java b/common/src/main/java/io/netty/util/concurrent/PromiseCombiner.java
index 730a9fe205..d53f7b2294 100644
--- a/common/src/main/java/io/netty/util/concurrent/PromiseCombiner.java
+++ b/common/src/main/java/io/netty/util/concurrent/PromiseCombiner.java
@@ -113,11 +113,12 @@ public final class PromiseCombiner {
      * @param aggregatePromise the promise to notify when all combined futures have finished
      */
     public void finish(Promise<Void> aggregatePromise) {
+        requireNonNull(aggregatePromise, "aggregatePromise");
         if (doneAdding) {
             throw new IllegalStateException("Already finished");
         }
         doneAdding = true;
-        this.aggregatePromise = requireNonNull(aggregatePromise, "aggregatePromise");
+        this.aggregatePromise = aggregatePromise;
         if (doneCount == expectedCount) {
             tryPromise();
         }
diff --git a/common/src/test/java/io/netty/util/concurrent/PromiseCombinerTest.java b/common/src/test/java/io/netty/util/concurrent/PromiseCombinerTest.java
index 1625a7f3b3..b528f92f99 100644
--- a/common/src/test/java/io/netty/util/concurrent/PromiseCombinerTest.java
+++ b/common/src/test/java/io/netty/util/concurrent/PromiseCombinerTest.java
@@ -15,6 +15,7 @@
  */
 package io.netty.util.concurrent;
 
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
@@ -58,6 +59,18 @@ public class PromiseCombinerTest {
         combiner = new PromiseCombiner();
     }
 
+    @Test
+    public void testNullArgument() {
+        try {
+            combiner.finish(null);
+            Assert.fail();
+        } catch (NullPointerException expected) {
+            // expected
+        }
+        combiner.finish(p1);
+        verify(p1).trySuccess(null);
+    }
+
     @Test
     public void testNullAggregatePromise() {
         combiner.finish(p1);