Fix tests that flake when building in parallel (#3124)

Author: tballison

tika-grpc/src/test/java/org/apache/tika/pipes/grpc/TikaGrpcServerTest.java

@@ -39,6 +39,8 @@
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import com.asarkar.grpc.test.GrpcCleanupExtension;
 import com.asarkar.grpc.test.Resources;
@@ -55,6 +57,7 @@
 import org.apache.commons.io.FileUtils;
 import org.jetbrains.annotations.NotNull;
 import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -141,6 +144,47 @@ static void clean() {
         }
     }
 
+    private final List<TikaGrpcServerImpl> services = new ArrayList<>();
+
+    /**
+     * Every service built here is closed by {@link #closeServices()}. Only the manager that
+     * started a pipes-server child can delete that child's temp dir, so a service the test
+     * drops on the floor -- including on an assertion failure -- orphans it.
+     */
+    private TikaGrpcServerImpl newService(Path config) throws Exception {
+        TikaGrpcServerImpl service = new TikaGrpcServerImpl(config.toAbsolutePath().toString());
+        services.add(service);
+        return service;
+    }
+
+    @AfterEach
+    void closeServices() throws Exception {
+        for (TikaGrpcServerImpl service : services) {
+            //both are idempotent, so a test that already shut down in-line is fine
+            service.shutdown();
+            service.postShutdown();
+        }
+        services.clear();
+        assertNoOrphanedServerTempDirs();
+    }
+
+    /**
+     * Fails the test that leaked rather than leaving it for whoever notices the temp dir later.
+     * Reliable because surefire gives each module its own java.io.tmpdir and runs these
+     * classes one at a time.
+     */
+    private static void assertNoOrphanedServerTempDirs() throws Exception {
+        Path tmp = Paths.get(System.getProperty("java.io.tmpdir"));
+        try (Stream<Path> paths = Files.list(tmp)) {
+            List<String> orphans = paths
+                    .map(p -> p.getFileName().toString())
+                    .filter(n -> n.startsWith("pipes-server-"))
+                    .sorted()
+                    .collect(Collectors.toList());
+            assertTrue(orphans.isEmpty(), "orphaned pipes-server temp dirs: " + orphans);
+        }
+    }
+
     static final int NUM_FETCHERS_TO_CREATE = 10;
 
     @Test
@@ -150,7 +194,7 @@ public void testFetcherCrud(Resources resources) throws Exception {
         Server server = InProcessServerBuilder
                 .forName(serverName)
                 .directExecutor()
-                .addService(new TikaGrpcServerImpl(tikaConfigUnlocked.toAbsolutePath().toString()))
+                .addService(newService(tikaConfigUnlocked))
                 .build()
                 .start();
         resources.register(server, Duration.ofSeconds(10));
@@ -451,13 +495,14 @@ public void testSavePipesIteratorValidatesType(Resources resources) throws Excep
         assertNotNull(reply.getMessage());
     }
 
