Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: Watchdog controls lifecycle of the future, not executor #1890

Merged
merged 3 commits into from
Dec 2, 2022
Merged

fix: Watchdog controls lifecycle of the future, not executor #1890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82875be
fix: Watchdog control lifecycle of the future, not executor
meltsufin Nov 17, 2022
a7f978f
docs: clarify WhatchdogProvider.shouldAutoClose()
meltsufin Nov 17, 2022
b035fbf
code formatting
meltsufin Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions gax/src/main/java/com/google/api/gax/rpc/Watchdog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@
import java.util.Map.Entry;
import java.util.concurrent.CancellationException;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;
Expand All @@ -61,6 +63,7 @@
* </ul>
*/
public final class Watchdog implements Runnable, BackgroundResource {

private static final Logger LOG = Logger.getLogger(Watchdog.class.getName());

// Dummy value to convert the ConcurrentHashMap into a Set
Expand Down Expand Up @@ -138,12 +141,12 @@ public void shutdown() {

@Override
public boolean isShutdown() {
return executor.isShutdown();
return future.isCancelled();
}

@Override
public boolean isTerminated() {
return executor.isTerminated();
return future.isDone();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably equivalent to future.isCancelled().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think isDone and isCancelled are equivalent. And both are slightly wrong for isTerminated. I think the strict definition of isTerminated is: not only has the task been unscheduled, but also that the last invocation stopped running.

Practically I dont think it matters too much. So if you want to take a shortcut and just use isDone, but document it.

Alternatively use a CountdownLatch where each run() is wrapped in an increment & decrement operation on the latch. awaitTermination can then block on the latch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the Javadoc for isDone:

Returns true if this task completed. Completion may be due to normal termination, an exception, or cancellation -- in all of these cases, this method will return true.

It sounds like the last invocation would have stopped running. What makes you think otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cancellation of the future is only cancelling the scheduled tasks, if the run() method is already running and takes long time to complete, then isDone() could return true while the run() is still running. I just reproduced this in my local as well.
That being said, not sure if it matters that much in practice like Igor said, I don't know how long it takes for a typical run() to complete. Using a CountdownLatch is not enough in this case since it does not support increment, only decrement. So we may need something more if we want to fix it perfectly.

}

@Override
Expand All @@ -153,7 +156,14 @@ public void shutdownNow() {

@Override
public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException {
return executor.awaitTermination(duration, unit);
try {
future.get(duration, unit);
return true;
} catch (ExecutionException | CancellationException e) {
return true;
} catch (TimeoutException e) {
return false;
}
}

@Override
Expand Down
1 change: 1 addition & 0 deletions gax/src/main/java/com/google/api/gax/rpc/WatchdogProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,6 @@ public interface WatchdogProvider {

Watchdog getWatchdog();

/** Return true if the watchdog should be automatically unscheduled. */
boolean shouldAutoClose();
}