Don't attempt to submit to a shut-down scheduler (#138)

We have a runtime shutdown hook which shuts down this scheduler.
Attempting to submit a task to a shutdown scheduler throws an exception.

Instead, guard submiting tasks to it on having acquired the lock and
checking that it isn't shutdown. It's fine not to schedule a task to
shutdown in the future, if we're already shutting down.

We could alternatively catch the exception (potentially avoiding needing
the lock), but as we're acquiring the lock anyway, this way we have more
clear logic and don't need to worry about other potential exceptions
getting incorrectly swallowed.

Also, have TimeoutHandler starts its own ScheduledThreadPoolExecutor
rather than being passed one, so that it knows nothing else could be
sneakily shutting it down or submitting tasks to it, so we can actually
uphold the @GuardedBy invariant.

java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/GrpcServer.java

@@ -63,7 +63,7 @@ public void start() throws IOException {
             new Thread() {
               @Override
               public void run() {
-                timeoutHandler.cancelOutstanding();
+                timeoutHandler.cancelOutstandingAndStopScheduling();
                 try {
                   GrpcServer.this.stop();
                 } catch (InterruptedException e) {

java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/Main.java

@@ -3,7 +3,6 @@
 import java.io.IOException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.CommandLineParser;
 import org.apache.commons.cli.DefaultParser;
@@ -22,8 +21,7 @@ public static void main(String[] args) throws IOException, InterruptedException
     line = commandLineOptions(args);
     Main main = new Main();
 
-    TimeoutHandler timeoutHander =
-        new TimeoutHandler(new ScheduledThreadPoolExecutor(1), main.idleTimeout());
+    TimeoutHandler timeoutHander = new TimeoutHandler(main.idleTimeout());
     main.runServer(timeoutHander);
   }
 

java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/TimeoutHandler.java

@@ -3,6 +3,7 @@
 import com.google.errorprone.annotations.concurrent.GuardedBy;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.slf4j.Logger;
@@ -11,7 +12,11 @@
 public class TimeoutHandler {
   private static final Logger logger = LoggerFactory.getLogger(TimeoutHandler.class);
 
+  // Anything which shuts down the executor, or relies on it not being shutdown, must do so under
+  // this lock.
+  @GuardedBy("lastFutureLock")
   private final ScheduledExecutorService executor;
+
   private final int timeoutSeconds;
   private final AtomicInteger inFlightRequests = new AtomicInteger(0);
 
@@ -20,8 +25,8 @@ public class TimeoutHandler {
   @GuardedBy("lastFutureLock")
   private ScheduledFuture<?> lastFuture = null;
 
-  public TimeoutHandler(ScheduledExecutorService executor, int timeoutSeconds) {
-    this.executor = executor;
+  public TimeoutHandler(int timeoutSeconds) {
+    this.executor = new ScheduledThreadPoolExecutor(1);
     this.timeoutSeconds = timeoutSeconds;
     schedule();
   }
@@ -45,6 +50,12 @@ void schedule() {
       if (last != null) {
         last.cancel(true);
       }
+      if (this.executor.isShutdown()) {
+        // If the executor is already shutdown, the process is already terminating - we neither can
+        // (because it's shutdown) nor need to (because we're about to terminate) schedule a new
+        // task.
+        return;
+      }
       this.lastFuture =
           this.executor.schedule(
               () -> {
@@ -59,13 +70,19 @@ void schedule() {
     }
   }
 
-  public void cancelOutstanding() {
+  /**
+   * Cancel any outstanding scheduled tasks, and shut down any background threads.
+   *
+   * <p>This object becomes useless after this method is called - it will not perform its timeout
+   * functionality any more.
+   */
+  public void cancelOutstandingAndStopScheduling() {
     synchronized (lastFutureLock) {
       if (lastFuture != null) {
         lastFuture.cancel(true);
         lastFuture = null;
       }
+      this.executor.shutdownNow();
     }
-    this.executor.shutdownNow();
   }
 }