-    private static TikaGrpc.TikaBlockingStub startServer(Resources resources, Path config)
+    //non-static: newService tracks the service on the per-test instance so it gets closed
+    private TikaGrpc.TikaBlockingStub startServer(Resources resources, Path config)
             throws Exception {
         String serverName = InProcessServerBuilder.generateName();
         Server server = InProcessServerBuilder
                 .forName(serverName)
                 .directExecutor()
-                .addService(new TikaGrpcServerImpl(config.toAbsolutePath().toString()))
+                .addService(newService(config))
                 .build()
                 .start();
         resources.register(server, Duration.ofSeconds(10));
@@ -481,7 +526,7 @@ public void testServerSideStreamingSendsTerminalSignal(Resources resources) thro
         Server server = InProcessServerBuilder
                 .forName(serverName)
                 .directExecutor()
-                .addService(new TikaGrpcServerImpl(tikaConfigUnlocked.toAbsolutePath().toString()))
+                .addService(newService(tikaConfigUnlocked))
                 .build()
                 .start();
         resources.register(server, Duration.ofSeconds(10));
@@ -544,7 +589,7 @@ public void onCompleted() {
     public void testBiStream(Resources resources) throws Exception {
         String serverName = InProcessServerBuilder.generateName();
 
-        TikaGrpcServerImpl tikaGrpcServerImpl = new TikaGrpcServerImpl(tikaConfigUnlocked.toAbsolutePath().toString());
+        TikaGrpcServerImpl tikaGrpcServerImpl = newService(tikaConfigUnlocked);
         Server server = InProcessServerBuilder
                 .forName(serverName)
                 .directExecutor()

tika-parent/pom.xml

@@ -310,6 +310,13 @@
     <puppycrawl.version>12.3.1</puppycrawl.version>
     <rat.version>0.18</rat.version>
     <scm.version>2.2.1</scm.version>
+    <!-- Per-module temp dir for the surefire JVM. Keeps one module's temp files out of
+         every other module's view: under -T1C the modules share one java.io.tmpdir, and
+         a test that counts apache-tika-* there races whatever else is building
+         (TestContainerAwareDetector.testRemovalTempfiles flaked exactly this way).
+         Created by the create-test-tmpdir antrun execution below, because the JVM will
+         not create a missing java.io.tmpdir; it throws NoSuchFileException. -->
+    <tika.test.tmpdir>${project.build.directory}/test-tmp</tika.test.tmpdir>
     <checkstyle.configLocation>${maven.multiModuleProjectDirectory}/tika-parent/checkstyle.xml</checkstyle.configLocation>
     <spotless.goal>apply</spotless.goal>
     <spotless.header.file>${maven.multiModuleProjectDirectory}/tika-parent/license-header.txt</spotless.header.file>
@@ -1506,9 +1513,34 @@
         <configuration>
           <!-- for manual testing of i18n, try for example: -Duser.language=zh -Duser.region=CN or
           -Duser.language=de -Duser.country=DE -->
-          <argLine>-Xmx4g -Djava.awt.headless=true @{surefireArgLine}</argLine>
+          <!-- java.io.tmpdir MUST be quoted: argLine is split on whitespace, and the Windows
+               job deliberately checks out into a path containing a space. It must also be a
+               real -D on the command line, not <systemPropertyVariables>: Files.createTempFile
+               reads the startup value, so a System.setProperty after JVM start is a no-op. -->
+          <argLine>-Xmx4g -Djava.awt.headless=true "-Djava.io.tmpdir=${tika.test.tmpdir}" @{surefireArgLine}</argLine>
         </configuration>
       </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-antrun-plugin</artifactId>
+        <version>${maven.antrun.version}</version>
+        <executions>
+          <execution>
+            <id>create-test-tmpdir</id>
+            <phase>process-test-resources</phase>
+            <goals>
+              <goal>run</goal>
+            </goals>
+            <configuration>
+              <target>
+                <!-- delete first so an incremental (unclean) build still starts empty -->
+                <delete dir="${tika.test.tmpdir}" />
+                <mkdir dir="${tika.test.tmpdir}" />
+              </target>
+            </configuration>
+          </execution>
+        </executions>
+      </plugin>
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-shade-plugin</artifactId>

tika-parsers/tika-parsers-extended/tika-parser-scientific-module/pom.xml

@@ -111,7 +111,8 @@ May the gods of dependency management fix this in the future.
       <plugin>
           <artifactId>maven-surefire-plugin</artifactId>
           <configuration>
-              <argLine>-Dcom.google.protobuf.use_unsafe_pre22_gencode</argLine>
+              <!-- this argLine replaces tika-parent's, so it has to repeat the tmpdir isolation -->
+              <argLine>-Dcom.google.protobuf.use_unsafe_pre22_gencode "-Djava.io.tmpdir=${tika.test.tmpdir}"</argLine>
           </configuration>
       </plugin>
       <plugin>

tika-parsers/tika-parsers-standard/tika-parsers-standard-integration-tests/src/test/java/org/apache/tika/detect/TestContainerAwareDetector.java

@@ -30,15 +30,16 @@
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Objects;
 import java.util.Random;
+import java.util.stream.Stream;
 
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 
 import org.apache.tika.MultiThreadedTikaTest;
 import org.apache.tika.Tika;
@@ -49,6 +50,7 @@
 import org.apache.tika.detect.zip.StreamingZipContainerDetector;
 import org.apache.tika.detect.zip.ZipContainerDetector;
 import org.apache.tika.exception.TikaException;
+import org.apache.tika.io.TemporaryResources;
 import org.apache.tika.io.TikaInputStream;
 import org.apache.tika.metadata.HttpHeaders;
 import org.apache.tika.metadata.Metadata;
@@ -71,6 +73,8 @@ public class TestContainerAwareDetector extends MultiThreadedTikaTest {
     private final StreamingZipContainerDetector streamingZipDetector =
             new StreamingZipContainerDetector();
 
+    @TempDir
+    private Path tempDir;
 
     @AfterEach
     public void tearDown() throws TikaException {
@@ -416,23 +420,33 @@ public void testRemovalTempfiles() throws Exception {
         assertRemovalTempfiles("test-documents.zip");
     }
 
-    private int countTemporaryFiles() {
-        //TODO: fix this.  This can prevent multiple parallel builds
-        //from running at the same time because there can be more than one
-        //process writing to apache-tika-*
-        return Objects.requireNonNull(new File(System.getProperty("java.io.tmpdir"))
-                .listFiles((dir, name) -> name.startsWith("apache-tika-"))).length;
+    /**
+     * Counts spool files in {@link #tempDir}. The shared java.io.tmpdir cannot be counted
+     * reliably: parallel module builds (-T1C) have other JVMs writing apache-tika-* there.
+     */
+    private int countTemporaryFiles() throws IOException {
+        try (Stream<Path> files = Files.list(tempDir)) {
+            return (int) files.count();
+        }
     }
 
     private void assertRemovalTempfiles(String fileName) throws Exception {
-        int numberOfTempFiles = countTemporaryFiles();
-
-        try (TikaInputStream tis = TikaInputStream
-                .get(getResourceAsUrl("/test-documents/" + fileName))) {
-            detector.detect(tis, new Metadata(), new ParseContext());
+        Metadata metadata = new Metadata();
+        metadata.set(TikaCoreProperties.RESOURCE_NAME_KEY, fileName);
+        TemporaryResources tmp = new TemporaryResources();
+        tmp.setTemporaryFileDirectory(tempDir);
+
+        try (InputStream is = getClass().getResourceAsStream("/test-documents/" + fileName)) {
+            assertNotNull(is);
+            try (TikaInputStream tis = TikaInputStream.get(is, tmp, metadata)) {
+                detector.detect(tis, metadata, new ParseContext());
+                //force the spool so this test can't pass vacuously
+                tis.getPath();
+                assertEquals(1, countTemporaryFiles());
+            }
         }
 
-        assertEquals(numberOfTempFiles, countTemporaryFiles());
+        assertEquals(0, countTemporaryFiles());
     }
 
     @Test

tika-pipes/tika-pipes-config-store-ignite/pom.xml

@@ -144,7 +144,9 @@
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
+          <!-- this argLine replaces tika-parent's, so it has to repeat the tmpdir isolation -->
           <argLine>
+            "-Djava.io.tmpdir=${tika.test.tmpdir}"
             --add-opens java.base/java.nio=ALL-UNNAMED
             --add-opens java.base/java.util=ALL-UNNAMED
             --add-opens java.base/java.lang=ALL-UNNAMED

tika-server/tika-server-core/pom.xml

@@ -152,7 +152,8 @@
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
           <redirectTestOutputToFile>true</redirectTestOutputToFile>
-          <argLine>-da -XX:+HeapDumpOnOutOfMemoryError -Xmx512m</argLine>
+          <!-- this argLine replaces tika-parent's, so it has to repeat the tmpdir isolation -->
+          <argLine>-da -XX:+HeapDumpOnOutOfMemoryError -Xmx512m "-Djava.io.tmpdir=${tika.test.tmpdir}"</argLine>
           <systemPropertyVariables>
             <java.util.logging.config.file>
               ${basedir}/src/main/resources/commons-logging.properties

tika-server/tika-server-standard/pom.xml

@@ -122,7 +122,8 @@
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
           <redirectTestOutputToFile>true</redirectTestOutputToFile>
-          <argLine>-da -XX:+HeapDumpOnOutOfMemoryError -Xmx512m</argLine>
+          <!-- this argLine replaces tika-parent's, so it has to repeat the tmpdir isolation -->
+          <argLine>-da -XX:+HeapDumpOnOutOfMemoryError -Xmx512m "-Djava.io.tmpdir=${tika.test.tmpdir}"</argLine>
           <systemPropertyVariables>
             <java.util.logging.config.file>
               ${basedir}/src/main/resources/log4j2.xml