Repository navigation
fix(p3-shim): add output flush and shutdown for Node and browsers - #2412
vados-cosmonic merged 15 commits into
Conversation
vados-cosmonic
left a comment
There was a problem hiding this comment.
Hey @darshpatel1052 thanks for working on this -- this looks like a great start but there's quite a thorny issue here which makes this difficult.
The biggest one here being that exit does not/should not mean we actually drain all the content of a stream. What we can do, however is have a utility for draining the stdout/stderr streams that can be used before the host program exits, if users want.
Programs that want to write and ensure those writes are processed need to flush, or the host needs to ensure that all the streams are flushed before returning.
The main problem is actually deeper here -- the problem is that wasmtime (and possibly other hosts) actually wait for the flush before confirming the write. Jco does not implement that exactly (we take the write, and ack it as soon as the write is complete, without waiting for a flush -- arguably a more correct reading of the spec), so this is how we get writes that are pending at exit where wasmtime would not.
There is actually a feature that is coming which will allow streams to indicate when the write is complete (rather than just the bytes being accepted by the other side), but that change has not arrived just yet (so we essentially can't add it here).
Would you mind reworking this PR to go in the shutdown & drain utility to the stdout/stderr streams in particular -- a mechanism for waiting on them to flush (if there are pending writes)? Once we have those two things we can add a helper that waits on both (and possibly call that helper from jco run) to make things easier for components that use stdin/stdout and don't flush on their own.
Unfortunately I think until we have the new stream mechanism we won't actually be able to properly solve the problem, absent changing write to wait for flushes.
Keep WASI exit immediate and expose separate stdout/stderr flush and shutdown utilities. Let the command runner drain accepted output after normal return without changing component stream-write acknowledgments. Cover per-stream isolation, pending callbacks, abrupt exits and explicit host flushing before a nonzero exit.
Thanks for explaining, that makes sense. I’ve pushed
|
|
Hey @darshpatel1052 so I think this is almost ready to go now -- I noticed that there wasn't exactly easy browser support and we want to put some documentation in the book for this new feature, so I added those in follow up commits. Thanks you for working on this! |
vados-cosmonic
left a comment
There was a problem hiding this comment.
LGTM 🚀
Added a few commits to widen the scope of this feature (and docs and stuff), but it looks good to go now. Thanks again @darshpatel1052 !
Yay, thank you! Really happy to hear that. Thanks again for the review and all your additions, I learned a lot working through this! |
623c714
Closes #2397.
Preview 3 command output could remain queued when
jco runexited after a normal return. Add explicitstdout._flush()/stderr._flush()and._shutdown()helpers, combined CLI helpers, and command-runner shutdown after return.The lifecycle API is available in both Node.js and browser hosts. Node waits for writable callbacks. Browsers retain their existing synchronous output handlers; buffered or asynchronous sinks can opt into an additional drain acknowledgement through
_setStdout(handler, drain)or_setStderr(handler, drain). Existing one-argument setters and stream-write completion semantics remain compatible. WASI exit stays immediate and does not drain output. Shutdown closes active host readers without requiring guest EOF and leaves shared destinations open.Includes pending-read disposal fixes, Node/browser consumer declaration checks, concurrent output regressions with shared process helpers, and README plus advanced-book documentation.
Validation:
Browser output adapter tests exercise the actual browser implementations under Node. The Chrome fixture suite remains unavailable in this environment because the browser is missing
libglib-2.0.so.0.