[lucene] branch branch_9x updated: Shared executor for LuceneTestCase#newSearcher callers (#12588)

[ja...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

javanna pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/lucene.git


The following commit(s) were added to refs/heads/branch_9x by this push:
     new 0fb47cd44a6 Shared executor for LuceneTestCase#newSearcher callers (#12588)
0fb47cd44a6 is described below

commit 0fb47cd44a6cb051075998f31a81890a4a842040
Author: Luca Cavanna <ja...@apache.org>
AuthorDate: Mon Sep 25 17:00:59 2023 +0200

    Shared executor for LuceneTestCase#newSearcher callers (#12588)
    
    Until now, LuceneTestCase#newSearcher randomly associates the returned
    IndexSearcher instance with an executor that is ad-hoc created, which
    gets shut down when the index reader is closed.
    
    This has made us catch a couple of cases where we were not properly
    closing readers in tests. Most recently, we have been seeing test
    failures (OOM - unable to create thread) due to too many executor
    instances created as part of the same test. This is to be attributed to
    creating too many searcher instance, each one getting its separate
    executor, which all get shutdown at the end of the entire suite. The
    main offender for this is QueryUtils which creates a new searcher for
    each leaf reader, and the top-level reader gets closed in the
    AfterClass, hence all the executors will stay around for the entire
    duration of the test suite that relies on QueryUtils.
    
    This commit eagerly creates an executor in an additional before class
    method for LuceneTestCase, and associates that with each searcher that
    is supposed to get a non null executor.
    
    Note that the executor is shutdown in the after class to ensure that
    no threads leak in tests.
    
    This has the additional advantage that it removes the need to close the
    executor as part of an index reader close listener, which also requires
    the reader to have an associated reader cache helper.
---
 .../apache/lucene/tests/util/LuceneTestCase.java   | 51 +++++++++++++---------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java b/lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java
index a869eaf34a4..1f3ac470686 100644
--- a/lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java
+++ b/lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java
@@ -93,6 +93,7 @@ import java.util.Random;
 import java.util.Set;
 import java.util.TimeZone;
 import java.util.TreeSet;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
@@ -1900,6 +1901,32 @@ public abstract class LuceneTestCase extends Assert {
     System.clearProperty(ConcurrentMergeScheduler.DEFAULT_CPU_CORE_COUNT_PROPERTY);
   }
 
+  private static ExecutorService executor;
+
+  @BeforeClass
+  public static void setUpExecutorService() {
+    int threads = TestUtil.nextInt(random(), 1, 2);
+    executor =
+        new ThreadPoolExecutor(
+            threads,
+            threads,
+            0L,
+            TimeUnit.MILLISECONDS,
+            new LinkedBlockingQueue<Runnable>(),
+            new NamedThreadFactory("LuceneTestCase"));
+    // uncomment to intensify LUCENE-3840
+    // executor.prestartAllCoreThreads();
+    if (VERBOSE) {
+      System.out.println("NOTE: Created shared ExecutorService with " + threads + " threads");
+    }
+  }
+
+  @AfterClass
+  public static void shutdownExecutorService() {
+    TestUtil.shutdownExecutorService(executor);
+    executor = null;
+  }
+
   /** Create a new searcher over the reader. This searcher might randomly use threads. */
   public static IndexSearcher newSearcher(IndexReader r) {
     return newSearcher(r, true);
@@ -1963,30 +1990,14 @@ public abstract class LuceneTestCase extends Assert {
       ret.setSimilarity(classEnvRule.similarity);
       return ret;
     } else {
-      int threads = 0;
-      final ThreadPoolExecutor ex;
-      if (r.getReaderCacheHelper() == null || random.nextBoolean()) {
+      final ExecutorService ex;
+      if (random.nextBoolean()) {
         ex = null;
       } else {
-        threads = TestUtil.nextInt(random, 1, 8);
-        ex =
-            new ThreadPoolExecutor(
-                threads,
-                threads,
-                0L,
-                TimeUnit.MILLISECONDS,
-                new LinkedBlockingQueue<Runnable>(),
-                new NamedThreadFactory("LuceneTestCase"));
-        // uncomment to intensify LUCENE-3840
-        // ex.prestartAllCoreThreads();
-      }
-      if (ex != null) {
+        ex = executor;
         if (VERBOSE) {
-          System.out.println(
-              "NOTE: newSearcher using ExecutorService with " + threads + " threads");
+          System.out.println("NOTE: newSearcher using shared ExecutorService");
         }
-        r.getReaderCacheHelper()
-            .addClosedListener(cacheKey -> TestUtil.shutdownExecutorService(ex));
       }
       IndexSearcher ret;
       if (wrapWithAssertions) {