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

Switch from output_async to spawn_async, and buffer output. #6105

Merged
merged 2 commits into from Jul 14, 2018

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented Jul 12, 2018

Problem

See #6089.

Solution

Switch to spawn_async and fold over a stream of outputs. To stream/duplicate output elsewhere, we'd add logic in the fold.

Result

Fixes #6089.

@stuhood stuhood requested review from baroquebobcat and illicitonion Jul 12, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks!

@@ -37,6 +38,25 @@ impl CommandRunner {
}
}

fn outputs_stream_for_child(
mut child: Child,
) -> Box<Stream<Item = ChildOutput, Error = String> + Send> {

This comment has been minimized.

@illicitonion

illicitonion Jul 13, 2018

Contributor

I know I was the one who said we shouldn't use impl Future, but I'd be tempted to use impl Stream here :P My reasoning being:

  1. This isn't really a thing we do elsewhere, so it probably doesn't add to confusion
  2. This won't be passed around to other things which may need to ambiguously handle both

But I definitely don't mind either way

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

Agreed!

.map(move |snapshot| FallibleExecuteProcessResult {
stdout: stdout.freeze(),
stderr: stderr.freeze(),
exit_code: exit_code.unwrap_or(-1),

This comment has been minimized.

@illicitonion

illicitonion Jul 13, 2018

Contributor

I would expect there to always be an exit code, right? Maybe make this an exit_code.ok_or_else(|| "Process had no exit code".to_owned())

This comment has been minimized.

@illicitonion

illicitonion Jul 13, 2018

Contributor

Aha, in that case can we do -signal rather than -1? :)

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

Yeeep!

@stuhood stuhood merged commit 8feda5d into pantsbuild:master Jul 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/streaming-process-outputs branch Jul 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment