Skip to content

Commit

Permalink
Unconditionally call setException for RejectedExecutionException.
Browse files Browse the repository at this point in the history
Under JDK11, the write to thrownByExecute / thrownFromDelegate is upsetting TSAN. I *suspect* that TSAN is incorrect (and that it is likely to identify the same "problem" in other code).

However, we years ago questioned whether the code I'm removing served any real purpose...
(fixes #2877)
...so now seems like a good time to get rid of it and, at minimum, hopefully eliminate the current TSAN failure.

The code that we're running in the executor is under our control. (That is, we're the ones who implement Runnable.run().) We're catching exceptions (and thus any RejectedExecutionException) in the obvious places. Occasionally we do call listeners, but naturally we do that only after completing the future or at least calling setFuture (after which a stray setException(rejectedExecutionException) would be a no-op).

The one(?) exception to that is that InterruptibleTask.run() can call currentThread.interrupt(), which we learned a few years ago can call arbitrary code through nio callbacks. At some point, though, I throw my hands up and say "whatever."

(If the interrupt were to be a problem, it would have to be with an executor that executes tasks inline, like directExecutor. But I note that rejectionPropagatingExecutor, at least, already defends against this for directExecutor itself (by skipping the RejectedExecutionException logic entirely). So the danger exists only with CombinedFutureInterruptibleTask or with a non-directExecutor that can execute tasks inline without catching exceptions -- and again, only in concert with nio interrupt callbacks. I think.)

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=321374425
  • Loading branch information
cpovirk authored and netdpb committed Jul 16, 2020
1 parent 32f2d77 commit da93601
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ protected void interruptTask() {
@WeakOuter
private abstract class CombinedFutureInterruptibleTask<T> extends InterruptibleTask<T> {
private final Executor listenerExecutor;
boolean thrownByExecute = true;

CombinedFutureInterruptibleTask(Executor listenerExecutor) {
this.listenerExecutor = checkNotNull(listenerExecutor);
Expand All @@ -104,9 +103,7 @@ final void execute() {
try {
listenerExecutor.execute(this);
} catch (RejectedExecutionException e) {
if (thrownByExecute) {
CombinedFuture.this.setException(e);
}
CombinedFuture.this.setException(e);
}
}

Expand Down Expand Up @@ -153,7 +150,6 @@ private final class AsyncCallableInterruptibleTask

@Override
ListenableFuture<V> runInterruptibly() throws Exception {
thrownByExecute = false;
ListenableFuture<V> result = callable.call();
return checkNotNull(
result,
Expand Down Expand Up @@ -184,7 +180,6 @@ private final class CallableInterruptibleTask extends CombinedFutureInterruptibl

@Override
V runInterruptibly() throws Exception {
thrownByExecute = false;
return callable.call();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -973,31 +973,12 @@ static Executor rejectionPropagatingExecutor(
return delegate;
}
return new Executor() {
boolean thrownFromDelegate = true;

@Override
public void execute(final Runnable command) {
public void execute(Runnable command) {
try {
delegate.execute(
new Runnable() {
@Override
public void run() {
thrownFromDelegate = false;
command.run();
}

@Override
public String toString() {
return command.toString();
}
});
delegate.execute(command);
} catch (RejectedExecutionException e) {
if (thrownFromDelegate) {
// wrap exception?
future.setException(e);
}
// otherwise it must have been thrown from a transitive call and the delegate runnable
// should have handled it.
future.setException(e);
}
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ protected void interruptTask() {
@WeakOuter
private abstract class CombinedFutureInterruptibleTask<T> extends InterruptibleTask<T> {
private final Executor listenerExecutor;
boolean thrownByExecute = true;

CombinedFutureInterruptibleTask(Executor listenerExecutor) {
this.listenerExecutor = checkNotNull(listenerExecutor);
Expand All @@ -104,9 +103,7 @@ final void execute() {
try {
listenerExecutor.execute(this);
} catch (RejectedExecutionException e) {
if (thrownByExecute) {
CombinedFuture.this.setException(e);
}
CombinedFuture.this.setException(e);
}
}

Expand Down Expand Up @@ -153,7 +150,6 @@ private final class AsyncCallableInterruptibleTask

@Override
ListenableFuture<V> runInterruptibly() throws Exception {
thrownByExecute = false;
ListenableFuture<V> result = callable.call();
return checkNotNull(
result,
Expand Down Expand Up @@ -184,7 +180,6 @@ private final class CallableInterruptibleTask extends CombinedFutureInterruptibl

@Override
V runInterruptibly() throws Exception {
thrownByExecute = false;
return callable.call();
}

Expand Down
25 changes: 3 additions & 22 deletions guava/src/com/google/common/util/concurrent/MoreExecutors.java
Original file line number Diff line number Diff line change
Expand Up @@ -1078,31 +1078,12 @@ static Executor rejectionPropagatingExecutor(
return delegate;
}
return new Executor() {
boolean thrownFromDelegate = true;

@Override
public void execute(final Runnable command) {
public void execute(Runnable command) {
try {
delegate.execute(
new Runnable() {
@Override
public void run() {
thrownFromDelegate = false;
command.run();
}

@Override
public String toString() {
return command.toString();
}
});
delegate.execute(command);
} catch (RejectedExecutionException e) {
if (thrownFromDelegate) {
// wrap exception?
future.setException(e);
}
// otherwise it must have been thrown from a transitive call and the delegate runnable
// should have handled it.
future.setException(e);
}
}
};
Expand Down

0 comments on commit da93601

Please sign in to comment.