Fix: Stuck loading screen during Mono/XAudio install#1395
Conversation
- Works on all devices - No effect on gameplay — these commands only run at first boot, container swap, booting screen, not during a game session FIX: Now the app just reads one stream from top to bottom. There's nothing to deadlock against because there's only one pipe, one reader, no coordination needed. Merges stderr into stdout at the OS level — only one pipe. It can never deadlock with itself Issue: Wine logging + Mono install = child writes both stdout and stderr simultaneously. Stderr fills its 64 KB OS pipe buffer WHILE the parent is blocked reading stdout → child blocks on write(stderr) → parent never sees stdout EOF → both sides wait forever. process.waitFor() with no timeout means it sits there permanently. When installing Mono or extracting XAudio DLLs during first boot, some users would get permanently stuck on the loading screen — especially when wine debug logging was turned on, or when setting up a new Proton version. The root cause was a pipe deadlock: wine writes a large amount of debug output to stderr during installation. The OS pipe buffer (64 KB) would fill up, causing wine to freeze waiting for someone to read it. Meanwhile, the app was waiting for wine to finish — so both sides were stuck waiting on each other, forever.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ProcessHelper.execWithOutput and changes Bionic/Glibc program launchers to delegate shell-command execution and unified stdout/stderr capture to that helper, replacing local Runtime.exec stream handling. ChangesProcess Execution Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a first-boot / container-setup hang by addressing a stdout/stderr pipe deadlock when running Wine-based setup commands (Mono/XAudio), and reduces log spam from very large Wine debug output during Mono install.
Changes:
- Switched
BionicProgramLauncherComponent.execShellCommandtoProcessBuilderwithredirectErrorStream(true)and a single merged output reader to prevent stdout/stderr deadlocks. - Added Mono install logging cap (head/tail snippet) in
XServerScreento avoid flooding logs with massive Wine debug output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java | Uses ProcessBuilder with merged stderr/stdout to avoid pipe deadlocks when capturing command output. |
| app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt | Limits Mono install log output to a short snippet (head/tail) to reduce log volume. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 524-528: The current read loop in BionicProgramLauncherComponent
drains process.getInputStream() into an unbounded StringBuilder named output,
which can blow memory for noisy processes; change the loop that reads from
BufferedReader r to always drain to EOF but only retain a bounded amount of text
(e.g., last N bytes/characters or first N plus tail) in output: implement a
fixed-size ring/truncate strategy when appending to output (or stop appending
after the cap while continuing to read), so you still consume the stream to
avoid hangs but prevent unbounded growth; keep references to
process.getInputStream(), BufferedReader r, and output when locating where to
implement the bounded capture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bfa856b-7b0f-4465-9c82-a2784f1f51e6
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1">
P2: Removed log capping for potentially 10-50MB Mono installer output, reintroducing memory/log-pressure regression</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Quite a few AI comments here. Would you mind addressing these @Catpotatos and I'll take another look at this afterwards?
There was a problem hiding this comment.
Hi @phobos665 , i believe it's all been addressed now, i added a seperate method with pipe drain for the callers that explicitly mention includeStderr=false to address the bot comment. All others that don't mention it for booting/launching, will go through the merged pipe.
Tested on Retroid Pocket 5:
- All Protons installed on first time install with Wine logs + Box64 logs on. No infinite bootings.
- Tested an Amazon, Steam and custom game.
- a daemon thread drains and discards stderr concurrently so the 64 KB pipe buffer never fills while the main thread reads stdout. - When not merging, drain stderr on a daemon thread to prevent pipe-buffer deadlock. - when includeStderr doesn't explicity say includeStderr=false, then use the merged pipe technique example: The Mono install calls execShellCommand → one child process starts, runs, finishes, pipes are closed. Then later SteamTokenLogin calls execShellCommand → a completely separate child process, separate pipes. Majority reaps the rewards of having a merged pipe, any process that doesn't want stderr doesn't go through the merge and has a drain feature for stderr.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java (1)
558-560: ⚡ Quick winRestore interrupt status (and consider destroying the child) on
InterruptedException.
process.waitFor()andstderrDrainer.join(5_000)both throwInterruptedException, which is currently swallowed by the broadcatch (Exception e). When this method runs on a thread pool / cooperative-cancellation context, the lost interrupt flag breaks downstream cancellation. Additionally, if interrupted mid-waitFor(), the child Wine process keeps running with no cleanup.♻️ Suggested handling
- try { + java.lang.Process process = null; + try { Log.d("BionicProgramLauncherComponent", "Shell command is " + finalCommand); ProcessBuilder pb = new ProcessBuilder(ProcessHelper.splitCommand(finalCommand)); ... - java.lang.Process process = pb.start(); + process = pb.start(); ... if (stderrDrainer != null) { stderrDrainer.join(5_000); } + } catch (InterruptedException e) { + if (process != null) process.destroy(); + Thread.currentThread().interrupt(); + output.append("Error: ").append(e.getMessage()); } catch (Exception e) { output.append("Error: ").append(e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java` around lines 558 - 560, The catch-all in BionicProgramLauncherComponent (around process.waitFor() and stderrDrainer.join(5_000)) swallows InterruptedException; change the exception handling so InterruptedException is caught separately, call Thread.currentThread().interrupt() to restore the interrupt status, and ensure the child Process (the variable named process or proc) is cleaned up (call destroy() and/or destroyForcibly() and waitFor) before returning; keep the existing generic exception handling for other Exceptions and still append the error message to output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 558-560: The catch-all in BionicProgramLauncherComponent (around
process.waitFor() and stderrDrainer.join(5_000)) swallows InterruptedException;
change the exception handling so InterruptedException is caught separately, call
Thread.currentThread().interrupt() to restore the interrupt status, and ensure
the child Process (the variable named process or proc) is cleaned up (call
destroy() and/or destroyForcibly() and waitFor) before returning; keep the
existing generic exception handling for other Exceptions and still append the
error message to output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0cc80a8-e1bc-4a63-b0a8-1072c399705b
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
| ProcessBuilder pb = new ProcessBuilder(ProcessHelper.splitCommand(finalCommand)); | ||
| Map<String, String> env = pb.environment(); | ||
| env.clear(); | ||
| for (String kv : envVars.toStringArray()) { | ||
| int eq = kv.indexOf('='); | ||
| if (eq > 0) env.put(kv.substring(0, eq), kv.substring(eq + 1)); |
There was a problem hiding this comment.
TL;DR, move the complexity to ProcessHelper, and call the same method in GlibcProgramLauncherComponent
Hey, i think we would need to do this for glibcProgramLauncherComponent as well. A suggestion, move out this code to ProcessHelper and make a new function, something like:
public static String execWithOutput(String command, String[] envp, File workingDir, boolean includeStderr) {
StringBuilder output = new StringBuilder();
Thread stderrDrainer = null;
try {
ProcessBuilder pb = new ProcessBuilder(splitCommand(command));
Map<String, String> env = pb.environment();
env.clear();
if (envp != null) {
for (String kv : envp) {
int eq = kv.indexOf('=');
if (eq > 0) env.put(kv.substring(0, eq), kv.substring(eq + 1));
}
}
if (workingDir != null) pb.directory(workingDir);
pb.redirectErrorStream(includeStderr);
java.lang.Process process = pb.start();
if (!includeStderr) {
final InputStream stderrStream = process.getErrorStream();
stderrDrainer = new Thread(() -> {
try {
byte[] buf = new byte[4096];
while (stderrStream.read(buf) != -1) {}
} catch (IOException ignored) {}
}, "stderr-drainer");
stderrDrainer.setDaemon(true);
stderrDrainer.start();
}
try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
String line;
while ((line = reader.readLine()) != null) output.append(line).append("\n");
}
process.waitFor();
Then in Bionic and Glibc Program launcher components, in execShellCommand:
Log.d("BionicProgramLauncherComponent", "Shell command is " + finalCommand);
return ProcessHelper.execWithOutput(finalCommand, envVars.toStringArray(),
workingDir != null ? workingDir : imageFs.getRootDir(), includeStderr);
Thoughts?
There was a problem hiding this comment.
This seems plausible, 1 block, call from both.
i just has an infinite booting and some slow loading screens on my Konkr Pocket (running nightlies) so coming back to this PR.
Changing now, will test and update soon.
|
@Catpotatos - can you see if this is still relevant? some changes got merged in |
…onent and GlibcProgramLauncherComponent
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/winlator/core/ProcessHelper.java`:
- Around line 124-126: The code in ProcessHelper (within execWithOutput where
pb.environment() is assigned to env) currently calls env.clear() unconditionally
which removes the inherited environment; change the logic so the existing
environment is preserved when envp is null: only clear and replace the
environment when envp is non-null (i.e., move or gate env.clear() and the
population loop behind the if (envp != null) check), or alternatively, if envp
is non-null, apply/overwrite only the provided entries into pb.environment()
rather than wiping it out; update the code around pb.environment(), env.clear(),
and the envp handling to reflect this.
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 505-507: Remove the log that prints the raw shell command in
BionicProgramLauncherComponent (the Log.d call that concatenates finalCommand);
instead log a static message or a redacted string (e.g., "Shell command executed
(redacted)") and, if helpful, log only non-sensitive metadata (exit code,
workingDir presence) rather than full arguments. Update the call site around
ProcessHelper.execWithOutput and any helper execShellCommand usage to avoid
exposing finalCommand or sensitive envVars in logs, and apply the same change in
the Glibc launcher counterpart to keep behavior consistent across launchers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09f27812-7c72-4844-86d0-dc2365a98151
📒 Files selected for processing (3)
app/src/main/java/com/winlator/core/ProcessHelper.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/java/com/winlator/xenvironment/components/GlibcProgramLauncherComponent.java
| Map<String, String> env = pb.environment(); | ||
| env.clear(); | ||
| if (envp != null) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Java/Android, when Runtime.exec(String[], String[], File) is called with envp = null, does the child inherit the current process environment? How should ProcessBuilder be configured to preserve that behavior?
💡 Result:
Yes, when Runtime.exec(String[], String[], File) is called with envp = null, the child process inherits the environment settings of the current Java process [1][2][3]. To achieve this same behavior using ProcessBuilder, you do not need to perform any special configuration. By default, a ProcessBuilder instance initializes its environment map as a copy of the current process's environment [4][5]. When you call start on a ProcessBuilder, the subprocess uses this map as its environment [5]. If you have modified the environment map (e.g., by adding or removing variables) and wish to revert to the default behavior of inheriting the current process's environment, you would need to manually clear the map and repopulate it with the current process's environment variables, typically retrieved via System.getenv [4][5]. However, for a newly created ProcessBuilder, inheritance is the default state [4][5].
Citations:
- 1: https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/Runtime.html
- 2: https://docs.oracle.com/javase/8/docs/api/java/lang/Runtime.html
- 3: https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/Runtime.html
- 4: https://docs.oracle.com/en/java/javase/22/core/attributes-that-processbuilder-manages.html
- 5: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ProcessBuilder.html
🏁 Script executed:
find . -name "ProcessHelper.java" -type fRepository: utkarshdalal/GameNative
Length of output: 124
🏁 Script executed:
cat -n ./app/src/main/java/com/winlator/core/ProcessHelper.java | sed -n '115,135p'Repository: utkarshdalal/GameNative
Length of output: 1366
Preserve inherited environment when envp is null.
env.clear() at line 125 runs before the null check, so execWithOutput(..., null, ...) now starts children with an empty environment instead of inheriting the current one like Runtime.exec(..., null, ...) does.
Suggested fix
ProcessBuilder pb = new ProcessBuilder(splitCommand(command));
Map<String, String> env = pb.environment();
- env.clear();
if (envp != null) {
+ env.clear();
for (String kv : envp) {
int eq = kv.indexOf('=');
if (eq > 0) env.put(kv.substring(0, eq), kv.substring(eq + 1));
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/winlator/core/ProcessHelper.java` around lines 124 -
126, The code in ProcessHelper (within execWithOutput where pb.environment() is
assigned to env) currently calls env.clear() unconditionally which removes the
inherited environment; change the logic so the existing environment is preserved
when envp is null: only clear and replace the environment when envp is non-null
(i.e., move or gate env.clear() and the population loop behind the if (envp !=
null) check), or alternatively, if envp is non-null, apply/overwrite only the
provided entries into pb.environment() rather than wiping it out; update the
code around pb.environment(), env.clear(), and the envp handling to reflect
this.
| Log.d("BionicProgramLauncherComponent", "Shell command is " + finalCommand); | ||
| return ProcessHelper.execWithOutput(finalCommand, envVars.toStringArray(), | ||
| workingDir != null ? workingDir : imageFs.getRootDir(), includeStderr); |
There was a problem hiding this comment.
Don't log the raw shell command.
finalCommand can carry sensitive arguments, and execShellCommand(...) is used by auth-adjacent flows, so this will leak secrets and paths into logcat. Prefer a static message or a redacted form, and mirror that change in the Glibc launcher too.
Suggested fix
- Log.d("BionicProgramLauncherComponent", "Shell command is " + finalCommand);
+ Log.d("BionicProgramLauncherComponent", "Executing shell command");
return ProcessHelper.execWithOutput(finalCommand, envVars.toStringArray(),
workingDir != null ? workingDir : imageFs.getRootDir(), includeStderr);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`
around lines 505 - 507, Remove the log that prints the raw shell command in
BionicProgramLauncherComponent (the Log.d call that concatenates finalCommand);
instead log a static message or a redacted string (e.g., "Shell command executed
(redacted)") and, if helpful, log only non-sensitive metadata (exit code,
workingDir presence) rather than full arguments. Update the call site around
ProcessHelper.execWithOutput and any helper execShellCommand usage to avoid
exposing finalCommand or sensitive envVars in logs, and apply the same change in
the Glibc launcher counterpart to keep behavior consistent across launchers.
Description
FIX:
Now the app just reads one stream from top to bottom. There's nothing to deadlock against because there's only one pipe, one reader, no coordination needed.
Using ProcessBuilder. Merges stderr into stdout at the OS level — only one pipe. It can never deadlock with itself
Added fallback for processes that call includeStderr=false like SteamTokenLogin.
Capped logs of Mono output to lower memory usage when pumping logs. Catching beginning and end to ensure all necessary bits are still logged. Mono install with wine logging enabled could produce 10–50 MB of combined output.Issue:
Wine logging + Mono install = child writes both stdout and stderr simultaneously. Stderr fills its 64 KB OS pipe buffer WHILE the parent is blocked reading stdout → child blocks on write(stderr) → parent never sees stdout EOF → both sides wait forever. process.waitFor() with no timeout means it sits there permanently.
When installing Mono or extracting XAudio DLLs during first boot, some users would get permanently stuck on the loading screen — especially when wine debug logging was turned on, or when setting up a new Proton version.
The root cause was a pipe deadlock: wine writes a large amount of debug output to stderr during installation. The OS pipe buffer (64 KB) would fill up, causing wine to freeze waiting for someone to read it. Meanwhile, the app was waiting for wine to finish — so both sides were stuck waiting on each other, forever.
Tested on:
Retroid Pocked 5 with Wine logs and Box64 logs on while installing proton 9 and 10
Huawei Mate 20 Pro (Android 10 device with weaker CPU) with Wine logs and Box64 logs on while installing proton 9 and 10
Install was successful on both and no hanging on Mono or Xaudio Installation/Extractions.
Proton 11 installation from 0, also now testes on Retroid Pocket 5, while both logs are on, no stuck booting.
Proton 10.0-4-arm64ec-1 took 25 seconds on first boot in Retroid Pocket 5
Recording
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Fixes the loading screen hang during Mono/XAudio setup by eliminating stdout/stderr deadlocks. Centralizes process execution, merges stderr into stdout when requested, and safely drains stderr otherwise; only runs on first boot, container swap, or boot screen.
ProcessHelper.execWithOutput:ProcessBuilderwithredirectErrorStream(includeStderr), read to EOF beforewaitFor(), and daemon-drain stderr whenincludeStderr=false.BionicProgramLauncherComponentandGlibcProgramLauncherComponentto use the helper; removedRuntime.exec; honors caller env and working directory.Written for commit 395ef7a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Refactor