Skip to content

Commit

Permalink
When we catch InterruptedException, take care to restore the interr…
Browse files Browse the repository at this point in the history
…upt bit.

We already were doing this in the cases in which our `catch` block was of the form `catch (InterruptedException e)`, but we weren't doing it when we were catching a more general type, such as `Exception`.

RELNOTES=`util.concurrent`: Fixed some cases in which we could catch `InterruptedException` but fail to restore the interrupt bit.
PiperOrigin-RevId: 447706714
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed May 10, 2022
1 parent cd5cf32 commit 8f0350a
Show file tree
Hide file tree
Showing 22 changed files with 80 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.util.concurrent.MoreExecutors.rejectionPropagatingExecutor;
import static com.google.common.util.concurrent.NullnessCasts.uncheckedCastNullableTToT;
import static com.google.common.util.concurrent.Platform.isInstanceOfThrowableClass;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtCompatible;
import com.google.common.base.Function;
Expand Down Expand Up @@ -132,6 +133,7 @@ public final void run() {
try {
fallbackResult = doFallback(localFallback, castThrowable);
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
setException(t);
return;
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.common.util.concurrent;

import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.Beta;
import com.google.common.annotations.GwtIncompatible;
import com.google.common.base.Supplier;
Expand Down Expand Up @@ -65,9 +67,11 @@ public void run() {
try {
AbstractExecutionThreadService.this.run();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
try {
shutDown();
} catch (Exception ignored) {
restoreInterruptIfIsInterruptedException(ignored);
// TODO(lukes): if guava ever moves to java7, this would be a good
// candidate for a suppressed exception, or maybe we could generalize
// Closer.Suppressor
Expand All @@ -84,6 +88,7 @@ public void run() {
shutDown();
notifyStopped();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.common.util.concurrent;

import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.base.Supplier;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
Expand Down Expand Up @@ -61,6 +63,7 @@ public void run() {
startUp();
notifyStarted();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand All @@ -78,6 +81,7 @@ public void run() {
shutDown();
notifyStopped();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.util.concurrent.Futures.immediateCancelledFuture;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;
import static java.util.Objects.requireNonNull;

import com.google.common.annotations.GwtIncompatible;
Expand Down Expand Up @@ -202,9 +203,11 @@ public void run() {
}
AbstractScheduledService.this.runOneIteration();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
try {
shutDown();
} catch (Exception ignored) {
restoreInterruptIfIsInterruptedException(ignored);
logger.log(
Level.WARNING,
"Error while attempting to shut down the service after failure.",
Expand Down Expand Up @@ -242,6 +245,7 @@ public void run() {
runningTask = scheduler().schedule(delegate, executorService, task);
notifyStarted();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
if (runningTask != null) {
// prevent the task from running if possible
Expand Down Expand Up @@ -280,6 +284,7 @@ public void run() {
}
notifyStopped();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand Down Expand Up @@ -553,6 +558,7 @@ public Cancellable reschedule() {
try {
schedule = CustomScheduler.this.getNextSchedule();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
service.notifyFailed(t);
return new FutureAsCancellable(immediateCancelledFuture());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;
import static com.google.common.util.concurrent.Service.State.FAILED;
import static com.google.common.util.concurrent.Service.State.NEW;
import static com.google.common.util.concurrent.Service.State.RUNNING;
Expand Down Expand Up @@ -249,6 +250,7 @@ public final Service startAsync() {
enqueueStartingEvent();
doStart();
} catch (Throwable startupFailure) {
restoreInterruptIfIsInterruptedException(startupFailure);
notifyFailed(startupFailure);
} finally {
monitor.leave();
Expand Down Expand Up @@ -288,6 +290,7 @@ public final Service stopAsync() {
throw new AssertionError("isStoppable is incorrectly implemented, saw: " + previous);
}
} catch (Throwable shutdownFailure) {
restoreInterruptIfIsInterruptedException(shutdownFailure);
notifyFailed(shutdownFailure);
} finally {
monitor.leave();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.util.concurrent.Futures.getDone;
import static com.google.common.util.concurrent.MoreExecutors.rejectionPropagatingExecutor;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtCompatible;
import com.google.common.base.Function;
Expand Down Expand Up @@ -121,6 +122,7 @@ public final void run() {
try {
transformResult = doTransform(localFunction, sourceResult);
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
// This exception is irrelevant in this thread, but useful for the client.
setException(t);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.common.util.concurrent;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.Beta;
import com.google.common.annotations.GwtIncompatible;
Expand Down Expand Up @@ -59,6 +60,7 @@ public <T> T newProxy(
} catch (RuntimeException e) {
throw new UncheckedExecutionException(e);
} catch (Exception e) {
restoreInterruptIfIsInterruptedException(e);
throw new ExecutionException(e);
} catch (Error e) {
throw new ExecutionError(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.common.util.concurrent;

import static com.google.common.util.concurrent.NullnessCasts.uncheckedCastNullableTToT;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.VisibleForTesting;
Expand Down Expand Up @@ -74,6 +75,7 @@ public final void run() {
result = runInterruptibly();
}
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
error = t;
} finally {
// Attempt to set the task as done so that further attempts to interrupt will fail.
Expand Down
10 changes: 10 additions & 0 deletions android/guava/src/com/google/common/util/concurrent/Platform.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

package com.google.common.util.concurrent;

import static com.google.common.base.Preconditions.checkNotNull;
import static java.lang.Thread.currentThread;

import com.google.common.annotations.GwtCompatible;
import javax.annotation.CheckForNull;

Expand All @@ -26,5 +29,12 @@ static boolean isInstanceOfThrowableClass(
return expectedClass.isInstance(t);
}

static void restoreInterruptIfIsInterruptedException(Throwable t) {
checkNotNull(t); // to satisfy NullPointerTester
if (t instanceof InterruptedException) {
currentThread().interrupt();
}
}

private Platform() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Throwables.throwIfUnchecked;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -66,6 +67,7 @@ protected Runnable wrapTask(Runnable command) {
try {
wrapped.call();
} catch (Exception e) {
restoreInterruptIfIsInterruptedException(e);
throwIfUnchecked(e);
throw new RuntimeException(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,7 @@ static boolean isInstanceOfThrowableClass(Throwable t, Class<? extends Throwable
return true;
}

static void restoreInterruptIfIsInterruptedException(Throwable t) {}

private Platform() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.util.concurrent.MoreExecutors.rejectionPropagatingExecutor;
import static com.google.common.util.concurrent.NullnessCasts.uncheckedCastNullableTToT;
import static com.google.common.util.concurrent.Platform.isInstanceOfThrowableClass;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtCompatible;
import com.google.common.base.Function;
Expand Down Expand Up @@ -132,6 +133,7 @@ public final void run() {
try {
fallbackResult = doFallback(localFallback, castThrowable);
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
setException(t);
return;
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.common.util.concurrent;

import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.Beta;
import com.google.common.annotations.GwtIncompatible;
import com.google.common.base.Supplier;
Expand Down Expand Up @@ -66,9 +68,11 @@ public void run() {
try {
AbstractExecutionThreadService.this.run();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
try {
shutDown();
} catch (Exception ignored) {
restoreInterruptIfIsInterruptedException(ignored);
// TODO(lukes): if guava ever moves to java7, this would be a good
// candidate for a suppressed exception, or maybe we could generalize
// Closer.Suppressor
Expand All @@ -85,6 +89,7 @@ public void run() {
shutDown();
notifyStopped();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.common.util.concurrent;

import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtIncompatible;
import com.google.common.base.Supplier;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
Expand Down Expand Up @@ -62,6 +64,7 @@ public void run() {
startUp();
notifyStarted();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand All @@ -79,6 +82,7 @@ public void run() {
shutDown();
notifyStopped();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.util.concurrent.Futures.immediateCancelledFuture;
import static com.google.common.util.concurrent.Internal.toNanosSaturated;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.NANOSECONDS;

Expand Down Expand Up @@ -232,9 +233,11 @@ public void run() {
}
AbstractScheduledService.this.runOneIteration();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
try {
shutDown();
} catch (Exception ignored) {
restoreInterruptIfIsInterruptedException(ignored);
logger.log(
Level.WARNING,
"Error while attempting to shut down the service after failure.",
Expand Down Expand Up @@ -272,6 +275,7 @@ public void run() {
runningTask = scheduler().schedule(delegate, executorService, task);
notifyStarted();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
if (runningTask != null) {
// prevent the task from running if possible
Expand Down Expand Up @@ -310,6 +314,7 @@ public void run() {
}
notifyStopped();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
notifyFailed(t);
}
}
Expand Down Expand Up @@ -595,6 +600,7 @@ public Cancellable reschedule() {
try {
schedule = CustomScheduler.this.getNextSchedule();
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
service.notifyFailed(t);
return new FutureAsCancellable(immediateCancelledFuture());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;
import static com.google.common.util.concurrent.Service.State.FAILED;
import static com.google.common.util.concurrent.Service.State.NEW;
import static com.google.common.util.concurrent.Service.State.RUNNING;
Expand Down Expand Up @@ -250,6 +251,7 @@ public final Service startAsync() {
enqueueStartingEvent();
doStart();
} catch (Throwable startupFailure) {
restoreInterruptIfIsInterruptedException(startupFailure);
notifyFailed(startupFailure);
} finally {
monitor.leave();
Expand Down Expand Up @@ -289,6 +291,7 @@ public final Service stopAsync() {
throw new AssertionError("isStoppable is incorrectly implemented, saw: " + previous);
}
} catch (Throwable shutdownFailure) {
restoreInterruptIfIsInterruptedException(shutdownFailure);
notifyFailed(shutdownFailure);
} finally {
monitor.leave();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.util.concurrent.Futures.getDone;
import static com.google.common.util.concurrent.MoreExecutors.rejectionPropagatingExecutor;
import static com.google.common.util.concurrent.Platform.restoreInterruptIfIsInterruptedException;

import com.google.common.annotations.GwtCompatible;
import com.google.common.base.Function;
Expand Down Expand Up @@ -121,6 +122,7 @@ public final void run() {
try {
transformResult = doTransform(localFunction, sourceResult);
} catch (Throwable t) {
restoreInterruptIfIsInterruptedException(t);
// This exception is irrelevant in this thread, but useful for the client.
setException(t);
return;
Expand Down
Loading

0 comments on commit 8f0350a

Please sign in to comment.