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

Create Pipe wrapper around pipes #7740

Merged
merged 2 commits into from May 21, 2019

Conversation

Projects
None yet
4 participants
@blorente
Copy link
Contributor

commented May 16, 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

@stuhood
Copy link
Member

left a comment

Nice, thanks!

@staticmethod
def open(isatty):
"""Open a pipe and create wrapper object."""
# TODO This doesn't need to be a contextmanager.

This comment has been minimized.

Copy link
@stuhood

stuhood May 16, 2019

Member

Is this stale?

def _pipe(isatty):
r_fd, w_fd = os.openpty() if isatty else os.pipe()
yield (r_fd, w_fd)
class Pipe(object):

This comment has been minimized.

Copy link
@stuhood

stuhood May 16, 2019

Member

Worth declaring as a datatype?

This comment has been minimized.

Copy link
@blorente

blorente May 21, 2019

Author Contributor

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
Copy link
Contributor

left a comment

Yay! This is some of our trickiest code, and this is a great way to better abstract it all.

Show resolved Hide resolved src/python/pants/java/nailgun_io.py Outdated
@stuhood
Copy link
Member

left a comment

Thanks!

@stuhood stuhood merged commit 805ef8a into pantsbuild:master May 21, 2019

1 check passed

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

illicitonion added a commit that referenced this pull request May 22, 2019

Have the daemon wait until we stop writing to the socket. (#7782)
## 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 added a commit that referenced this pull request May 22, 2019

Create Pipe wrapper around pipes (#7740)
### 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 added a commit that referenced this pull request May 22, 2019

Have the daemon wait until we stop writing to the socket. (#7782)
## 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
You can’t perform that action at this time.