From e38bc8f6c33fb9013c30a68c7f0f435894a76327 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 13:00:00 +0200 Subject: [PATCH 1/8] fix(reporter): time out a PasteBin upload instead of hanging forever Found while manually testing the reporter dialog: clicking "PasteBin" disabled the button, showed "please wait", and never resolved either way. PastebinUploadRunnable.call() makes a real HTTP POST via jpastebin with no timeout of its own, and UploadPanel.upload() just ran it on a bare thread and waited unboundedly for callable.call() to return - a slow or unreachable server left no way to tell "still working" from "will never finish". upload() now submits to an ExecutorService and waits at most 30s (configurable via a package-private constructor for tests) via future.get(timeout, SECONDS), cancelling the future and reporting a clear "Upload timed out after 30s" failure if it's not done by then. The wait-and-dispatch logic is split into a small static method, awaitUpload(), that's plain Future/Consumer plumbing with no Swing dependency - lets UploadPanelTest exercise the timeout, success, and underlying-failure-unwrapped-from-ExecutionException paths directly against real Futures, without a button-click harness. Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/pages/UploadPanel.java | 86 ++++++++++++-- .../crashreporter/pages/UploadPanelTest.java | 108 ++++++++++++++++++ 2 files changed, 186 insertions(+), 8 deletions(-) create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java index 5f85028..98900ac 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java @@ -25,6 +25,14 @@ import java.io.IOException; import java.net.URL; import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.function.Consumer; import java.util.function.Supplier; /** @@ -34,6 +42,8 @@ public class UploadPanel extends JPanel { private static final long serialVersionUID = -8247883237201535146L; + private static final long DEFAULT_UPLOAD_TIMEOUT_SECONDS = 30; + private JButton uploadPasteBinButton; private boolean isComplete; private URL uploadURL; @@ -44,14 +54,27 @@ public class UploadPanel extends JPanel { private final Supplier logFileNameSupplier; + private final long uploadTimeoutSeconds; + private JButton uploadSkipButton; private JLabel titleLabel; public UploadPanel(GlobalProperties properties, Supplier logTextSupp, Supplier logFileNameSupp) { + this(properties, logTextSupp, logFileNameSupp, DEFAULT_UPLOAD_TIMEOUT_SECONDS); + } + + /** + * @param uploadTimeoutSeconds how long {@link #upload} waits for the upload {@link Callable} before treating it + * as failed - package-private constructor so tests can use a short timeout instead of + * {@link #DEFAULT_UPLOAD_TIMEOUT_SECONDS}. + */ + UploadPanel(GlobalProperties properties, Supplier logTextSupp, Supplier logFileNameSupp, + long uploadTimeoutSeconds) { this.textSupplier = logTextSupp; this.logFileNameSupplier = logFileNameSupp; + this.uploadTimeoutSeconds = uploadTimeoutSeconds; setLayout(new BorderLayout(50, 20)); statusLabel = new JLabel(I18N.getMessage("noUpload"), SwingConstants.RIGHT); statusLabel.setFont(statusLabel.getFont().deriveFont(Font.BOLD)); @@ -116,22 +139,69 @@ public URL getUploadedFileURL() { return uploadURL; } + /** + * Runs {@code callable} on its own thread and waits up to {@link #uploadTimeoutSeconds} for it + * to finish - {@code PastebinUploadRunnable} makes a real HTTP call with no timeout of its own, + * so without one here a slow or unreachable server leaves the button disabled and the status + * label reading "please wait" forever, with no way for the user to tell the difference between + * "still working" and "will never finish". + */ private void upload(final Callable callable) { - Runnable runnable = new Runnable() { + final ExecutorService executor = Executors.newSingleThreadExecutor(new ThreadFactory() { + @Override + public Thread newThread(Runnable r) { + Thread thread = new Thread(r, "Upload"); + thread.setDaemon(true); + return thread; + } + }); + final Future future = executor.submit(callable); + Thread watcher = new Thread(new Runnable() { @Override public void run() { try { - URL link = callable.call(); - uploadSuccess(link); - } catch (Exception e) { - uploadFailed(e); + awaitUpload(future, uploadTimeoutSeconds, new Consumer() { + @Override + public void accept(URL link) { + uploadSuccess(link); + } + }, new Consumer() { + @Override + public void accept(Exception e) { + uploadFailed(e); + } + }); + } finally { + executor.shutdownNow(); } } - }; + }, "Upload-Watcher"); + watcher.setDaemon(true); + watcher.start(); + } - Thread thread = new Thread(runnable, "Upload"); - thread.start(); + /** + * Waits up to {@code timeoutSeconds} for {@code future}, then dispatches to exactly one of the + * two callbacks - split out from {@link #upload} as a plain, Swing-free method so the timeout + * and exception-unwrapping logic can be tested directly against a real {@link Future} without + * needing a full {@code UploadPanel}/button-click harness. + */ + static void awaitUpload(Future future, long timeoutSeconds, Consumer onSuccess, Consumer onFailure) { + try { + URL link = future.get(timeoutSeconds, TimeUnit.SECONDS); + onSuccess.accept(link); + } catch (TimeoutException e) { + future.cancel(true); + onFailure.accept(new IOException( + "Upload timed out after " + timeoutSeconds + "s - the server may be unreachable", e)); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + onFailure.accept(cause instanceof Exception ? (Exception) cause : e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + onFailure.accept(e); + } } private void updateStatus() { diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java new file mode 100644 index 0000000..67580ac --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java @@ -0,0 +1,108 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Regression tests for the PasteBin upload hang found while manually testing the reporter dialog: + * {@code PastebinUploadRunnable} makes a real HTTP call with no timeout of its own, so a slow or + * unreachable server left the upload button disabled and the status label reading "please wait" + * forever, with no way to tell "still working" from "never finishing". + */ +class UploadPanelTest { + + private ExecutorService executor; + + @AfterEach + void shutdown() { + if (executor != null) { + executor.shutdownNow(); + } + } + + @Test + void aSlowUploadFailsWithATimeoutInsteadOfHangingForever() throws Exception { + executor = Executors.newSingleThreadExecutor(); + Future future = executor.submit(new Callable() { + @Override + public URL call() throws InterruptedException, MalformedURLException { + // Longer than the 1-second timeout below - simulates the observed hang. + Thread.sleep(5_000); + return new URL("https://pastebin.com/never-reached"); + } + }); + + AtomicReference successResult = new AtomicReference<>(); + AtomicReference failureResult = new AtomicReference<>(); + CountDownLatch done = new CountDownLatch(1); + + UploadPanel.awaitUpload(future, 1, link -> { + successResult.set(link); + done.countDown(); + }, e -> { + failureResult.set(e); + done.countDown(); + }); + + assertTrue(done.await(1, TimeUnit.SECONDS), "awaitUpload must return once its own timeout elapses"); + assertNull(successResult.get(), "a timed-out upload must not report success"); + assertTrue(failureResult.get() instanceof IOException, "expected a timeout to surface as an IOException, got: " + failureResult.get()); + assertTrue(failureResult.get().getMessage().contains("timed out"), + "expected a message naming the timeout, got: " + failureResult.get().getMessage()); + assertTrue(future.isCancelled(), "the underlying upload task should be cancelled once it's timed out"); + } + + @Test + void aFastUploadReportsSuccess() throws Exception { + executor = Executors.newSingleThreadExecutor(); + final URL expected = new URL("https://pastebin.com/abc123"); + Future future = executor.submit(new Callable() { + @Override + public URL call() { + return expected; + } + }); + + AtomicReference successResult = new AtomicReference<>(); + UploadPanel.awaitUpload(future, 5, successResult::set, e -> fail("expected success, got: " + e)); + + assertEquals(expected, successResult.get()); + } + + @Test + void anUnderlyingFailureIsUnwrappedFromExecutionException() throws Exception { + executor = Executors.newSingleThreadExecutor(); + final IOException realCause = new IOException("invalid API key"); + Future future = executor.submit(new Callable() { + @Override + public URL call() throws IOException { + throw realCause; + } + }); + + AtomicReference failureResult = new AtomicReference<>(); + UploadPanel.awaitUpload(future, 5, link -> fail("expected failure"), failureResult::set); + + assertEquals(realCause, failureResult.get(), + "expected the real cause unwrapped from ExecutionException, not the wrapper itself"); + } +} From 0002a9b02fdb7a262c07ae60c13b20ba242727f3 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 13:14:56 +0200 Subject: [PATCH 2/8] fix(reporter): declare jpastebin's Jackson dependency explicitly Clicking "PasteBin" threw NoClassDefFoundError: com/fasterxml/jackson/core/type/TypeReference the first time it actually reached a Jackson class - reproduced directly by calling PastebinUploadRunnable.call() outside the dialog. jpastebin's own embedded META-INF/maven/org/jpastebin/pom.xml (inside the jar) pins jackson-databind/jackson-core/jackson-annotations 2.9.7, but the POM Gradle actually resolves for org:jpastebin:1.0.1 from the JBoss repo is an empty Nexus-generated stub with no at all - so Gradle never pulled Jackson in. Declares all three explicitly via the jackson-bom platform rather than three separately-pinned versions: jackson-annotations renumbered its own versioning away from core/databind's x.y.z scheme starting at 2.20, so hand-pinning all three to the same string breaks depending on which release line you pick. The BOM keeps them resolvable together regardless. 2.9.7 is a 2018 release with known CVEs; Jackson's 2.x line keeps this level of API (ObjectMapper, TypeReference, annotations) stable, so the current release is a safe drop-in rather than matching jpastebin's old pin. Verified end to end: a direct call to PastebinUploadRunnable now succeeds against the real API instead of throwing. Co-Authored-By: Claude Sonnet 5 --- cr-core/build.gradle.kts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cr-core/build.gradle.kts b/cr-core/build.gradle.kts index 99f734b..f649779 100644 --- a/cr-core/build.gradle.kts +++ b/cr-core/build.gradle.kts @@ -62,6 +62,20 @@ dependencies { pmd("net.sourceforge.pmd:pmd-java:7.0.0-rc4") implementation("org:jpastebin:1.0.1") + // jpastebin needs these at runtime (see its own embedded META-INF/maven/org/jpastebin/pom.xml, + // which pins Jackson 2.9.7) but the POM Gradle actually resolves from the JBoss repo is an + // empty Nexus-generated stub with no at all - so without declaring these + // ourselves, PastebinUploadRunnable.call() throws NoClassDefFoundError the first time it + // touches a Jackson class, only when someone actually clicks "Upload". 2.9.7 is a 2018 release + // with known CVEs; Jackson's 2.x line keeps this level of API (ObjectMapper, TypeReference, + // annotations) stable, so a current release is a safe drop-in rather than matching the old pin. + // The BOM (not three separately-pinned versions) because jackson-annotations renumbered its own + // versioning away from core/databind's x.y.z scheme starting at 2.20 - the BOM is what keeps the + // three resolvable together regardless of a given module's own version string. + implementation(platform("com.fasterxml.jackson:jackson-bom:2.22.2")) + implementation("com.fasterxml.jackson.core:jackson-databind") + implementation("com.fasterxml.jackson.core:jackson-core") + implementation("com.fasterxml.jackson.core:jackson-annotations") implementation("org.apache.httpcomponents:httpclient:4.5.13") implementation("org.apache.httpcomponents:httpmime:4.5.13") From 500c3217717807ef9047a409e2333f63f8d5c213 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 13:15:16 +0200 Subject: [PATCH 3/8] fix(reporter): always print upload failures to stderr, not just a popup uploadFailed() only ever showed a JOptionPane - nothing else in this codebase logs upload failures anywhere. That popup reaches whoever happens to be watching the screen at that exact moment and leaves no trace at all once dismissed; whoever launched the process (a script, a supervisor, a developer tailing output) has no way to find out what happened after the fact. This is exactly what made the Jackson NoClassDefFoundError above hard to pin down in the first place - it only ever appeared as a popup. e.printStackTrace(System.err) now runs unconditionally before the dialog, on the same thread, so it can't get lost even if the JOptionPane is dismissed instantly. New package-private uploadForTesting() hook (bypasses the real ActionListener/network call so UploadPanelFailureLoggingTest can drive a failure directly) plus the same GlobalProperties NPE guard used elsewhere in this repo (needed just to construct GlobalProperties() in cr-core's own test classpath - see the sibling PRs). Co-Authored-By: Claude Sonnet 5 --- .../crashreporter/pages/UploadPanel.java | 13 ++++ .../pages/UploadPanelFailureLoggingTest.java | 65 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java index 98900ac..c59c61d 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java @@ -139,6 +139,11 @@ public URL getUploadedFileURL() { return uploadURL; } + /** Package-private test hook - equivalent to a real "PasteBin" click, but with a caller-supplied Callable. */ + void uploadForTesting(Callable callable) { + upload(callable); + } + /** * Runs {@code callable} on its own thread and waits up to {@link #uploadTimeoutSeconds} for it * to finish - {@code PastebinUploadRunnable} makes a real HTTP call with no timeout of its own, @@ -236,6 +241,14 @@ public void run() { } private void uploadFailed(final Exception e) { + // Printed unconditionally, not just shown in the dialog below: a JOptionPane only reaches + // whoever is watching the screen at that exact moment, and leaves no trace at all once + // it's dismissed - nothing else in this codebase logs upload failures anywhere. Whoever + // launched this process (a script, a supervisor, a developer tailing output) needs to be + // able to find out what happened after the fact, not just the person who happened to be + // looking right then. + e.printStackTrace(System.err); + SwingUtilities.invokeLater(new Runnable() { @Override diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java new file mode 100644 index 0000000..f20c503 --- /dev/null +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java @@ -0,0 +1,65 @@ +// Copyright 2026 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.crashreporter.pages; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.terasology.crashreporter.GlobalProperties; + +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.Callable; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression test for an upload failure that previously left no trace anywhere once its + * {@code JOptionPane} was dismissed - found while manually testing the reporter dialog: a real + * exception (a missing-Jackson {@code NoClassDefFoundError} - see the jackson-bom fix elsewhere in + * this commit) only ever showed up in a popup, with nothing printed to stderr for whoever launched + * the process to find afterward. + */ +class UploadPanelFailureLoggingTest { + + private PrintStream originalErr; + private ByteArrayOutputStream capturedErr; + + @BeforeEach + void redirectStderr() { + originalErr = System.err; + capturedErr = new ByteArrayOutputStream(); + System.setErr(new PrintStream(capturedErr, true, StandardCharsets.UTF_8)); + } + + @AfterEach + void restoreStderr() { + System.setErr(originalErr); + } + + @Test + void aFailedUploadIsPrintedToStderrNotJustShownInAPopup() throws InterruptedException { + UploadPanel panel = new UploadPanel(new GlobalProperties(), () -> "log text", () -> "log.txt"); + + final Exception cause = new IllegalStateException("upload failed: missing Jackson class"); + panel.uploadForTesting(new Callable() { + @Override + public URL call() throws Exception { + throw cause; + } + }); + + long deadline = System.currentTimeMillis() + 2000; + while (capturedErr.size() == 0 && System.currentTimeMillis() < deadline) { + Thread.sleep(20); + } + + String stderr = capturedErr.toString(StandardCharsets.UTF_8); + assertTrue(stderr.contains("IllegalStateException"), "Expected the exception type on stderr, got: " + stderr); + assertTrue(stderr.contains("upload failed: missing Jackson class"), + "Expected the exception message on stderr, got: " + stderr); + } +} From a45220be4e2c9471cf0fb76e94e0a94c9914d7e6 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 17:35:31 +0200 Subject: [PATCH 4/8] docs(reporter): clarify UploadPanelTest never touches the network Addresses @BenjaminAmos's review concern on #64 (now folded into this PR): these tests are already fully offline - PastebinUploadRunnable is never instantiated, and new URL(...) only parses a string, it never opens a connection. Making that explicit in the class javadoc so it isn't mistaken for a real network-touching test again. Co-Authored-By: Claude Sonnet 5 --- .../org/terasology/crashreporter/pages/UploadPanelTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java index 67580ac..b22031c 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelTest.java @@ -27,6 +27,11 @@ * {@code PastebinUploadRunnable} makes a real HTTP call with no timeout of its own, so a slow or * unreachable server left the upload button disabled and the status label reading "please wait" * forever, with no way to tell "still working" from "never finishing". + * + *

Fully offline: {@code PastebinUploadRunnable} is never instantiated here, so no test makes a + * real HTTP call. "Slow"/"failing" uploads are hand-written {@link Callable}s (sleep-then-return, + * throw); the {@code pastebin.com} URLs below are only ever passed to {@link URL#URL(String)}, + * which parses a string and never opens a connection. */ class UploadPanelTest { From 111b34343c2b62cb32ddebde7e072cdb5ac498de Mon Sep 17 00:00:00 2001 From: soloturn Date: Wed, 26 Aug 2026 20:10:11 +0200 Subject: [PATCH 5/8] fix(reporter): drop the test-exclusive uploadForTesting seam --- .../crashreporter/pages/UploadPanel.java | 8 ++------ .../pages/UploadPanelFailureLoggingTest.java | 18 +++--------------- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java index c59c61d..6f39faf 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java @@ -139,11 +139,6 @@ public URL getUploadedFileURL() { return uploadURL; } - /** Package-private test hook - equivalent to a real "PasteBin" click, but with a caller-supplied Callable. */ - void uploadForTesting(Callable callable) { - upload(callable); - } - /** * Runs {@code callable} on its own thread and waits up to {@link #uploadTimeoutSeconds} for it * to finish - {@code PastebinUploadRunnable} makes a real HTTP call with no timeout of its own, @@ -240,7 +235,8 @@ public void run() { }); } - private void uploadFailed(final Exception e) { + // Package-private so tests can call it directly, no test-only seam needed. + void uploadFailed(final Exception e) { // Printed unconditionally, not just shown in the dialog below: a JOptionPane only reaches // whoever is watching the screen at that exact moment, and leaves no trace at all once // it's dismissed - nothing else in this codebase logs upload failures anywhere. Whoever diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java index f20c503..d34796a 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java @@ -10,9 +10,7 @@ import java.io.ByteArrayOutputStream; import java.io.PrintStream; -import java.net.URL; import java.nio.charset.StandardCharsets; -import java.util.concurrent.Callable; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -41,21 +39,11 @@ void restoreStderr() { } @Test - void aFailedUploadIsPrintedToStderrNotJustShownInAPopup() throws InterruptedException { + void aFailedUploadIsPrintedToStderrNotJustShownInAPopup() { UploadPanel panel = new UploadPanel(new GlobalProperties(), () -> "log text", () -> "log.txt"); - final Exception cause = new IllegalStateException("upload failed: missing Jackson class"); - panel.uploadForTesting(new Callable() { - @Override - public URL call() throws Exception { - throw cause; - } - }); - - long deadline = System.currentTimeMillis() + 2000; - while (capturedErr.size() == 0 && System.currentTimeMillis() < deadline) { - Thread.sleep(20); - } + // Synchronous stderr print, no thread/timing dance needed. + panel.uploadFailed(new IllegalStateException("upload failed: missing Jackson class")); String stderr = capturedErr.toString(StandardCharsets.UTF_8); assertTrue(stderr.contains("IllegalStateException"), "Expected the exception type on stderr, got: " + stderr); From 05f5455c453d72c7b52e358ee1305ec1648e368c Mon Sep 17 00:00:00 2001 From: Cervator Date: Sat, 26 Sep 2026 23:05:58 -0400 Subject: [PATCH 6/8] fix(reporter): close log readers and the folder watch so tests pass on Windows `ErrorMessagePanel` never released its per-tab `RandomAccessFile` or the `WatchService`, so on Windows JUnit could not delete the `@TempDir` and all three panel tests failed with `Failed to delete temp directory`. Linux CI never noticed. The dialog now closes them via `removeNotify` on dispose. Co-Authored-By: Claude Fable 5.1 --- .../pages/ErrorMessagePanel.java | 46 ++++++++++++++++++- .../crashreporter/pages/LogUpdateWorker.java | 21 +++++++-- .../pages/ErrorMessagePanelTabOrderTest.java | 14 +++--- .../pages/ErrorMessagePanelTest.java | 39 +++++++++++----- 4 files changed, 99 insertions(+), 21 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java index 952db4a..3eb25ef 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java @@ -23,6 +23,7 @@ import java.awt.Font; import java.beans.PropertyChangeEvent; import java.beans.PropertyChangeListener; +import java.io.Closeable; import java.io.File; import java.io.IOException; import java.io.PrintWriter; @@ -41,8 +42,13 @@ /** * Shows the error message plus stack trace. + * + *

Holds a {@link RandomAccessFile} per log tab (to tail appended lines) and a directory watch, + * for as long as it lives. {@link #close()} releases them; the dialog triggers it through + * {@link #removeNotify()} on dispose, and tests must call it before their temp folder is cleaned up - + * Windows refuses to delete a directory while either handle is open. */ -public class ErrorMessagePanel extends JPanel { +public class ErrorMessagePanel extends JPanel implements Closeable { private static final long serialVersionUID = 8449689452512733452L; @@ -182,6 +188,44 @@ public int compare(Path p0, Path p1) { }); } + @Override + public void close() throws IOException { + IOException first = null; + try { + logUpdateWorker.close(); + } catch (IOException e) { + first = e; + } + for (RandomAccessFile logReader : logReaders) { + try { + logReader.close(); + } catch (IOException e) { + if (first == null) { + first = e; + } + } + } + logReaders.clear(); + if (first != null) { + throw first; + } + } + + /** + * Swing calls this when the panel leaves a displayable hierarchy - for this dialog, that is + * dispose. Card-layout page switches do not remove the panel, so tailing keeps working while + * the user moves between pages. + */ + @Override + public void removeNotify() { + super.removeNotify(); + try { + close(); + } catch (IOException e) { + e.printStackTrace(System.err); + } + } + @Override public void setVisible(boolean aFlag) { super.setVisible(aFlag); diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java index fb319c4..901b874 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java @@ -5,7 +5,9 @@ import com.sun.nio.file.SensitivityWatchEventModifier; import javax.swing.SwingWorker; +import java.io.Closeable; import java.io.IOException; +import java.nio.file.ClosedWatchServiceException; import java.nio.file.FileSystems; import java.nio.file.Path; import java.nio.file.WatchEvent; @@ -20,7 +22,7 @@ * It watches change in the background thread, see {@code doInBackground} method. * It processes event in EDT thread, see {@code process} method. */ -public class LogUpdateWorker extends SwingWorker> { +public class LogUpdateWorker extends SwingWorker> implements Closeable { public static final String CREATED = "CREATE_LOG"; public static final String MODIFIED = "MODIFIED_LOG"; @@ -45,8 +47,8 @@ protected Void doInBackground() throws Exception { WatchKey key; try { key = watchService.take(); - } catch (InterruptedException e) { - e.printStackTrace(); + } catch (InterruptedException | ClosedWatchServiceException e) { + // Either way someone asked us to stop - see close(). return null; } @@ -67,6 +69,19 @@ protected Void doInBackground() throws Exception { return null; } + /** + * Stops watching and releases the {@link WatchService}. On Windows an open watch on a + * directory keeps that directory from being deleted, so anything that owns a panel with a + * worker (the dialog, a test's temp folder) needs this to run before the folder goes away. + */ + @Override + public void close() throws IOException { + cancel(true); + if (watchService != null) { + watchService.close(); + } + } + @Override protected void process(List> chunks) { super.process(chunks); diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java index f418822..9b0e5cb 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTabOrderTest.java @@ -31,12 +31,14 @@ void tabsAreOrderedAlphabeticallyRegardlessOfCreationOrder(@TempDir Path logFold Thread.sleep(10); writeLog(logFolder, "Terasology-init.log", "INIT"); - ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), - logFolder, CrashReporter.MODE.CRASH_REPORTER); - - List titles = panel.getTabTitles(); - assertEquals(Arrays.asList("Terasology-init.log", "Terasology-menu.log"), titles, - "Expected tabs in alphabetical order regardless of which file was created first, got: " + titles); + // Closed before returning so the panel's open readers and folder watch don't stop JUnit + // deleting the temp folder on Windows. + try (ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), + logFolder, CrashReporter.MODE.CRASH_REPORTER)) { + List titles = panel.getTabTitles(); + assertEquals(Arrays.asList("Terasology-init.log", "Terasology-menu.log"), titles, + "Expected tabs in alphabetical order regardless of which file was created first, got: " + titles); + } } private static void writeLog(Path folder, String name, String content) throws IOException { diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTest.java index 34e0e4f..8032996 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/ErrorMessagePanelTest.java @@ -18,6 +18,9 @@ /** * Regression tests for #53: PasteBin upload only included whichever log tab happened to be * selected, silently dropping every other log file present. + * + *

Every panel is closed before the test returns: it keeps a reader open per log file plus a + * watch on the folder, and on Windows that stops JUnit from deleting the {@code @TempDir}. */ class ErrorMessagePanelTest { @@ -26,27 +29,41 @@ void getLogIncludesEveryLogFileNotJustTheSelectedTab(@TempDir Path logFolder) th writeLog(logFolder, "Terasology-init.log", "INIT LOG CONTENT"); writeLog(logFolder, "Terasology-menu.log", "MENU LOG CONTENT"); - ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), - logFolder, CrashReporter.MODE.CRASH_REPORTER); - - String log = panel.getLog(); - assertTrue(log.contains("INIT LOG CONTENT"), "Expected the init log's content in the combined log, got: " + log); - assertTrue(log.contains("MENU LOG CONTENT"), "Expected the menu log's content in the combined log, got: " + log); - assertTrue(log.contains("Terasology-init.log"), "Expected a header naming the init log tab, got: " + log); - assertTrue(log.contains("Terasology-menu.log"), "Expected a header naming the menu log tab, got: " + log); + try (ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), + logFolder, CrashReporter.MODE.CRASH_REPORTER)) { + String log = panel.getLog(); + assertTrue(log.contains("INIT LOG CONTENT"), "Expected the init log's content in the combined log, got: " + log); + assertTrue(log.contains("MENU LOG CONTENT"), "Expected the menu log's content in the combined log, got: " + log); + assertTrue(log.contains("Terasology-init.log"), "Expected a header naming the init log tab, got: " + log); + assertTrue(log.contains("Terasology-menu.log"), "Expected a header naming the menu log tab, got: " + log); + } } @Test void getLogStillWorksWithASingleLogFile(@TempDir Path logFolder) throws IOException { writeLog(logFolder, "Terasology.log", "SOLO LOG CONTENT"); + try (ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), + logFolder, CrashReporter.MODE.CRASH_REPORTER)) { + assertTrue(panel.getLog().contains("SOLO LOG CONTENT")); + } + } + + @Test + void closeReleasesTheLogFilesSoTheFolderCanBeDeleted(@TempDir Path logFolder) throws IOException { + Path log = writeLog(logFolder, "Terasology.log", "LOG CONTENT"); + ErrorMessagePanel panel = new ErrorMessagePanel(new GlobalProperties(), new RuntimeException("boom"), logFolder, CrashReporter.MODE.CRASH_REPORTER); + panel.close(); - assertTrue(panel.getLog().contains("SOLO LOG CONTENT")); + // Only Windows actually refuses this while a reader is open, so on other platforms this + // documents the contract rather than proving it. + Files.delete(log); + assertTrue(Files.notExists(log), "Expected the log file to be deletable once the panel is closed"); } - private static void writeLog(Path folder, String name, String content) throws IOException { - Files.write(folder.resolve(name), content.getBytes(StandardCharsets.UTF_8)); + private static Path writeLog(Path folder, String name, String content) throws IOException { + return Files.write(folder.resolve(name), content.getBytes(StandardCharsets.UTF_8)); } } From e6195c57359f2a2e7236f68443b1c2a265ef52d3 Mon Sep 17 00:00:00 2001 From: Cervator Date: Sun, 27 Sep 2026 16:07:16 -0400 Subject: [PATCH 7/8] =?UTF-8?q?fix(reporter):=20PR=20#70=20review=20round?= =?UTF-8?q?=201=20=E2=80=94=20guard=20late=20log=20events,=20keep=20tests?= =?UTF-8?q?=20headless=20and=20Java=208?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit and Copilot, round 1. `SwingWorker` can still deliver already-published events after `cancel()`, so a late callback could reopen a reader after `close()`; a closed flag now drops them. The failure test drove a real `JOptionPane`; tests run headless and `uploadFailed` skips the dialog there. Test used Java 10 charset overloads against a Java 8 target. Co-Authored-By: Claude Fable 5.1 --- cr-core/build.gradle.kts | 3 +++ .../crashreporter/pages/ErrorMessagePanel.java | 11 +++++++++++ .../crashreporter/pages/LogUpdateWorker.java | 4 ++++ .../terasology/crashreporter/pages/UploadPanel.java | 9 +++++++-- .../pages/UploadPanelFailureLoggingTest.java | 10 ++++++---- 5 files changed, 31 insertions(+), 6 deletions(-) diff --git a/cr-core/build.gradle.kts b/cr-core/build.gradle.kts index f649779..07f937a 100644 --- a/cr-core/build.gradle.kts +++ b/cr-core/build.gradle.kts @@ -214,6 +214,9 @@ tasks.javadoc { tasks.test { useJUnitPlatform() + // Same environment as CI, on every machine: no test may pop a real dialog on a developer's + // screen, and headless is where a stray JOptionPane fails loudly instead of blocking. + systemProperty("java.awt.headless", "true") // InteractiveTestCases is a manual runner (see runInteractiveTest), not an automated test - // there are no @Test methods in this project. failOnNoDiscoveredTests.set(false) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java index 3eb25ef..6adb31c 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java @@ -61,6 +61,10 @@ public class ErrorMessagePanel extends JPanel implements Closeable { // logReaders is the list of each log file's reader private final List logReaders = Lists.newArrayList(); + // SwingWorker may still deliver already-published events to the EDT after cancel(); this + // stops those late callbacks from reopening a reader or indexing into the cleared list. + private volatile boolean closed; + /** * @param exception the exception to display * @param logFileFolder the folder that contains the relevant log files @@ -190,6 +194,7 @@ public int compare(Path p0, Path p1) { @Override public void close() throws IOException { + closed = true; IOException first = null; try { logUpdateWorker.close(); @@ -341,6 +346,9 @@ private static String readLogFileContent(Path logFile) { * @param newLogPath path of the new log file */ private void addNewTab(Path logFileFolder, Path newLogPath) { + if (closed) { + return; + } logFiles.add(newLogPath); sortLogFiles(logFiles); int index = logFiles.indexOf(newLogPath); @@ -368,6 +376,9 @@ private void addNewTab(Path logFileFolder, Path newLogPath) { * @param changedLogPath path of the changed log file */ private void updateLog(Path changedLogPath) { + if (closed) { + return; + } int index = logFiles.indexOf(changedLogPath); if (index != -1) { RandomAccessFile logReader = logReaders.get(index); diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java index 901b874..bc290e2 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/LogUpdateWorker.java @@ -84,6 +84,10 @@ public void close() throws IOException { @Override protected void process(List> chunks) { + if (isCancelled()) { + // publish() may have queued this before close() ran; the panel is gone. + return; + } super.process(chunks); for (WatchEvent event : chunks) { WatchEvent.Kind kind = event.kind(); diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java index 6f39faf..65fa271 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java @@ -18,6 +18,7 @@ import java.awt.BorderLayout; import java.awt.Cursor; import java.awt.Font; +import java.awt.GraphicsEnvironment; import java.awt.GridLayout; import java.awt.event.ActionEvent; import java.awt.event.ActionListener; @@ -249,8 +250,12 @@ void uploadFailed(final Exception e) { @Override public void run() { - String uploadFailed = I18N.getMessage("uploadFailed"); - JOptionPane.showMessageDialog(null, e.getLocalizedMessage(), uploadFailed, JOptionPane.ERROR_MESSAGE); + // Headless (CI, unit tests): stderr above is the whole report; a modal here would + // throw HeadlessException on the EDT or, with a display, block the test JVM. + if (!GraphicsEnvironment.isHeadless()) { + String uploadFailed = I18N.getMessage("uploadFailed"); + JOptionPane.showMessageDialog(null, e.getLocalizedMessage(), uploadFailed, JOptionPane.ERROR_MESSAGE); + } uploadPasteBinButton.setEnabled(true); updateStatus(); } diff --git a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java index d34796a..1a8e476 100644 --- a/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java +++ b/cr-core/src/test/java/org/terasology/crashreporter/pages/UploadPanelFailureLoggingTest.java @@ -10,6 +10,7 @@ import java.io.ByteArrayOutputStream; import java.io.PrintStream; +import java.io.UnsupportedEncodingException; import java.nio.charset.StandardCharsets; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -27,10 +28,11 @@ class UploadPanelFailureLoggingTest { private ByteArrayOutputStream capturedErr; @BeforeEach - void redirectStderr() { + void redirectStderr() throws UnsupportedEncodingException { originalErr = System.err; capturedErr = new ByteArrayOutputStream(); - System.setErr(new PrintStream(capturedErr, true, StandardCharsets.UTF_8)); + // String charset overloads: the Charset ones are Java 10+, and cr-core targets Java 8. + System.setErr(new PrintStream(capturedErr, true, StandardCharsets.UTF_8.name())); } @AfterEach @@ -39,13 +41,13 @@ void restoreStderr() { } @Test - void aFailedUploadIsPrintedToStderrNotJustShownInAPopup() { + void aFailedUploadIsPrintedToStderrNotJustShownInAPopup() throws UnsupportedEncodingException { UploadPanel panel = new UploadPanel(new GlobalProperties(), () -> "log text", () -> "log.txt"); // Synchronous stderr print, no thread/timing dance needed. panel.uploadFailed(new IllegalStateException("upload failed: missing Jackson class")); - String stderr = capturedErr.toString(StandardCharsets.UTF_8); + String stderr = capturedErr.toString(StandardCharsets.UTF_8.name()); assertTrue(stderr.contains("IllegalStateException"), "Expected the exception type on stderr, got: " + stderr); assertTrue(stderr.contains("upload failed: missing Jackson class"), "Expected the exception message on stderr, got: " + stderr); From d9e4f3649f9b5504147d5ac1e853ab2f8656efb6 Mon Sep 17 00:00:00 2001 From: Cervator Date: Sun, 27 Sep 2026 21:44:32 -0400 Subject: [PATCH 8/8] =?UTF-8?q?fix(reporter):=20PR=20#70=20review=20round?= =?UTF-8?q?=202=20=E2=80=94=20serialize=20ErrorMessagePanel.close()=20with?= =?UTF-8?q?=20log=20callbacks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit round 2. The dialog closes on the EDT where callbacks already run, but a test closes from its own thread, so the closed check and the clear could interleave. `synchronized` on the three methods; nothing inside waits on the EDT, so no deadlock path. Co-Authored-By: Claude Fable 5.1 --- .../crashreporter/pages/ErrorMessagePanel.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java index 6adb31c..3f35b88 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/pages/ErrorMessagePanel.java @@ -63,7 +63,8 @@ public class ErrorMessagePanel extends JPanel implements Closeable { // SwingWorker may still deliver already-published events to the EDT after cancel(); this // stops those late callbacks from reopening a reader or indexing into the cleared list. - private volatile boolean closed; + // Guarded by this panel's monitor, see close(). + private boolean closed; /** * @param exception the exception to display @@ -192,8 +193,10 @@ public int compare(Path p0, Path p1) { }); } + // synchronized with addNewTab/updateLog: the dialog closes on the EDT, where callbacks already + // run, but a test closes from its own thread while a queued callback may be mid-flight. @Override - public void close() throws IOException { + public synchronized void close() throws IOException { closed = true; IOException first = null; try { @@ -345,7 +348,7 @@ private static String readLogFileContent(Path logFile) { * @param logFileFolder log folder * @param newLogPath path of the new log file */ - private void addNewTab(Path logFileFolder, Path newLogPath) { + private synchronized void addNewTab(Path logFileFolder, Path newLogPath) { if (closed) { return; } @@ -375,7 +378,7 @@ private void addNewTab(Path logFileFolder, Path newLogPath) { * Update log information * @param changedLogPath path of the changed log file */ - private void updateLog(Path changedLogPath) { + private synchronized void updateLog(Path changedLogPath) { if (closed) { return; }