Ctrl+D behaviour mismatched with/without pantsd
#5058
Comments
stuhood
pushed a commit
that referenced
this issue
Nov 9, 2017
### Problem The `DaemonPantsRunner` did not support consuming `STDIN` and `STDIN_EOF` chunks off of the nailgun socket, and so subprocesses of the pants daemon that read from `stdin` would immediately exit. Additionally, because `stdin`/`stderr` were not actual pipes in the `DaemonPantsRunner` (rather, they were file-like objects that wrote directly to the nailgun socket) we were not able to fork and still proxy `stdout`/`stderr` from the forked process. ### Solution * Replace the synchronous file-like `NailgunStreamWriter` with an adjusted version of the previous reader code that uses a thread to copy from an anonymous `os.pipe`/`os.openpty` onto the socket. * Use `NailgunStreamWriter` to publish `sys.stdout`/`sys.stderr` from the server, and `sys.stdin` from the client. * Implement `NailgunStreamStdinReader` to read `stdin` nailgun chunks on the server and proxy them to `sys.stdin`. * Explicitly pass `sys.stdin` and `sys.stderr` when forking with SubprocessExecutor to disable additional handling in `Popen` that caused forked processes to write directly to fd 1 (the socket, in this case). * Add an `@ensure_daemon` decorator, and use it to ensure that python and scala repls work in the context of the daemon. * Move the watchman directory under the `subprocess_dir`, to fix an issue where calling `./pants kill-pantsd` would fail if the `subprocess_dir` matched but the `workdir` didn't. ### Result Fixes #3774. There is one slight oddity, which is that our `Ctrl+D` handling doesn't quite align with daemonless usage: opened #5058 about that.
kwlzn
pushed a commit
to twitter/pants
that referenced
this issue
Nov 27, 2017
### Problem The `DaemonPantsRunner` did not support consuming `STDIN` and `STDIN_EOF` chunks off of the nailgun socket, and so subprocesses of the pants daemon that read from `stdin` would immediately exit. Additionally, because `stdin`/`stderr` were not actual pipes in the `DaemonPantsRunner` (rather, they were file-like objects that wrote directly to the nailgun socket) we were not able to fork and still proxy `stdout`/`stderr` from the forked process. ### Solution * Replace the synchronous file-like `NailgunStreamWriter` with an adjusted version of the previous reader code that uses a thread to copy from an anonymous `os.pipe`/`os.openpty` onto the socket. * Use `NailgunStreamWriter` to publish `sys.stdout`/`sys.stderr` from the server, and `sys.stdin` from the client. * Implement `NailgunStreamStdinReader` to read `stdin` nailgun chunks on the server and proxy them to `sys.stdin`. * Explicitly pass `sys.stdin` and `sys.stderr` when forking with SubprocessExecutor to disable additional handling in `Popen` that caused forked processes to write directly to fd 1 (the socket, in this case). * Add an `@ensure_daemon` decorator, and use it to ensure that python and scala repls work in the context of the daemon. * Move the watchman directory under the `subprocess_dir`, to fix an issue where calling `./pants kill-pantsd` would fail if the `subprocess_dir` matched but the `workdir` didn't. ### Result Fixes pantsbuild#3774. There is one slight oddity, which is that our `Ctrl+D` handling doesn't quite align with daemonless usage: opened pantsbuild#5058 about that.
|
This also seems to affect the ability to move the cursor in the repl. I'm going to move this back into the daemon beta milestone. |
|
Sounds like @kwlzn is on this one. Thanks Kris! |
|
Also related to #5174. |
|
(Moved both of these into the daemon beta to cluster around #5174. Although it seems like maybe they could also be merged/resolved-dupe.) |
kwlzn
added a commit
that referenced
this issue
Jan 27, 2018
Problem Currently, in the case of pantsd-based runs, because we marshall stdio across a socket via the nailgun protocol we have no way for subprocesses to indicate tty settings like they normally would when directly connected to a tty device. This causes interactive things like ./pants repl (which rely on libraries like jline/readline) to behave strangely - not responding to up/down/left/right/^U/^K/^D, echoing double lines or double chars, etc. The same case is present for ./pants run of interactive programs. These are all things normally controlled via the tty. Solution Send the fully qualified path to the tty via the /dev filesystem as nailgun environment variables. If we detect the thin client as connected to the same tty on all 3 stdio descriptors, we'll now directly open the TTY in the pantsd-runner process and redirect stdio to it. This is inherited by subprocesses, too. In order to ensure subprocesses potentially connected to the thin clients tty don't outlive the pants run, we now also send SIGINT on ^C/^\ to the runners entire process group to ensure any subprocesses also receive the signal (they may still ignore it tho, in some cases - see below). Additionally, in the case of the python repl (which ignores ^C by default) - we now wrap that with a new signal_handler_as contextmanager to ignore ^C in the pants-side process to mimic the correct behavior of a vanilla python repl (which is to log a handled KeyboardInterrupt). I've also ported STTYSettings to use of tcgetattr/tcsetattr (to avoid subprocess invokes of stty) and wrapped the thin client invoke in that. Result Both the python and scala repl cases behave as expected in the daemon. Fixes #5174 Fixes #5058
stuhood
pushed a commit
that referenced
this issue
Jan 27, 2018
Problem Currently, in the case of pantsd-based runs, because we marshall stdio across a socket via the nailgun protocol we have no way for subprocesses to indicate tty settings like they normally would when directly connected to a tty device. This causes interactive things like ./pants repl (which rely on libraries like jline/readline) to behave strangely - not responding to up/down/left/right/^U/^K/^D, echoing double lines or double chars, etc. The same case is present for ./pants run of interactive programs. These are all things normally controlled via the tty. Solution Send the fully qualified path to the tty via the /dev filesystem as nailgun environment variables. If we detect the thin client as connected to the same tty on all 3 stdio descriptors, we'll now directly open the TTY in the pantsd-runner process and redirect stdio to it. This is inherited by subprocesses, too. In order to ensure subprocesses potentially connected to the thin clients tty don't outlive the pants run, we now also send SIGINT on ^C/^\ to the runners entire process group to ensure any subprocesses also receive the signal (they may still ignore it tho, in some cases - see below). Additionally, in the case of the python repl (which ignores ^C by default) - we now wrap that with a new signal_handler_as contextmanager to ignore ^C in the pants-side process to mimic the correct behavior of a vanilla python repl (which is to log a handled KeyboardInterrupt). I've also ported STTYSettings to use of tcgetattr/tcsetattr (to avoid subprocess invokes of stty) and wrapped the thin client invoke in that. Result Both the python and scala repl cases behave as expected in the daemon. Fixes #5174 Fixes #5058
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Sending
Ctrl+Dto a scala repl usually triggers shutdown of the repl, but in the context of the daemon, it does not.stdinproperly closes (ie, theChunkType.STDIN_EOFmessage is properly sent), but some other magic is missing that is preventing stdin from completely closing.On the other hand, something like:
...works fine and exits cleanly. Also, sending
Ctrl+Cdoes what you'd expect and cleanly exits the repl.The text was updated successfully, but these errors were encountered: