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"); + } +}