From 550270cddcf307c9b268fc109340a04b8d8a369d Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 13:00:00 +0200 Subject: [PATCH 1/4] 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 cc30939..a003279 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 @@ -28,6 +28,14 @@ import java.net.URISyntaxException; 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; /** @@ -37,6 +45,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; @@ -47,14 +57,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)); @@ -119,22 +142,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 1c47d3f45208b5d7b8bc5b4d58afa89d79c8d8bd Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 13:14:56 +0200 Subject: [PATCH 2/4] 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 7fcf709..a7dad6a 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 3b730655248dd8d69c0014ceb9416e72d877eeb7 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 13:15:16 +0200 Subject: [PATCH 3/4] 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/GlobalProperties.java | 22 ++++--- .../crashreporter/pages/UploadPanel.java | 13 ++++ .../pages/UploadPanelFailureLoggingTest.java | 65 +++++++++++++++++++ 3 files changed, 91 insertions(+), 9 deletions(-) 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/GlobalProperties.java b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java index 7c7cade..94a062b 100644 --- a/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java +++ b/cr-core/src/main/java/org/terasology/crashreporter/GlobalProperties.java @@ -39,16 +39,20 @@ public enum KEY { public GlobalProperties() { String propsUrl = "/crashreporter.properties"; String defaultPropsUrl = "/crashreporter_defaults.properties"; - try (InputStream stream = CrashReporter.class.getResourceAsStream(defaultPropsUrl)) { - properties.load(stream); - } catch (IOException e) { - // this should never go wrong - System.err.println("Unable to load default properties"); - } - try (InputStream stream = CrashReporter.class.getResourceAsStream(propsUrl)) { - properties.load(stream); + loadIfPresent(defaultPropsUrl); + // Only cr-core's downstream consumers (cr-terasology, cr-destsol, ...) ship this file - + // it's absent when cr-core is used standalone, which getResourceAsStream signals with + // null rather than an IOException, so that has to be checked explicitly. + loadIfPresent(propsUrl); + } + + private void loadIfPresent(String resourceUrl) { + try (InputStream stream = CrashReporter.class.getResourceAsStream(resourceUrl)) { + if (stream != null) { + properties.load(stream); + } } catch (IOException e) { - System.err.println("Unable to load " + propsUrl); + System.err.println("Unable to load " + resourceUrl); } } 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 cc30939..7103d5a 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 @@ -119,6 +119,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); + } + private void upload(final Callable callable) { Runnable runnable = new Runnable() { @@ -169,6 +174,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 55fd78319a5723657ab31e29d2af9bc87cde77c5 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sat, 22 Aug 2026 17:35:31 +0200 Subject: [PATCH 4/4] 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 {