Repository: netty/netty
Issue: [4.1] Futures removeListener: JavaDoc and implementation are inconsistent - duplicate listeners
Labels: documentation

#### Netty version: 4.1.0.CR7
#### Context:

I encountered an inconsistency between the JavaDoc description and implementation of the [Future](http://netty.io/4.1/api/io/netty/util/concurrent/Future.html).
I want to add multiple times (e.g. x 1000) the same close listener to a channel but I don't want this listener to be called multiple times.
Also I don't want to remove the listener prior to adding it. I could do this but I don't like the wasteful [Java array copy](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/concurrent/DefaultFutureListeners.java#L62) calls.
##### JavaDoc vs Implementation

The Netty 4.1 JavaDoc on [Future](http://netty.io/4.1/api/io/netty/util/concurrent/Future.html#removeListener%28io.netty.util.concurrent.GenericFutureListener%29) says:

> Future<V> removeListener(GenericFutureListener<? extends Future<? super V>> listener)
> 
> Removes the specified listener from this future. The specified listener is no longer notified when this future is done. If the specified listener is not associated with this future, this method does nothing and returns silently.

But if I add multiple times the **same listener** to the future, only the first entry is removed. ([see implementation](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/concurrent/DefaultFutureListeners.java#L55))

With the current implementation in mind the JavaDoc should state that only the first occurrence of the listener is removed. Like:

> Removes the first occurrence of the specified listener from this future.
##### ~~More on futures (_could be an independent proposal_):~~

~~Even more the listener is called multiple times. I think futures are part of the observer pattern. That means an observer should be notified only once an event occurs.~~

~~That means there should be no duplicate listeners in the future.
E.g.: the [listeners array](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/concurrent/DefaultFutureListeners.java#L22) sould be changed to a set, or duplicates avoided.~~

~~Or is there a puprose, that I can not see at the moment? Like performance? But if it is performance, then would not multiple calls on the same listener effect the performance? For example:~~

``` java
private final ChannelFutureListener closeListener = f -> cache.invalidate(f.channel);

public void oftenCalled(Channel ch) {
  ch.closeFuture().addListener(closeListener);
  ...
}
```

~~With the current implementation `closeListener` would be called as often as the method `oftenCalled`.~~
I had just a discussion that there could be a use-case of a self-incrementing counter, that uses the future to add a future increment. This counter would need multiple entries.
Therefore I propose to add a new method "addListenerIfAbsent", that could resolve the proposal. I'll create a new issue for that.

Best regards,
Michael


Issue comments:
Splitted the proposal to new issue: https://github.com/netty/netty/issues/5352

@mickare - Thanks for bringing this to our attention. The javadocs should be updated. Let me take care.


Pull request review comments:

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/concurrent/Future.java b/common/src/main/java/io/netty/util/concurrent/Future.java
index 8ddb860e149..16ffa722813 100644
--- a/common/src/main/java/io/netty/util/concurrent/Future.java
+++ b/common/src/main/java/io/netty/util/concurrent/Future.java
@@ -63,7 +63,7 @@ public interface Future<V> extends java.util.concurrent.Future<V> {
     Future<V> addListeners(GenericFutureListener<? extends Future<? super V>>... listeners);
 
     /**
-     * Removes the specified listener from this future.
+     * Removes the first occurrence of the specified listener from this future.
      * The specified listener is no longer notified when this
      * future is {@linkplain #isDone() done}.  If the specified
      * listener is not associated with this future, this method
@@ -72,7 +72,7 @@ public interface Future<V> extends java.util.concurrent.Future<V> {
     Future<V> removeListener(GenericFutureListener<? extends Future<? super V>> listener);
 
     /**
-     * Removes the specified listeners from this future.
+     * Removes the first occurrence for each of the listeners from this future.
      * The specified listeners are no longer notified when this
      * future is {@linkplain #isDone() done}.  If the specified
      * listeners are not associated with this future, this method


Test patch: