#9784 use shutdown() instead of shutdownNow() (#10063)

asolntsev · pujagani · diemol · web-flow · commit 28070d2de242 · 2021-11-24T11:05:36.000+05:30
* #9784 use `shutdown()` instead of `shutdownNow()` it waits for completion of already started tasks. Otherwise it may causes errors in those tasks, including succeeding NoClassDefFoundError. * #9784 shutdown gracefully the executor service Co-authored-by: Puja Jagani <puja.jagani93@gmail.com> Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
diff --git a/java/src/org/openqa/selenium/concurrent/BUILD.bazel b/java/src/org/openqa/selenium/concurrent/BUILD.bazel
@@ -5,6 +5,7 @@ java_library(
     srcs = glob(["*.java"]),
     visibility = [
         "//java/src/org/openqa/selenium/grid:__subpackages__",
+        "//java/src/org/openqa/selenium/remote:__subpackages__"
     ],
     deps = [
         "//java/src/org/openqa/selenium:core",
diff --git a/java/src/org/openqa/selenium/concurrent/ExecutorServices.java b/java/src/org/openqa/selenium/concurrent/ExecutorServices.java
@@ -0,0 +1,26 @@
+package org.openqa.selenium.concurrent;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.logging.Level.WARNING;
+
+import java.util.concurrent.ExecutorService;
+import java.util.logging.Logger;
+
+public class ExecutorServices {
+
+  private static final Logger LOG = Logger.getLogger(ExecutorServices.class.getName());
+
+  public static void shutdownGracefully(String name, ExecutorService service) {
+    service.shutdown();
+    try {
+      if (!service.awaitTermination(5, SECONDS)) {
+        LOG.warning(String.format("Failed to shutdown %s", name));
+        service.shutdownNow();
+      }
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      LOG.log(WARNING, String.format("Failed to shutdown %s", name), e);
+      service.shutdownNow();
+    }
+  }
+}
diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java
@@ -2,6 +2,7 @@
 
 import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.openqa.selenium.concurrent.ExecutorServices.shutdownGracefully;
 
 import com.google.common.annotations.VisibleForTesting;
 
@@ -85,6 +86,7 @@
   description = "New session queue")
 public class LocalNewSessionQueue extends NewSessionQueue implements Closeable {
 
+  private static final String NAME = "Local New Session Queue";
   private final EventBus bus;
   private final SlotMatcher slotMatcher;
   private final Duration requestTimeout;
@@ -95,7 +97,7 @@ public class LocalNewSessionQueue extends NewSessionQueue implements Closeable {
   private final ScheduledExecutorService service = Executors.newSingleThreadScheduledExecutor(r -> {
     Thread thread = new Thread(r);
     thread.setDaemon(true);
-    thread.setName("Local New Session Queue");
+    thread.setName(NAME);
     return thread;
   });
 
@@ -407,7 +409,7 @@ public boolean isReady() {
 
   @Override
   public void close() throws IOException {
-    service.shutdownNow();
+    shutdownGracefully(NAME, service);
   }
 
   private void failDueToTimeout(RequestId reqId) {
@@ -418,7 +420,7 @@ private class Data {
     public final Instant endTime;
     public Either<SessionNotCreatedException, CreateSessionResponse> result;
     private boolean complete;
-    private CountDownLatch latch = new CountDownLatch(1);
+    private final CountDownLatch latch = new CountDownLatch(1);
 
     public Data(Instant enqueued) {
       this.endTime = enqueued.plus(requestTimeout);
diff --git a/java/src/org/openqa/selenium/remote/BUILD.bazel b/java/src/org/openqa/selenium/remote/BUILD.bazel
@@ -48,6 +48,7 @@ java_library(
     ],
     deps = [
         "//java/src/org/openqa/selenium:core",
+        "//java/src/org/openqa/selenium/concurrent",
         "//java/src/org/openqa/selenium/devtools",
         "//java/src/org/openqa/selenium/io",
         "//java/src/org/openqa/selenium/json",
diff --git a/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java b/java/src/org/openqa/selenium/remote/service/DriverCommandExecutor.java
@@ -17,6 +17,8 @@
 
 package org.openqa.selenium.remote.service;
 
+import static org.openqa.selenium.concurrent.ExecutorServices.shutdownGracefully;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Throwables;
 
@@ -46,10 +48,11 @@
  */
 public class DriverCommandExecutor extends HttpCommandExecutor implements Closeable {
 
+  private static final String NAME = "Driver Command Executor";
   private final DriverService service;
   private final ExecutorService executorService = Executors.newFixedThreadPool(2, r -> {
     Thread thread = new Thread(r);
-    thread.setName("Driver Command Executor");
+    thread.setName(NAME);
     thread.setDaemon(true);
     return thread;
   });
@@ -131,7 +134,7 @@ public Response execute(Command command) throws IOException {
         Thread.currentThread().interrupt();
         throw new WebDriverException("Timed out waiting for driver server to stop.", e);
       } finally {
-        executorService.shutdownNow();
+        close();
       }
 
     } else {
@@ -166,6 +169,6 @@ Response invokeExecute(Command command) throws IOException {
 
   @Override
   public void close() {
-    executorService.shutdownNow();
+    shutdownGracefully(NAME, executorService);
   }
 }
diff --git a/java/src/org/openqa/selenium/remote/service/DriverService.java b/java/src/org/openqa/selenium/remote/service/DriverService.java
@@ -17,6 +17,10 @@
 
 package org.openqa.selenium.remote.service;
 
+import static java.util.Collections.emptyMap;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.openqa.selenium.concurrent.ExecutorServices.shutdownGracefully;
+
 import com.google.common.collect.ImmutableMap;
 
 import org.openqa.selenium.Beta;
@@ -46,9 +50,6 @@
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.locks.ReentrantLock;
 
-import static java.util.Collections.emptyMap;
-import static java.util.concurrent.TimeUnit.SECONDS;
-
 /**
  * Manages the life and death of a native executable driver server.
  *
@@ -60,10 +61,12 @@
  */
 public class DriverService implements Closeable {
 
+  private static final String NAME = "Driver Service Executor";
   protected static final Duration DEFAULT_TIMEOUT = Duration.ofSeconds(20);
+
   private final ExecutorService executorService = Executors.newFixedThreadPool(2, r -> {
     Thread thread = new Thread(r);
-    thread.setName("Driver Service Executor");
+    thread.setName(NAME);
     thread.setDaemon(true);
     return thread;
   });
@@ -286,7 +289,7 @@ public void stop() {
     } finally {
       process = null;
       lock.unlock();
-      executorService.shutdownNow();
+      close();
     }
 
     if (toThrow != null) {
@@ -308,7 +311,7 @@ protected OutputStream getOutputStream() {
 
   @Override
   public void close() {
-    executorService.shutdownNow();
+    shutdownGracefully(NAME, executorService);
   }
 
   private enum StartOrDie {
