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

Fix a deadlock in local process execution. #6292

Merged
merged 1 commit into from Aug 3, 2018

Conversation

Projects
None yet
3 participants
@jsirois
Copy link
Member

jsirois commented Aug 3, 2018

Previously we hand-rolled collection of process output streams; we now
leverage tokio-process to do this for us.

Fixes #6242

Fix a deadlock in local process execution.
Previously we hand-rolled collection of process output streams; we now
leverage tokio-process to do this for us.

Fixes #6242
@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 3, 2018

NB: I did not dig too far into where our hand-rolled stream-collection might be deadlocking. My only hunch was that stdin was not dealt with, but you'd expect tokio-process would handle Stdio:null() for us. Nonetheless, code is simplified and I now cannot reproduce a hang (100+ iterations of ./pants test.pytest --no-timeouts --cache-ignore tests/python/pants_test/engine:isolated_process -- -ktest_integration_concat_with_snapshots_stdout) which was easy to do before. The only bit I'm concerned with is the tokio-process Output struct has stdout and stderr as Vec<u8>. The Bytes from implementation re-uses the underlying vec memory so there is no copying; ie: I think we're good, but this is worth scrutiny.

@jsirois jsirois requested review from illicitonion and stuhood Aug 3, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Aug 3, 2018

This is basically a revert of #6105 which is a shame because that was a useful direction to be going, both to support timeouts, and to support streaming for console rules in the future... It would be good for someone (one of me, @jsirois, or @stuhood, I guess) to do a little poking at what's wrong with the logic before blowing it away...

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 3, 2018

Aha. A comment for posterity would be wonderful to justify all the hoops.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 3, 2018

I could add an inverse comment / todo if that works for you all.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 3, 2018

To be clear, I think removing CI hangs takes precedence over supporting an unused future feature crease. I am happy to investigate the original code once CI is unblocked if you both agree.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Aug 3, 2018

Works for me :) thanks!

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 3, 2018

Yep, totally works for me. Very sorry for the trouble!

@stuhood

stuhood approved these changes Aug 3, 2018

@stuhood stuhood merged commit f6595ab into pantsbuild:master Aug 3, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

jsirois commented Aug 3, 2018

OK - turns out the bug was fairly obvious in retrospect - aren't they all. Follow-up coming that re-instates the unused future feature crease sans hang.

@jsirois jsirois deleted the jsirois:issues/6242 branch Aug 3, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Fix a deadlock in local process execution. (pantsbuild#6292)
Previously we hand-rolled collection of process output streams; we now
leverage tokio-process to do this for us.

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