Stream CR-only progress ticks in run() instead of buffering them into one message - #2731
Conversation
During a plugin install or update the download progress line clumps into one very long line instead of updating in place. A captured example was a single 3486 character message holding 41 carriage returns and one trailing line feed. Root cause is in run(): the subprocess pipe was read with fgets(), which only breaks on LF. A command that emits classic CR-only progress ticks therefore keeps filling the same fgets() read until a LF finally arrives (or EOF), and the whole progress sequence leaves run() as one message. The receiving side splits on CR per received message, so once the ticks are already glued together inside a single message it can no longer render them in place. Read the pipe character by character and flush on CR or LF, so every tick is passed on as its own message. 'multiplugin' in the same directory already reads its child output this way and is the reference for the pattern. Two details are handled beyond that reference: a final chunk without a terminator is still flushed before returning, and fgetc() returning false at end of stream ends the loop explicitly rather than relying on the silent false to string cast. Return value (pclose) and the emitted bytes are unchanged; only the message boundaries differ. Ordinary LF-only output is unaffected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesPlugin output handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change streams carriage-return progress updates as separate messages while preserving emitted bytes and existing return behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 Warning |
1 participant
Fixes the plugin-install progress garbling reported on the Unraid forum, confirmed by Jorge (@jorgeb).
The bug
emhttp/plugins/dynamix.plugin.manager/scripts/plugin'srun()read subprocess output withfgets(), which only splits on\n. A command whose output uses\r-only progress ticks (no\nbetween them, the classic in-place progress bar) gets every tick buffered into onefgets()read that doesn't return until an actual\nshows up or the process exits. That single, already-clumped blob then goes out as one nchan message. The client's own per-message\rhandling inrenderMessage()(emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php) is correct, it operates on whole received messages, but by the time a clumped message arrives there's nothing left for it to split. Reproduced live: a real install produced a single 3486-character message with 41 embedded\r.The fix
Ports the pattern this repo already uses correctly in the sibling script
multiplugin: read one character at a time withfgetc()and flush on\ror\n, so each tick becomes its own message.function run($command) { $run = popen($command,'r'); - while (!feof($run)) write(fgets($run)); + $line = ''; + while (!feof($run)) { + $char = fgetc($run); + if ($char === false) break; + $line .= $char; + if (in_array($char,["\r","\n"])) {write($line); $line = '';} + } + if ($line !== '') write($line); return pclose($run); }Only
run()changes;multipluginis untouched.Two small, deliberate differences from
multiplugin's version, both strictly better and both verified against real output:$char === falsebreak on end-of-stream, instead of relying onfgets-style implicit end-of-stream handling. This also means a stream that errors without ever setting EOF can't spin the loop forever, which the old code was theoretically exposed to.if ($line !== '') write($line)after the loop, so output with no final terminator (or output that's just the literal string"0") still reaches the client.multiplugin's own!empty($line)guard would drop a bare"0", sinceempty("0")is true in PHP; this uses!== ''instead.Testing
Emitted bytes are unchanged in every case tested (only the message boundaries differ):
\r-only progress ticks, normal\n-terminated lines, a mix of both, output with no trailing terminator, a bare"0"with no terminator, and no output at all. Verified against the five call sites ofrun()in this file (pre_hooks,post_hooks, and the three run variants) that none of them depend on the old message granularity, only on the return value.php -lpasses.One pre-existing property, unchanged by this PR and already present in
multiplugin: a stream that mixes\n-only lines with later\r\n-terminated lines can lose one line at the transition, because the client's renderer treats a\r-terminated message as "replace the previous line." Not introduced here, and unlikely on the Linux run-scripts this path executes (wget/curl-style progress is\r-only, not CRLF).Summary by CodeRabbit