Repository navigation
fix(reporter): PasteBin upload - timeout, missing Jackson dep, silent failures #70
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,13 +18,22 @@ | |
| 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; | ||
| import java.awt.event.MouseAdapter; | ||
| 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 +43,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 +55,27 @@ public class UploadPanel extends JPanel { | |
|
|
||
| private final Supplier<String> logFileNameSupplier; | ||
|
|
||
| private final long uploadTimeoutSeconds; | ||
|
|
||
| private JButton uploadSkipButton; | ||
|
|
||
| private JLabel titleLabel; | ||
|
|
||
| public UploadPanel(GlobalProperties properties, Supplier<String> logTextSupp, Supplier<String> 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<String> logTextSupp, Supplier<String> 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 +140,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<URL> callable) { | ||
| 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<URL> future = executor.submit(callable); | ||
|
|
||
| Thread watcher = new Thread(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| try { | ||
| awaitUpload(future, uploadTimeoutSeconds, new Consumer<URL>() { | ||
| @Override | ||
| public void accept(URL link) { | ||
| uploadSuccess(link); | ||
| } | ||
| }, new Consumer<Exception>() { | ||
| @Override | ||
| public void accept(Exception e) { | ||
| uploadFailed(e); | ||
| } | ||
| }); | ||
| } finally { | ||
| executor.shutdownNow(); | ||
| } | ||
| } | ||
| }, "Upload-Watcher"); | ||
| watcher.setDaemon(true); | ||
| watcher.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<URL> future, long timeoutSeconds, Consumer<URL> onSuccess, Consumer<Exception> onFailure) { | ||
| try { | ||
| URL link = future.get(timeoutSeconds, TimeUnit.SECONDS); | ||
| onSuccess.accept(link); | ||
| } catch (TimeoutException e) { | ||
| future.cancel(true); | ||
|
There was a problem hiding this comment. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: set -eu
printf '%s\n' '--- changed file outline ---'
ast-grep outline cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java
printf '%s\n' '--- UploadPanel relevant source ---'
cat -n cr-core/src/main/java/org/terasology/crashreporter/pages/UploadPanel.java | sed -n '1,260p'
printf '%s\n' '--- Pastebin symbols and dependency declarations ---'
rg -n --hidden -g '!**/.git/**' 'PastebinPaste|jpastebin|paste\(' .
printf '%s\n' '--- candidate build files ---'
git ls-files | rg '(^|/)(pom\.xml|build\.gradle|build\.gradle\.kts|gradle\.properties|.*dependencies.*)$' | head -80
printf '%s\n' '--- revisions present ---'
git cat-file -e 05f5455c453d72c7b52e358ee1305ec1648e368c^{commit}
git cat-file -e 5b671d220261715b03c931de703d8c4b1069e0f^{commit}Repository: MovingBlocks/CrashReporter Length of output: 14706 🏁 Script executed: set -eu
printf '%s\n' '--- PastebinUploadRunnable ---'
cat -n cr-core/src/main/java/org/terasology/crashreporter/pages/PastebinUploadRunnable.java
printf '%s\n' '--- cr-core dependency context ---'
cat -n cr-core/build.gradle.kts | sed -n '1,100p'
printf '%s\n' '--- available jpastebin artifacts or sources ---'
find . /tmp -type f \( -iname '*jpastebin*' -o -iname '*pastebin*.jar' \) -print 2>/dev/null | head -100
printf '%s\n' '--- repository references to HTTP clients or timeout configuration ---'
rg -n --hidden -g '!**/.git/**' 'HttpURLConnection|URLConnection|OkHttp|Apache Http|setConnectTimeout|setReadTimeout|connectTimeout|readTimeout|SocketTimeout' cr-coreRepository: MovingBlocks/CrashReporter Length of output: 6915 🌐 Web query:
💡 Result: 🌐 Web query:
💡 Result: 🏁 Script executed: set -u
python3 - <<'PY'
from urllib.request import urlopen
urls = [
"https://www.javatips.net/api/jPastebin-master/src/main/java/org/jpaste/pastebin/PastebinPaste.java",
"https://www.javatips.net/api/jPastebin-master/src/main/java/org/jpaste/utils/web/Web.java",
]
for url in urls:
print(f"--- {url} ---")
try:
data = urlopen(url, timeout=10).read().decode("utf-8", "replace")
print(data[:12000])
except Exception as exc:
print(type(exc).__name__ + ": " + str(exc))
PYRepository: MovingBlocks/CrashReporter Length of output: 659 🏁 Script executed: set -u
python3 - <<'PY'
from urllib.request import Request, urlopen
import io, ssl, zipfile
base = "https://repository.jboss.org/nexus/content/repositories/public/org/jpastebin/1.0.1/"
ctx = ssl._create_unverified_context()
for name in ("jpastebin-1.0.1-sources.jar", "jpastebin-1.0.1.jar", "jpastebin-1.0.1.pom"):
url = base + name
print(f"--- {url} ---")
try:
data = urlopen(Request(url, headers={"User-Agent": "Mozilla/5.0"}), context=ctx, timeout=15).read()
print(f"bytes={len(data)}")
if name.endswith(".jar"):
with zipfile.ZipFile(io.BytesIO(data)) as z:
names = [n for n in z.namelist() if n.endswith(".java") or n.endswith(".class")]
print("\n".join(names))
for candidate in names:
if candidate.endswith("Web.java") or candidate.endswith("PastebinPaste.java"):
print(f"--- {candidate} ---")
print(z.read(candidate).decode("utf-8", "replace")[:20000])
else:
print(data.decode("utf-8", "replace")[:12000])
except Exception as exc:
print(type(exc).__name__ + ": " + str(exc))
PYRepository: MovingBlocks/CrashReporter Length of output: 1670 🏁 Script executed: set -u
python3 - <<'PY'
from urllib.request import Request, urlopen
import pathlib, ssl, subprocess, tempfile
url = "https://repository.jboss.org/nexus/content/repositories/public/org/jpastebin/1.0.1/jpastebin-1.0.1.jar"
data = urlopen(Request(url, headers={"User-Agent": "Mozilla/5.0"}),
context=ssl._create_unverified_context(), timeout=15).read()
with tempfile.TemporaryDirectory() as d:
jar = pathlib.Path(d) / "jpastebin-1.0.1.jar"
jar.write_bytes(data)
for cls in (
"org.jpastebin.pastebin.PastebinPaste",
"org.jpastebin.utils.web.Web",
"org.jpastebin.utils.web.Post",
):
print(f"--- {cls} ---")
result = subprocess.run(
["javap", "-classpath", str(jar), "-c", "-p", cls],
text=True, capture_output=True
)
print(result.stdout)
if result.stderr:
print(result.stderr)
PYRepository: MovingBlocks/CrashReporter Length of output: 729 🏁 Script executed: set -u
python3 - <<'PY'
from urllib.request import Request, urlopen
import io, ssl, zipfile
url = "https://repository.jboss.org/nexus/content/repositories/public/org/jpastebin/1.0.1/jpastebin-1.0.1.jar"
data = urlopen(Request(url, headers={"User-Agent": "Mozilla/5.0"}),
context=ssl._create_unverified_context(), timeout=15).read()
with zipfile.ZipFile(io.BytesIO(data)) as z:
for name in (
"org/jpastebin/pastebin/PastebinPaste.class",
"org/jpastebin/utils/web/Web.class",
"org/jpastebin/utils/web/Post.class",
):
print(f"--- {name}: printable strings ---")
raw = z.read(name)
strings = []
current = bytearray()
for b in raw:
if 32 <= b < 127:
current.append(b)
else:
if len(current) >= 4:
strings.append(current.decode("ascii"))
current.clear()
if len(current) >= 4:
strings.append(current.decode("ascii"))
print("\n".join(strings))
PYRepository: MovingBlocks/CrashReporter Length of output: 5381 Configure HTTP timeouts for the Pastebin upload.
Configure connection and read timeouts on the upload transport, or replace it with a transport that supports timeout cancellation. 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Agreed, but not in this PR: jpastebin exposes no timeout hook, so the real fix is replacing it with a direct HttpClient POST (which also removes the Jackson workaround here). Tracked as #71. This PR still strictly improves on master, where the upload hung forever with no feedback. |
||
| onFailure.accept(new IOException( | ||
|
Comment on lines
+195
to
+197
There was a problem hiding this comment. Accurate: cancel(true) cannot stop jpastebin's blocking socket call. Replacing jpastebin with an HttpClient POST that carries connect/read timeouts is tracked as #71 rather than expanding this PR. Holding the retry button until the socket dies would recreate the original hang. |
||
| "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() { | ||
|
|
@@ -165,13 +236,26 @@ public void run() { | |
| }); | ||
| } | ||
|
|
||
| // 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 | ||
| // 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 | ||
| public void run() { | ||
| // 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(); | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.