Make JavaBuilder use a unique coverage metadata directory

for each test instead of the same directory for all the tests. The previous implementation was using one directory for instrumenting the classes of a jar. For each each jar the metadata directory was deleted if it already existed. This is problematic for local execution when multiple tests are run in parallel because some threads will try to delete the directory and some will try to perform read/write operations on it.

This is an important fix for Bazel coverage users.

Fixes #4398.

RELNOTES: Java coverage works now with multiple jobs.
PiperOrigin-RevId: 199764483

2 files changed, 30 insertions, 20 deletions

diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java
index 50c936ac43..483b871d51 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java
@@ -148,17 +148,21 @@ public class SimpleJavaLibraryBuilder implements Closeable {
 
   public void buildJar(JavaLibraryBuildRequest build) throws IOException {
     JarCreator jar = new JarCreator(build.getOutputJar());
+    JacocoInstrumentationProcessor processor = null;
     try {
       jar.setNormalize(true);
       jar.setCompression(build.compressJar());
       jar.addDirectory(build.getClassDir());
       jar.setJarOwner(build.getTargetLabel(), build.getInjectingRuleKind());
-      JacocoInstrumentationProcessor processor = build.getJacocoInstrumentationProcessor();
+      processor = build.getJacocoInstrumentationProcessor();
       if (processor != null) {
         processor.processRequest(build, processor.isNewCoverageImplementation() ? jar : null);
       }
     } finally {
       jar.execute();
+      if (processor != null) {
+        processor.cleanup();
+      }
     }
   }
 
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java
index d3b37d257a..995c95d408 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java
@@ -37,24 +37,23 @@ public final class JacocoInstrumentationProcessor {
 
   public static JacocoInstrumentationProcessor create(List<String> args)
       throws InvalidCommandLineException {
-    if (args.size() < 2) {
+    if (args.size() < 1) {
       throw new InvalidCommandLineException(
-          "Number of arguments for Jacoco instrumentation should be 2+ (given "
+          "Number of arguments for Jacoco instrumentation should be 1+ (given "
               + args.size()
-              + ": metadataOutput metadataDirectory [filters*].");
+              + ": metadataOutput [filters*].");
     }
 
     // ignoring filters, they weren't used in the previous implementation
     // TODO(bazel-team): filters should be correctly handled
-    return new JacocoInstrumentationProcessor(args.get(1), args.get(0));
+    return new JacocoInstrumentationProcessor(args.get(0));
   }
 
-  private final String metadataDir;
+  private Path instrumentedClassesDirectory;
   private final String coverageInformation;
   private final boolean isNewCoverageImplementation;
 
-  private JacocoInstrumentationProcessor(String metadataDir, String coverageInfo) {
-    this.metadataDir = metadataDir;
+  private JacocoInstrumentationProcessor(String coverageInfo) {
     this.coverageInformation = coverageInfo;
     // This is part of the new Java coverage implementation where JacocoInstrumentationProcessor
     // receives a file that includes the relative paths of the uninstrumented Java files, instead
@@ -71,15 +70,10 @@ public final class JacocoInstrumentationProcessor {
    * jacocoMetadataDir, to be zipped up in the output file jacocoMetadataOutput.
    */
   public void processRequest(JavaLibraryBuildRequest build, JarCreator jar) throws IOException {
-    // Clean up jacocoMetadataDir to be used by postprocessing steps. This is important when
-    // running JavaBuilder locally, to remove stale entries from previous builds.
-    if (metadataDir != null) {
-      Path workDir = Paths.get(metadataDir);
-      if (Files.exists(workDir)) {
-        recursiveRemove(workDir);
-      }
-      Files.createDirectories(workDir);
-    }
+    // Use a directory for coverage metadata  that is unique to each built jar. Avoids
+    // multiple threads performing read/write/delete actions on the instrumented classes directory.
+    instrumentedClassesDirectory = getMetadataDirRelativeToJar(build.getOutputJar());
+    Files.createDirectories(instrumentedClassesDirectory);
     if (jar == null) {
       jar = new JarCreator(coverageInformation);
     }
@@ -87,14 +81,26 @@ public final class JacocoInstrumentationProcessor {
     jar.setCompression(build.compressJar());
     Instrumenter instr = new Instrumenter(new OfflineInstrumentationAccessGenerator());
     instrumentRecursively(instr, build.getClassDir());
-    jar.addDirectory(metadataDir);
+    jar.addDirectory(instrumentedClassesDirectory);
     if (isNewCoverageImplementation) {
       jar.addEntry(coverageInformation, coverageInformation);
     } else {
       jar.execute();
+      cleanup();
+    }
+  }
+
+  public void cleanup() throws IOException {
+    if (Files.exists(instrumentedClassesDirectory)) {
+      recursiveRemove(instrumentedClassesDirectory);
     }
   }
 
+  // Return the path of the coverage metadata directory relative to the output jar path.
+  private static Path getMetadataDirRelativeToJar(Path outputJar) {
+    return outputJar.resolveSibling(outputJar + "-coverage-metadata");
+  }
+
   /**
    * Runs Jacoco instrumentation processor over all .class files recursively, starting with root.
    */
@@ -119,9 +125,9 @@ public final class JacocoInstrumentationProcessor {
             if (isNewCoverageImplementation) {
               Path absoluteUninstrumentedCopy = Paths.get(file + ".uninstrumented");
               uninstrumentedCopy =
-                  Paths.get(metadataDir).resolve(root.relativize(absoluteUninstrumentedCopy));
+                  instrumentedClassesDirectory.resolve(root.relativize(absoluteUninstrumentedCopy));
             } else {
-              uninstrumentedCopy = Paths.get(metadataDir).resolve(root.relativize(file));
+              uninstrumentedCopy = instrumentedClassesDirectory.resolve(root.relativize(file));
             }
             Files.createDirectories(uninstrumentedCopy.getParent());
             Files.move(file, uninstrumentedCopy);