[GitHub] [solr] magibney commented on a diff in pull request #833: SOLR-16178: ZkController#fireEventListeners thread should be shutdown on close

[GitBox <gi...@apache.org>]

magibney commented on code in PR #833:
URL: https://github.com/apache/solr/pull/833#discussion_r864065556


##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -705,6 +709,8 @@ public void preClose() {
     } finally {
       ExecutorUtil.shutdownAndAwaitTermination(customThreadPool);
     }
+
+    ExecutorUtil.shutdownAndAwaitTermination(fireEventListenersThreadPool);

Review Comment:
   Do we perhaps want `shutdownNowAndAwaitTermination()` here? At least some related test failures involve submitted tasks in free-floating Thread blocking searcher init; I think `shutdownAndAwaitTermination()` would track those threads, so not _leak_ them, but might nonetheless deadlock in the same way?
   
   Would it be too cute to shutdown the `fireEventListenersThreadPool` via a task submitted to `customThreadPool`, immediately above? I don't think order should matter with this, so "the sooner the better" might be the way to go (unless there's a reason to delay `fireEventListenersThreadPool` shutdown until after the other close tasks are complete). FWIW I think this use would be in keeping with the spirit of the `customThreadPool`.



##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -2754,19 +2760,18 @@ private boolean fireEventListeners(String zkDir) {
       if (listeners != null && !listeners.isEmpty()) {
         final Set<Runnable> listenersCopy = new HashSet<>(listeners);
         // run these in a separate thread because this can be long running
-        new Thread(
-                () -> {
-                  log.debug("Running listeners for {}", zkDir);
-                  for (final Runnable listener : listenersCopy) {
-                    try {
-                      listener.run();
-                    } catch (Exception e) {
-                      log.warn("listener throws error", e);
-                    }
-                  }
-                },
-                "ZKEventListenerThread")
-            .start();
+        fireEventListenersThreadPool.submit(
+            () -> {
+              log.debug("Running listeners for {}", zkDir);
+              for (final Runnable listener : listenersCopy) {
+                try {
+                  listener.run();
+                } catch (Exception e) {
+                  log.warn("listener throws error", e);
+                }
+              }
+              return null;

Review Comment:
   Could we just remove `return null` and let this be a Runnable as opposed to a Callable? There's no return, and no checked exceptions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org