Merge pull request #111 from ia3andy/prep-for-live-reload

Make the watch process more consistant in case of errors
mvnpm · May 16, 2024 · 4068bbf · 4068bbf
2 parents d4714fc + 19375b3
commit 4068bbf
Show file tree

Hide file tree

Showing 7 changed files with 139 additions and 44 deletions.
diff --git a/src/main/java/io/mvnpm/esbuild/BuildEventListener.java b/src/main/java/io/mvnpm/esbuild/BuildEventListener.java
@@ -1,9 +1,9 @@
 package io.mvnpm.esbuild;
 
-import java.util.Optional;
+import io.mvnpm.esbuild.model.WatchBuildResult;
 
 public interface BuildEventListener {
 
-    void onBuild(Optional<BundleException> error);
+    void onBuild(WatchBuildResult result);
 
 }
diff --git a/src/main/java/io/mvnpm/esbuild/Bundler.java b/src/main/java/io/mvnpm/esbuild/Bundler.java
@@ -12,10 +12,7 @@
 import java.util.logging.Logger;
 
 import io.mvnpm.esbuild.install.WebDepsInstaller;
-import io.mvnpm.esbuild.model.BundleOptions;
-import io.mvnpm.esbuild.model.BundleResult;
-import io.mvnpm.esbuild.model.EsBuildConfig;
-import io.mvnpm.esbuild.model.ExecuteResult;
+import io.mvnpm.esbuild.model.*;
 import io.mvnpm.esbuild.resolve.Resolver;
 
 public class Bundler {
@@ -91,8 +88,8 @@ public static Watch watch(BundleOptions bundleOptions, BuildEventListener eventL
         final Path dist = workDir.resolve(out);
         final EsBuildConfig esBuildConfig = prepareForBundling(bundleOptions, workDir, dist, true);
 
-        final Process process = esBuild(workDir, esBuildConfig, eventListener);
-        return new Watch(process, workDir, dist);
+        final WatchStartResult r = esBuildWatch(workDir, esBuildConfig, eventListener);
+        return new Watch(r.process(), workDir, dist, r.firstBuildResult());
     }
 
     private static Path getWorkDir(BundleOptions bundleOptions) throws IOException {
@@ -115,10 +112,10 @@ public static void clearDependencies(Path nodeModulesDir) throws IOException {
         deleteRecursive(nodeModulesDir);
     }
 
-    protected static Process esBuild(Path workDir, EsBuildConfig esBuildConfig, BuildEventListener listener)
+    protected static WatchStartResult esBuildWatch(Path workDir, EsBuildConfig esBuildConfig, BuildEventListener listener)
             throws IOException {
         final Execute execute = getExecute(workDir, esBuildConfig);
-        return execute.execute(listener);
+        return execute.watch(listener);
     }
 
     protected static ExecuteResult esBuild(Path workDir, EsBuildConfig esBuildConfig) throws IOException {

diff --git a/src/main/java/io/mvnpm/esbuild/Execute.java b/src/main/java/io/mvnpm/esbuild/Execute.java
@@ -11,15 +11,20 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BooleanSupplier;
 import java.util.function.Consumer;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import io.mvnpm.esbuild.model.EsBuildConfig;
 import io.mvnpm.esbuild.model.ExecuteResult;
+import io.mvnpm.esbuild.model.WatchBuildResult;
+import io.mvnpm.esbuild.model.WatchStartResult;
 
 public class Execute {
 
@@ -65,8 +70,36 @@ public ExecuteResult executeAndWait() throws IOException {
         }
     }
 
-    public Process execute(BuildEventListener listener) throws IOException {
-        return createProcess(getCommand(), Optional.of(listener));
+    public WatchStartResult watch(BuildEventListener listener) throws IOException {
+        final Process process = createProcess(getCommand(), Optional.of(listener));
+        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) -> {
+                if (latch.getCount() == 1) {
+                    result.set(r);
+                    latch.countDown();
+                } else {
+                    listener.onBuild(r);
+                }
+            }, r -> {
+                if (latch.getCount() == 1) {
+                    result.set(r);
+                    latch.countDown();
+                } else if (!r.isSuccess()) {
+                    listener.onBuild(r);
+                }
+            }));
+            latch.await();
+            if (!process.isAlive() && !result.get().isSuccess()) {
+                throw result.get().bundleException();
+            }
+            return new WatchStartResult(result.get(), process);
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw new RuntimeException(e);
+        }
     }
 
     private String[] getCommand() {
@@ -91,38 +124,43 @@ private String[] getCommand(String[] args) {
     }
 
     public Process createProcess(final String[] command, final Optional<BuildEventListener> listener) throws IOException {
-        Process process = new ProcessBuilder().redirectErrorStream(listener.isPresent()).directory(workDir.toFile())
+        return new ProcessBuilder().redirectErrorStream(listener.isPresent()).directory(workDir.toFile())
                 .command(command).start();
-        final InputStream errorStream = process.getInputStream();
-        listener.ifPresent(
-                buildEventListener -> EXECUTOR_STREAMER
-                        .execute(new Streamer(process::isAlive, errorStream, buildEventListener)));
-        return process;
     }
 
     private record Streamer(BooleanSupplier isAlive, InputStream processStream,
-            BuildEventListener listener) implements Runnable {
+            BuildEventListener listener, Consumer<WatchBuildResult> onExit) implements Runnable {
 
         @Override
         public void run() {
-            final StringBuilder errorBuilder = new StringBuilder();
+            final AtomicBoolean hasError = new AtomicBoolean();
+            final StringBuilder outputBuilder = new StringBuilder();
             consumeStream(isAlive, processStream, l -> {
                 logger.fine(l);
+                outputBuilder.append("\n").append(l);
                 if (l.contains("build finished")) {
                     logger.fine("Build finished!");
-                    final String error = errorBuilder.toString();
-                    errorBuilder.setLength(0);
+                    final String output = outputBuilder.toString();
+                    final boolean error = hasError.getAndSet(false);
+                    outputBuilder.setLength(0);
                     EXECUTOR_BUILD_LISTENERS.execute(() -> {
-                        if (error.isEmpty()) {
-                            listener.onBuild(Optional.empty());
+                        if (!error) {
+                            listener.onBuild(new WatchBuildResult(output));
                         } else {
-                            listener.onBuild(Optional.of(new BundleException("Error during bundling", error)));
+                            listener.onBuild(
+                                    new WatchBuildResult(output, new BundleException("Error during bundling", output)));
                         }
                     });
-                } else if (l.contains("[ERROR]") || !errorBuilder.isEmpty()) {
-                    errorBuilder.append("\n").append(l);
+                } else if (l.contains("[ERROR]")) {
+                    hasError.set(true);
                 }
             });
+            if (!hasError.get()) {
+                onExit.accept(new WatchBuildResult(outputBuilder.toString()));
+            } else {
+                onExit.accept(new WatchBuildResult(outputBuilder.toString(),
+                        new BundleException("Process exited with error", outputBuilder.toString())));
+            }
         }
     }
 
@@ -132,13 +170,16 @@ private static String readStream(InputStream stream) {
         return s.toString();
     }
 
-    private static void consumeStream(BooleanSupplier shouldStop, InputStream stream, Consumer<String> newLineConsumer) {
+    private static void consumeStream(BooleanSupplier stayAlive, InputStream stream, Consumer<String> newLineConsumer) {
         try (
                 final InputStreamReader in = new InputStreamReader(stream, StandardCharsets.UTF_8);
                 final BufferedReader reader = new BufferedReader(in)) {
             String line;
-            while ((line = reader.readLine()) != null && shouldStop.getAsBoolean()) {
+            while ((line = reader.readLine()) != null) {
                 newLineConsumer.accept(line);
+                if (!stayAlive.getAsBoolean()) {
+                    break;
+                }
             }
         } catch (IOException e) {
             // ignore

diff --git a/src/main/java/io/mvnpm/esbuild/Watch.java b/src/main/java/io/mvnpm/esbuild/Watch.java
@@ -5,6 +5,7 @@
 import java.util.List;
 
 import io.mvnpm.esbuild.model.EntryPoint;
+import io.mvnpm.esbuild.model.WatchBuildResult;
 
 public class Watch {
 
@@ -13,10 +14,13 @@ public class Watch {
 
     private final Path dist;
 
-    public Watch(Process process, Path workDir, Path dist) {
+    private final WatchBuildResult firstBuildResult;
+
+    public Watch(Process process, Path workDir, Path dist, WatchBuildResult firstBuildResult) {
         this.process = process;
         this.workDir = workDir;
         this.dist = dist;
+        this.firstBuildResult = firstBuildResult;
     }
 
     public void updateEntries(List<EntryPoint> entries) throws IOException {
@@ -25,12 +29,31 @@ public void updateEntries(List<EntryPoint> entries) throws IOException {
 
     public void stop() {
         process.destroy();
+
+    }
+
+    public void waitForStop() {
+        process.destroy();
+        try {
+            process.waitFor();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw new RuntimeException(e);
+        }
     }
 
     public Path workDir() {
         return workDir;
     }
 
+    public WatchBuildResult firstBuildResult() {
+        return firstBuildResult;
+    }
+
+    public boolean isAlive() {
+        return process.isAlive();
+    }
+
     public Path dist() {
         return dist;
     }

diff --git a/src/main/java/io/mvnpm/esbuild/model/WatchBuildResult.java b/src/main/java/io/mvnpm/esbuild/model/WatchBuildResult.java
@@ -0,0 +1,13 @@
+package io.mvnpm.esbuild.model;
+
+import io.mvnpm.esbuild.BundleException;
+
+public record WatchBuildResult(String output, BundleException bundleException) {
+    public WatchBuildResult(String output) {
+        this(output, null);
+    }
+
+    public boolean isSuccess() {
+        return bundleException == null;
+    }
+}
diff --git a/src/main/java/io/mvnpm/esbuild/model/WatchStartResult.java b/src/main/java/io/mvnpm/esbuild/model/WatchStartResult.java
@@ -0,0 +1,4 @@
+package io.mvnpm.esbuild.model;
+
+public record WatchStartResult(WatchBuildResult firstBuildResult, Process process) {
+}
diff --git a/src/test/java/io/mvnpm/esbuild/BundlerTest.java b/src/test/java/io/mvnpm/esbuild/BundlerTest.java
@@ -65,6 +65,19 @@ public void shouldBundle() throws URISyntaxException, IOException {
         executeTest(List.of("/webjars/htmx.org-1.8.4.jar"), WebDependencyType.WEBJARS, "application-webjar.js", true);
     }
 
+    @Test
+    public void shouldWatchExitWithError() throws URISyntaxException, IOException, InterruptedException {
+        final BundleOptions options = getBundleOptions(List.of("/mvnpm/stimulus-3.2.1.jar"),
+                WebDependencyType.MVNPM,
+                "application-mvnpm.js").withEsConfig(EsBuildConfig.builder().fixedEntryNames().define("foo", "\"bar").build())
+                .build();
+
+        assertThrows(BundleException.class, () -> {
+            Bundler.watch(options, (r) -> {
+            }, true);
+        });
+    }
+
     @Test
     public void shouldWatch() throws URISyntaxException, IOException, InterruptedException {
         // given
@@ -75,29 +88,32 @@ public void shouldWatch() throws URISyntaxException, IOException, InterruptedExc
         // when
         AtomicReference<CountDownLatch> latch = new AtomicReference<>(new CountDownLatch(1));
         AtomicReference<BundleException> bundleException = new AtomicReference<>();
-        final Watch watch = Bundler.watch(options, (error) -> {
-            error.ifPresent(bundleException::set);
+        final Watch watch = Bundler.watch(options, (r) -> {
+            if (!r.isSuccess()) {
+                bundleException.set(r.bundleException());
+            }
             latch.get().countDown();
         }, true);
 
         // then
-        assertTrue(latch.get().await(2, TimeUnit.SECONDS));
-        assertNull(bundleException.get(), "No error during bundling");
+        assertTrue(latch.get().getCount() == 1, "First build is not using the listener");
+        assertTrue(watch.firstBuildResult().isSuccess(), "first build is success");
+        assertTrue(watch.isAlive(), "process is alive");
         final Path app = watch.workDir().resolve("application-mvnpm.js");
         assertTrue(Files.exists(app));
         final Path distApp = watch.dist().resolve("application-mvnpm.js");
         assertTrue(Files.exists(distApp));
 
         //  when
-        latch.set(new CountDownLatch(1));
         Files.writeString(app, "\nalert(\"foo\");", StandardOpenOption.APPEND);
         assertTrue(latch.get().await(2, TimeUnit.SECONDS));
         assertNull(bundleException.get(), "No error during bundling");
-
         assertTrue(Files.readString(distApp).contains("alert(\"foo\");"));
 
         // then
-        watch.stop();
+        watch.waitForStop();
+
+        assertFalse(watch.isAlive());
     }
 
     @Test
@@ -111,22 +127,23 @@ public void shouldWatchWithError() throws URISyntaxException, IOException, Inter
         // when
         AtomicReference<CountDownLatch> latch = new AtomicReference<>(new CountDownLatch(1));
         AtomicReference<BundleException> bundleException = new AtomicReference<>();
-        final Watch watch = Bundler.watch(options, (error) -> {
-            error.ifPresent(bundleException::set);
+        final Watch watch = Bundler.watch(options, (r) -> {
+            if (!r.isSuccess()) {
+                bundleException.set(r.bundleException());
+            }
             latch.get().countDown();
         }, true);
 
         // then
-        assertTrue(latch.get().await(2, TimeUnit.SECONDS));
-        assertNotNull(bundleException.get(), "Error during bundling");
-        assertTrue(bundleException.get().output().contains("[ERROR] Could not resolve \"\""));
+        assertTrue(latch.get().getCount() == 1, "First build is not using the listener");
+        assertTrue(watch.isAlive(), "process is alive");
+        assertNotNull(watch.firstBuildResult().bundleException(), "Error during bundling");
+        assertTrue(watch.firstBuildResult().bundleException().output().contains("[ERROR] Could not resolve \"\""));
 
         final Path app = watch.workDir().resolve("application-error.js");
         assertTrue(Files.exists(app));
 
         // when
-        bundleException.set(null);
-        latch.set(new CountDownLatch(1));
         Files.writeString(app, "alert(\"foo\");", StandardOpenOption.TRUNCATE_EXISTING);
 
         // then