Skip to content

Commit

Permalink
Fix improper synchronization in DefaultPromise. Fixes netty#5489
Browse files Browse the repository at this point in the history
Motivation:

A race detector found that DefaultPromise.listeners is improperly synchronized [1].
Worst case a listener will not be executed when the promise is completed.

Modifications:

Make DefaultPromise.listeners a volatile.

Result:

Hopefully, DefaultPromise is more correct under concurrent execution.

[1] grpc/grpc-java#2015
  • Loading branch information
buchgr authored and liuzhengyang committed Sep 10, 2017
1 parent 15bc8e1 commit e1b0dc9
Showing 1 changed file with 1 addition and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class DefaultPromise<V> extends AbstractFuture<V> implements Promise<V> {
*
* Threading - synchronized(this). We must support adding listeners when there is no EventExecutor.
*/
private Object listeners;
private volatile Object listeners;
/**
* Threading - synchronized(this). We are required to hold the monitor to use Java's underlying wait()/notifyAll().
*/
Expand Down Expand Up @@ -417,7 +417,6 @@ protected static void notifyListener(
}

private void notifyListeners() {
// Modifications to listeners should be done in a synchronized block before this, and should be visible here.
if (listeners == null) {
return;
}
Expand Down

0 comments on commit e1b0dc9

Please sign in to comment.