Merge pull request #120 from ia3andy/fix-deadlock

Fix deadlock issue when watching
mvnpm · Sep 30, 2024 · b54f983 · b54f983
2 parents 697bfbc + 08828bf
commit b54f983
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 33 deletions.
diff --git a/pom.xml b/pom.xml
@@ -37,7 +37,7 @@
     <maven.compiler.release>17</maven.compiler.release>
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <esbuild.version>0.20.1-mvnpm-0.0.7</esbuild.version>
-    <quarkus.version>3.6.0</quarkus.version>
+    <quarkus.version>3.11.1</quarkus.version>
     <esbuild.scss.version>v0.0.7</esbuild.scss.version>
     <formatter.plugin.version>2.23.0</formatter.plugin.version>
     <impsort.plugin.version>1.9.0</impsort.plugin.version>

diff --git a/src/main/java/io/mvnpm/esbuild/Execute.java b/src/main/java/io/mvnpm/esbuild/Execute.java
@@ -1,10 +1,6 @@
 package io.mvnpm.esbuild;
 
-import java.io.BufferedReader;
-import java.io.File;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.InputStreamReader;
+import java.io.*;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -33,8 +29,6 @@ public class Execute {
         t.setDaemon(true);
         return t;
     });
-    private static final ExecutorService EXECUTOR_BUILD_LISTENERS = Executors
-            .newSingleThreadExecutor(r -> new Thread(r, "Build Listeners"));
     private static final Logger logger = Logger.getLogger(Execute.class.getName());
 
     private final Path workDir;
@@ -72,11 +66,38 @@ public ExecuteResult executeAndWait() throws IOException {
 
     public WatchStartResult watch(BuildEventListener listener) throws IOException {
         final Process process = createProcess(getCommand(), Optional.of(listener));
+        final ExecutorService executorStreamer = Executors
+                .newSingleThreadExecutor(r -> new Thread(r, "Esbuild watch stdout streamer"));
+        final ExecutorService executorBuild = Executors
+                .newSingleThreadExecutor(r -> new Thread(r, "Esbuild build listeners notify"));
+        final AtomicReference<WatchBuildResult> result = new AtomicReference<>();
+        CountDownLatch latch = new CountDownLatch(1);
+        final WatchStartResult.WatchProcess watchProcess = new WatchStartResult.WatchProcess() {
+            @Override
+            public boolean isAlive() {
+                return process.isAlive();
+            }
+
+            @Override
+            public void close() throws IOException {
+                process.destroyForcibly();
+                executorStreamer.shutdownNow();
+                executorBuild.shutdownNow();
+                try {
+                    process.waitFor();
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                    throw new RuntimeException(e);
+                }
+                if (latch.getCount() == 1) {
+                    latch.countDown();
+                }
+            }
+        };
         try {
             final InputStream processStream = process.getInputStream();
-            CountDownLatch latch = new CountDownLatch(1);
-            final AtomicReference<WatchBuildResult> result = new AtomicReference<>();
-            EXECUTOR_STREAMER.execute(new Streamer(process::isAlive, processStream, (r) -> {
+
+            executorStreamer.execute(new Streamer(executorBuild, process::isAlive, processStream, (r) -> {
                 if (latch.getCount() == 1) {
                     result.set(r);
                     latch.countDown();
@@ -95,9 +116,11 @@ public WatchStartResult watch(BuildEventListener listener) throws IOException {
             if (!process.isAlive() && !result.get().isSuccess()) {
                 throw result.get().bundleException();
             }
-            return new WatchStartResult(result.get(), process);
+
+            return new WatchStartResult(result.get(), watchProcess);
         } catch (InterruptedException e) {
             Thread.currentThread().interrupt();
+            watchProcess.close();
             throw new RuntimeException(e);
         }
     }
@@ -128,7 +151,7 @@ public Process createProcess(final String[] command, final Optional<BuildEventLi
                 .command(command).start();
     }
 
-    private record Streamer(BooleanSupplier isAlive, InputStream processStream,
+    private record Streamer(ExecutorService executorBuild, BooleanSupplier isAlive, InputStream processStream,
             BuildEventListener listener, Consumer<WatchBuildResult> onExit) implements Runnable {
 
         @Override
@@ -143,7 +166,7 @@ public void run() {
                     final String output = outputBuilder.toString();
                     final boolean error = hasError.getAndSet(false);
                     outputBuilder.setLength(0);
-                    EXECUTOR_BUILD_LISTENERS.execute(() -> {
+                    executorBuild.execute(() -> {
                         if (!error) {
                             listener.onBuild(new WatchBuildResult(output));
                         } else {

diff --git a/src/main/java/io/mvnpm/esbuild/Watch.java b/src/main/java/io/mvnpm/esbuild/Watch.java
@@ -1,22 +1,24 @@
 package io.mvnpm.esbuild;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.List;
 
 import io.mvnpm.esbuild.model.EntryPoint;
 import io.mvnpm.esbuild.model.WatchBuildResult;
+import io.mvnpm.esbuild.model.WatchStartResult;
 
-public class Watch {
+public class Watch implements Closeable {
 
-    private final Process process;
+    private final WatchStartResult.WatchProcess process;
     private final Path workDir;
 
     private final Path dist;
 
     private final WatchBuildResult firstBuildResult;
 
-    public Watch(Process process, Path workDir, Path dist, WatchBuildResult firstBuildResult) {
+    public Watch(WatchStartResult.WatchProcess process, Path workDir, Path dist, WatchBuildResult firstBuildResult) {
         this.process = process;
         this.workDir = workDir;
         this.dist = dist;
@@ -27,19 +29,9 @@ public void updateEntries(List<EntryPoint> entries) throws IOException {
         entries.forEach(entry -> entry.process(workDir));
     }
 
-    public void stop() {
-        process.destroy();
-
-    }
-
-    public void waitForStop() {
-        process.destroy();
-        try {
-            process.waitFor();
-        } catch (InterruptedException e) {
-            Thread.currentThread().interrupt();
-            throw new RuntimeException(e);
-        }
+    @Override
+    public void close() throws IOException {
+        process.close();
     }
 
     public Path workDir() {
@@ -57,4 +49,5 @@ public boolean isAlive() {
     public Path dist() {
         return dist;
     }
+
 }
diff --git a/src/main/java/io/mvnpm/esbuild/model/WatchStartResult.java b/src/main/java/io/mvnpm/esbuild/model/WatchStartResult.java
@@ -1,4 +1,13 @@
 package io.mvnpm.esbuild.model;
 
-public record WatchStartResult(WatchBuildResult firstBuildResult, Process process) {
+import java.io.Closeable;
+
+public record WatchStartResult(WatchBuildResult firstBuildResult, WatchProcess process) {
+
+    public interface WatchProcess extends Closeable {
+
+        boolean isAlive();
+
+    }
+
 }
diff --git a/src/test/java/io/mvnpm/esbuild/BundlerTest.java b/src/test/java/io/mvnpm/esbuild/BundlerTest.java
@@ -111,7 +111,7 @@ public void shouldWatch() throws URISyntaxException, IOException, InterruptedExc
         assertTrue(Files.readString(distApp).contains("alert(\"foo\");"));
 
         // then
-        watch.waitForStop();
+        watch.close();
 
         assertFalse(watch.isAlive());
     }
@@ -154,7 +154,7 @@ public void shouldWatchWithError() throws URISyntaxException, IOException, Inter
         assertTrue(Files.readString(distApp).contains("alert(\"foo\");"));
 
         // then
-        watch.stop();
+        watch.close();
     }
 
     @Test