Skip to content

Commit fe8f6f8

Browse files
cpovirkGoogle Java Core Libraries
authored andcommitted
Update implementation comments, mostly around known issues.
The TODOs reference: - #2143 - #3553 RELNOTES=n/a PiperOrigin-RevId: 807371529
1 parent 288aeae commit fe8f6f8

File tree

2 files changed

+58
-2
lines changed

2 files changed

+58
-2
lines changed

android/guava/src/com/google/common/util/concurrent/MoreExecutors.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,12 @@ public final List<Runnable> shutdownNow() {
527527
return delegate.shutdownNow();
528528
}
529529

530+
/*
531+
* TODO: https://github.com/google/guava/issues/2143 - In addition to overriding `execute`, also
532+
* override the `Future`-returning methods of `ExecutorService` to propagate cancellation from
533+
* our `TrustedListenableFutureTask` to a `Future` returned by the delegate executor?
534+
*/
535+
530536
@Override
531537
public final void execute(Runnable command) {
532538
delegate.execute(command);
@@ -600,7 +606,12 @@ public boolean cancel(boolean mayInterruptIfRunning) {
600606
// Unless it is cancelled, the delegate may continue being scheduled
601607
scheduledDelegate.cancel(mayInterruptIfRunning);
602608

603-
// TODO(user): Cancel "this" if "scheduledDelegate" is cancelled.
609+
/*
610+
* We'd love to also arrange for the inverse -- that is, to also automatically cancel this
611+
* future if scheduledDelegate is cancelled (as happens after the delegate executor is
612+
* shut down: https://github.com/google/guava/issues/3553). But it seems unlikely that
613+
* that's possible to detect in general.
614+
*/
604615
}
605616
return cancelled;
606617
}
@@ -631,7 +642,24 @@ public void run() {
631642
delegate.run();
632643
} catch (Throwable t) {
633644
// Any Exception is either a RuntimeException or sneaky checked exception.
645+
646+
/*
647+
* We fail this `ListenableFuture`, whose result is exposed to the user through the
648+
* `ListenableScheduledTask` we return from the `schedule*` methods.
649+
*/
634650
setException(t);
651+
652+
/*
653+
* We fail the current run of the recurring task so that it is not rescheduled. This also
654+
* fails the `ScheduledFuture`, which might be visible only to users who operate directly
655+
* on the delegate executor's queue.
656+
*
657+
* (Users who try to operate directly on the `ScheduledFuture` may have additional
658+
* problems. For example, if they cancel that `Future`, it won't cancel the user-visible
659+
* `ListenableScheduledTask`. This is essentially the same problem as the
660+
* `ListenableScheduledTask` has with executor shutdown:
661+
* https://github.com/google/guava/issues/3553)
662+
*/
635663
throw t;
636664
}
637665
}

guava/src/com/google/common/util/concurrent/MoreExecutors.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,12 @@ public final List<Runnable> shutdownNow() {
524524
return delegate.shutdownNow();
525525
}
526526

527+
/*
528+
* TODO: https://github.com/google/guava/issues/2143 - In addition to overriding `execute`, also
529+
* override the `Future`-returning methods of `ExecutorService` to propagate cancellation from
530+
* our `TrustedListenableFutureTask` to a `Future` returned by the delegate executor?
531+
*/
532+
527533
@Override
528534
public final void execute(Runnable command) {
529535
delegate.execute(command);
@@ -597,7 +603,12 @@ public boolean cancel(boolean mayInterruptIfRunning) {
597603
// Unless it is cancelled, the delegate may continue being scheduled
598604
scheduledDelegate.cancel(mayInterruptIfRunning);
599605

600-
// TODO(user): Cancel "this" if "scheduledDelegate" is cancelled.
606+
/*
607+
* We'd love to also arrange for the inverse -- that is, to also automatically cancel this
608+
* future if scheduledDelegate is cancelled (as happens after the delegate executor is
609+
* shut down: https://github.com/google/guava/issues/3553). But it seems unlikely that
610+
* that's possible to detect in general.
611+
*/
601612
}
602613
return cancelled;
603614
}
@@ -628,7 +639,24 @@ public void run() {
628639
delegate.run();
629640
} catch (Throwable t) {
630641
// Any Exception is either a RuntimeException or sneaky checked exception.
642+
643+
/*
644+
* We fail this `ListenableFuture`, whose result is exposed to the user through the
645+
* `ListenableScheduledTask` we return from the `schedule*` methods.
646+
*/
631647
setException(t);
648+
649+
/*
650+
* We fail the current run of the recurring task so that it is not rescheduled. This also
651+
* fails the `ScheduledFuture`, which might be visible only to users who operate directly
652+
* on the delegate executor's queue.
653+
*
654+
* (Users who try to operate directly on the `ScheduledFuture` may have additional
655+
* problems. For example, if they cancel that `Future`, it won't cancel the user-visible
656+
* `ListenableScheduledTask`. This is essentially the same problem as the
657+
* `ListenableScheduledTask` has with executor shutdown:
658+
* https://github.com/google/guava/issues/3553)
659+
*/
632660
throw t;
633661
}
634662
}

0 commit comments

Comments
 (0)