Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream output while subprocess is running #43

Merged
merged 3 commits into from
Aug 27, 2023
Merged

Stream output while subprocess is running #43

merged 3 commits into from
Aug 27, 2023

Conversation

dimo414
Copy link
Owner

@dimo414 dimo414 commented Aug 15, 2023

This PR reimplements Bkt::execute_subprocess() and its callers to support streaming the subprocess' output while it's running, rather than blocking silently until the subprocess finishes.

As suggested in https://stackoverflow.com/q/66060139 the child process' streams are processed in background threads responsible for both persisting and streaming the output to the caller's stdout/stderr.

Scoped threads are used to support streaming to references such as &mut Vec<u8> and to loosen the type requirements of the stream (namely to not require that they are 'static).

CLI:

  • Subprocess out/err streams are duplicated to bkt's out/err while the subprocess runs, instead of caching all output first and then writing the cached output after the subprocess completes.
  • Streaming is enabled by default and is not configurable.

Library:

  • Added Bkt::retrieve_streaming and Bkt::refresh_streaming methods which accept out/err sinks. The subprocess' output is written to these streams in addition to being cached and available in the returned Invocation.
  • The preexisting Bkt::retrieve/Bkt::refresh behave as previously, i.e. they do not support streaming.
  • These methods are marked "experimental" and are subject to change or even removal.

Fixes #31

As suggested in https://stackoverflow.com/q/66060139 the child process'
streams are processed in background threads responsible for both
persisting and streaming the output to the caller's stdout/stderr.

Scoped threads are used to support streaming to references such as
`&mut Vec<u8>` and to loosen the type requirements of the stream (namely
to not require that they are 'static).

CLI:
* Subprocess out/err streams are duplicated to bkt's out/err while the
  subprocess runs, instead of caching all output first and then writing
  the cached output after the subprocess completes.
* Streaming is enabled by default and is not configurable.

Library:
* Added `Bkt::retrieve_streaming` and `Bkt::refresh_streaming` methods
  which accept out/err sinks. The subprocess' output is written to these
  streams in addition to being cached and available in the returned
  `Invocation`.
* These methods are marked "experimental" and are subject to change or
  even removal.
@dimo414
Copy link
Owner Author

dimo414 commented Aug 27, 2023

Moving this out of the PR description before merging:

I'm positing this as a PR to solicit feedback before merging, so please feel free to weigh in on any parts of this change you think could be improved. Some areas for potential feedback:

  • The signatures and ergonomics of the affected and new methods
    • In particular the impl Write+Send trait bound (which prevents us from using StdoutLock/StderrLock because they are not Send)
  • The use of scoped threads to avoid the 'static trait bound
  • Error handling and Result propagation
    • Notably, currently the .join() calls are .expect()-ed because they don't seem to be compatible with ?
  • The DisregardBrokenPipe behavior, and whether it should be moved into the library core

@dimo414 dimo414 merged commit 7d2e522 into master Aug 27, 2023
19 checks passed
@dimo414 dimo414 deleted the streaming branch January 16, 2024 08:10
dimo414 added a commit that referenced this pull request Jan 20, 2024
It's preserved in the commit history, notably in #43 where it was added,
so there's no need to keep it at HEAD.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: stream the subprocess' stdout and stderr while it's running
1 participant