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

[pantsd] Ctrl-C isn't forwarded properly when a pailun request is waiting for another one to finish #7920

Closed
blorente opened this issue Jun 21, 2019 · 3 comments · Fixed by #7924

Comments

@blorente
Copy link
Contributor

commented Jun 21, 2019

Currently, we don't send the PID and PGRP chunks through Nailgun until a DaemonPantsRunner has been instantiated and is running:

with self._maybe_shutdown_socket.lock:

This leads to behaviour like this, because the client relies on knowing the pid of the pantsd process when forwarding SIGINT to the daemon.

The problem we want to solve is how to stop a pants run that is waiting for another one to finish, without crashing the daemon.

The more I think about it the more it seems that using signals is not going to work for per-request cancellation, and we need a Nailgun-based solution. The approach that seems most feasible to me is to have another socket that the client can use to send messages to the server (or the same socket if bidirectional communication is possible), and to wrap the thread that is handling a request in another thread that reads messages from the client and stops the request being served when the client sends EXIT.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

We can probably just have the client die here, I think, without forwarding any signals!

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Yea, I'm pretty sure that just "going away" if the pid hasn't been sent works, as long as we treat "the pid has been sent" as the signal that "substantial work has started".

This continues to be a bit of an odd protocol extension, but it's useful here.

@stuhood stuhood added the pantsd label Jun 21, 2019

@ity ity removed the needs-cherrypick label Jun 25, 2019

blorente added a commit that referenced this issue Jun 26, 2019
Crash pailgun client on SIGINT if we haven't received the pid yet (#7924
)

## Problem

If the client hadn't received the remote process PID when it received a Ctrl-C signal, it was unable to forward it to the daemon. The client receives the PID once the actual pants work starts, so there was no way of Ctrl-C-ing out of pants when a request was waiting to aquire the pantsd lock. More context and examples here: #7920

## Solution

Crash the client when we receive ctrl-c and haven't received a remote pid yet, so that the request is no longer valid, and the server can stop trying to serve it.

## Result

Ctrl-C now works when waiting for the daemon lock, and it doesn't kill the daemon.
blorente added a commit to blorente/pants that referenced this issue Jun 26, 2019
Crash pailgun client on SIGINT if we haven't received the pid yet (pa…
…ntsbuild#7924)

## Problem

If the client hadn't received the remote process PID when it received a Ctrl-C signal, it was unable to forward it to the daemon. The client receives the PID once the actual pants work starts, so there was no way of Ctrl-C-ing out of pants when a request was waiting to aquire the pantsd lock. More context and examples here: pantsbuild#7920

## Solution

Crash the client when we receive ctrl-c and haven't received a remote pid yet, so that the request is no longer valid, and the server can stop trying to serve it.

## Result

Ctrl-C now works when waiting for the daemon lock, and it doesn't kill the daemon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.