Create Pipe wrapper around pipes #7740
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blorente
requested review from
stuhood,
illicitonion,
ity,
cosmicexplorer and
Eric-Arellano
stuhood
approved these changes
May 16, 2019
src/python/pants/java/nailgun_io.py
Outdated
| @staticmethod | ||
| def open(isatty): | ||
| """Open a pipe and create wrapper object.""" | ||
| # TODO This doesn't need to be a contextmanager. |
| def _pipe(isatty): | ||
| r_fd, w_fd = os.openpty() if isatty else os.pipe() | ||
| yield (r_fd, w_fd) | ||
| class Pipe(object): |
There was a problem hiding this comment.
Not for now, as we need default values for self.writable. Have left a TODO with the the link to a PR that will enable that.
Eric-Arellano
approved these changes
May 16, 2019
illicitonion
pushed a commit
that referenced
this pull request
May 22, 2019
## Problem As described in #7465, when we pipe the output of a pants run with pantsd into anything, it may truncate output. The root cause: - `NailgunStreamWriter` runs in another thread, and is periodically reading from `std{out,err}` and writing it in the Nailgun socket. Among other uses, we create one of these whenever we need to redirect stdout/err from pants into the client. - In the current implementation of pantsd, this happens in DaemonPantsRunner, whenever we call to `nailgunned_stdio` and either stdout, err or in are not TTYs (there's more nuance, but it's not relevant). We create pipes and give the writing end to the `DaemonPantsRunner`, while the `NailgunStreamWriter` listens to the reading end for output. The `DaemonPantsRunner` then redirects `sys.stdout/err` (and some other stuff) to the reading ends of those pipes, through `stdio_as`. This happens in two places: - When we warm up the v2 product graph, where things like v2 list are computed. - When we actually execute the pants run under pantsd. - When the daemon is finished with `nailgunned_stdio`, it stops the `NailgunStreamWriter` thread that is transmitting output to the client through the socket, and then it joins until it's done. **- The issue:** When the daemon `stop()`s the thread, this stops reading immediately, even if it hasn't finished transmitting every chunk to the client. Therefore, there is a race condition between `NailgunStreamWriter` trying to send all the chunk through the socket and `DaemonPantsRunner` trying to close it. This can be checked by inserting a `time.sleep(0.1)` [here](https://github.com/pantsbuild/pants/blob/3c17cd2b54b468590ddf8f486f37652ed0db6afc/src/python/pants/java/nailgun_io.py#L201), which will make the issue repro consistently. ## Solution Depends on #7740, and includes commits from that. **The only commit worth reviewing is b9d0c95** The `DaemonPantsRunner` must no longer `stop()` the thread, but rather mark the pipes that are is stdout/err as "I'm not going to write to this anymore, please close yourself when you're done reading from this". We create the class `PipedNailgunStreamWriter`, wich is aware that it's reading end belongs to a pipe. Whenever we don't have anything to read, we check if the write end of the pipe is closed. If so, we stop trying to read from that pipe. When no pipes are left, we close ourselves. ## Result Hopefully, `./pants --enable-pantsd list :: | wc -l` should give consistent output, and #7465 should be resolved.
illicitonion
pushed a commit
that referenced
this pull request
May 22, 2019
### Problem Dealing with raw file descriptors is confusing at best and error-prone at worst. ### Solution Define a `Pipe` abstraction that owns (read: "managest the lifetimes of") the write and read ends of a pipe, as well as provide convenience methods to query the state of the fds. ### Result It's now a bit more obvious what pipes are open. Part one of the fix for #7465
illicitonion
pushed a commit
that referenced
this pull request
May 22, 2019
## Problem As described in #7465, when we pipe the output of a pants run with pantsd into anything, it may truncate output. The root cause: - `NailgunStreamWriter` runs in another thread, and is periodically reading from `std{out,err}` and writing it in the Nailgun socket. Among other uses, we create one of these whenever we need to redirect stdout/err from pants into the client. - In the current implementation of pantsd, this happens in DaemonPantsRunner, whenever we call to `nailgunned_stdio` and either stdout, err or in are not TTYs (there's more nuance, but it's not relevant). We create pipes and give the writing end to the `DaemonPantsRunner`, while the `NailgunStreamWriter` listens to the reading end for output. The `DaemonPantsRunner` then redirects `sys.stdout/err` (and some other stuff) to the reading ends of those pipes, through `stdio_as`. This happens in two places: - When we warm up the v2 product graph, where things like v2 list are computed. - When we actually execute the pants run under pantsd. - When the daemon is finished with `nailgunned_stdio`, it stops the `NailgunStreamWriter` thread that is transmitting output to the client through the socket, and then it joins until it's done. **- The issue:** When the daemon `stop()`s the thread, this stops reading immediately, even if it hasn't finished transmitting every chunk to the client. Therefore, there is a race condition between `NailgunStreamWriter` trying to send all the chunk through the socket and `DaemonPantsRunner` trying to close it. This can be checked by inserting a `time.sleep(0.1)` [here](https://github.com/pantsbuild/pants/blob/3c17cd2b54b468590ddf8f486f37652ed0db6afc/src/python/pants/java/nailgun_io.py#L201), which will make the issue repro consistently. ## Solution Depends on #7740, and includes commits from that. **The only commit worth reviewing is b9d0c95** The `DaemonPantsRunner` must no longer `stop()` the thread, but rather mark the pipes that are is stdout/err as "I'm not going to write to this anymore, please close yourself when you're done reading from this". We create the class `PipedNailgunStreamWriter`, wich is aware that it's reading end belongs to a pipe. Whenever we don't have anything to read, we check if the write end of the pipe is closed. If so, we stop trying to read from that pipe. When no pipes are left, we close ourselves. ## Result Hopefully, `./pants --enable-pantsd list :: | wc -l` should give consistent output, and #7465 should be resolved.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Dealing with raw file descriptors is confusing at best and error-prone at worst.
Solution
Define a
Pipeabstraction that owns (read: "managest the lifetimes of") the write and read ends of a pipe, as well as provide convenience methods to query the state of the fds.Result
It's now a bit more obvious what pipes are open.
Part one of the fix for #7465