From 050ac709ba3db2a1c2df27f558bdcb348c4ac6f2 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Fri, 12 Feb 2016 10:23:39 -0800 Subject: [PATCH] PromiseNotifier does not propagate cancel events Motivation: If the Future that the PromiseNotifier is listening to is cancelled, it does not propagate the cancel to all the promises it is expected to notify. Modifications: - If the future is cancelled then all the promises should be cancelled - Add a UnaryPromiseNotifier if a collection of promises is not necessary Result: PromiseNotifier propagates cancel events to all promises --- .../util/concurrent/PromiseNotifier.java | 19 ++++--- .../util/concurrent/UnaryPromiseNotifier.java | 51 +++++++++++++++++++ .../util/concurrent/PromiseNotifierTest.java | 1 + 3 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 common/src/main/java/io/netty/util/concurrent/UnaryPromiseNotifier.java diff --git a/common/src/main/java/io/netty/util/concurrent/PromiseNotifier.java b/common/src/main/java/io/netty/util/concurrent/PromiseNotifier.java index 6de542bcb6..db1604fce9 100644 --- a/common/src/main/java/io/netty/util/concurrent/PromiseNotifier.java +++ b/common/src/main/java/io/netty/util/concurrent/PromiseNotifier.java @@ -57,13 +57,18 @@ public class PromiseNotifier> implements GenericFutureLis logger.warn("Failed to mark a promise as success because it is done already: {}", p); } } - return; - } - - Throwable cause = future.cause(); - for (Promise p: promises) { - if (!p.tryFailure(cause)) { - logger.warn("Failed to mark a promise as failure because it's done already: {}", p, cause); + } else if (future.isCancelled()) { + for (Promise p: promises) { + if (!p.cancel(false)) { + logger.warn("Failed to cancel a promise because it is done already: {}", p); + } + } + } else { + Throwable cause = future.cause(); + for (Promise p: promises) { + if (!p.tryFailure(cause)) { + logger.warn("Failed to mark a promise as failure because it's done already: {}", p, cause); + } } } } diff --git a/common/src/main/java/io/netty/util/concurrent/UnaryPromiseNotifier.java b/common/src/main/java/io/netty/util/concurrent/UnaryPromiseNotifier.java new file mode 100644 index 0000000000..9dc3a3c2ba --- /dev/null +++ b/common/src/main/java/io/netty/util/concurrent/UnaryPromiseNotifier.java @@ -0,0 +1,51 @@ +/* + * Copyright 2016 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.util.concurrent; + +import io.netty.util.internal.ObjectUtil; +import io.netty.util.internal.logging.InternalLogger; +import io.netty.util.internal.logging.InternalLoggerFactory; + +public final class UnaryPromiseNotifier implements FutureListener { + private static final InternalLogger logger = InternalLoggerFactory.getInstance(UnaryPromiseNotifier.class); + private final Promise promise; + + public UnaryPromiseNotifier(Promise promise) { + this.promise = ObjectUtil.checkNotNull(promise, "promise"); + } + + @Override + public void operationComplete(Future future) throws Exception { + cascadeTo(future, promise); + } + + public static void cascadeTo(Future completedFuture, Promise promise) { + if (completedFuture.isSuccess()) { + if (!promise.trySuccess(completedFuture.getNow())) { + logger.warn("Failed to mark a promise as success because it is done already: {}", promise); + } + } else if (completedFuture.isCancelled()) { + if (!promise.cancel(false)) { + logger.warn("Failed to cancel a promise because it is done already: {}", promise); + } + } else { + if (!promise.tryFailure(completedFuture.cause())) { + logger.warn("Failed to mark a promise as failure because it's done already: {}", promise, + completedFuture.cause()); + } + } + } +} diff --git a/common/src/test/java/io/netty/util/concurrent/PromiseNotifierTest.java b/common/src/test/java/io/netty/util/concurrent/PromiseNotifierTest.java index f163060a82..de477bf0ae 100644 --- a/common/src/test/java/io/netty/util/concurrent/PromiseNotifierTest.java +++ b/common/src/test/java/io/netty/util/concurrent/PromiseNotifierTest.java @@ -78,6 +78,7 @@ public class PromiseNotifierTest { Future future = createStrictMock(Future.class); Throwable t = createStrictMock(Throwable.class); expect(future.isSuccess()).andReturn(false); + expect(future.isCancelled()).andReturn(false); expect(future.cause()).andReturn(t); expect(p1.tryFailure(t)).andReturn(true); expect(p2.tryFailure(t)).andReturn(true);