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] Repair console interactivity in pantsd runs. #5352

Merged
merged 15 commits into from Jan 27, 2018

Conversation

Projects
None yet
4 participants
@kwlzn
Copy link
Member

kwlzn commented Jan 18, 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

@kwlzn kwlzn force-pushed the twitter:kwlzn/pantsd_direct_tty branch from de54837 to 3124655 Jan 18, 2018

@kwlzn kwlzn force-pushed the twitter:kwlzn/pantsd_direct_tty branch from 2094df6 to a53fe58 Jan 23, 2018

@kwlzn kwlzn force-pushed the twitter:kwlzn/pantsd_direct_tty branch from a53fe58 to 733c76e Jan 25, 2018

kwlzn added some commits Jan 25, 2018

@kwlzn kwlzn force-pushed the twitter:kwlzn/pantsd_direct_tty branch from fff7479 to f9d4bf5 Jan 25, 2018

@kwlzn kwlzn changed the title [WIP] [pantsd] Repair console interactivity in pantsd runs. [pantsd] Repair console interactivity in pantsd runs. Jan 26, 2018

@kwlzn kwlzn requested review from stuhood , jsirois and illicitonion Jan 26, 2018

@jsirois
Copy link
Member

jsirois left a comment

LGTM!

:param dict env: A dictionary representing the environment.
:returns: A tuple of boolean values indicating ttyname paths or None for (stdin, stdout, stderr).
"""
return tuple(env.get(cls.TTY_PATH_ENV.format(fd_id)) for fd_id in range(3))

This comment has been minimized.

@jsirois

jsirois Jan 26, 2018

Member

I'd be slightly happier with for fd_id, fd in ((0, stdin), (1, stdout), (2, stderr)): above and ... for fd_id in (0, 1, 2)) here just because 0, 1, & 2 are special and the lucky behavior of enumerate and range (0-based sequence generators), masks this a bit. That said, range(3) has precedent in this file.

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

works for me - fixed.

@@ -98,6 +99,15 @@ def stdio_as(stdout_fd, stderr_fd, stdin_fd):
yield


@contextmanager
def signal_handler_as(sig, handler):

This comment has been minimized.

@jsirois

jsirois Jan 26, 2018

Member

It would be great to keep-up doc discipline in util modules in-particular.

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

yep, my bad - fixed.

This comment has been minimized.

@stuhood
def save_tty_flags(self):
# N.B. `stty(1)` operates against stdin.
try:
self._tty_flags = tty.tcgetattr(sys.stdin.fileno())

This comment has been minimized.

@jsirois

jsirois Jan 26, 2018

Member

Is accessing termios module functions and constants via the tty module idiomatic? I don't know this space; so it is a bit confusing.

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

ah, good call - fixed.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks good :) Really excited about the approach of avoiding being a middle-man!

if all((stdin_isatty, stdout_isatty, stderr_isatty)):
stdin_ttyname, stdout_ttyname, stderr_ttyname = NailgunProtocol.ttynames_from_env(self._env)
assert stdin_ttyname == stdout_ttyname == stderr_ttyname, (
'expected all stdio ttys to be the same! '

This comment has been minimized.

@illicitonion

illicitonion Jan 26, 2018

Contributor

Include their names in the error to help us work out what's going on when they do file a bug?

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

good call, fixed!

@contextmanager
def preserved(cls):
"""Run potentially stty-modifying operations, e.g., REPL execution, in this contextmanager."""
inst = cls()

This comment has been minimized.

@illicitonion

illicitonion Jan 26, 2018

Contributor

Seems a little odd for this to be a classmethod which instantiates itself, rather than the caller calling with STTYSettings().preserved():, but not a big deal

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

was thinking in a similar vein to Class.open() or Class.create() to avoid lifecycle misuse.

is_atty = fd.isatty()
yield (cls.TTY_ENV_TMPL.format(fd_id), bytes(int(is_atty)))
if is_atty:
yield (cls.TTY_PATH_ENV.format(fd_id), os.ttyname(fd.fileno()) or '')

This comment has been minimized.

@illicitonion

illicitonion Jan 26, 2018

Contributor

Is an empty value useful here? If not, maybe phrase this as:

tty_path = os.ttyname(fd.fileno())
if tty_path:
  yield (cls.TTY_PATH_ENV.format(fd_id), tty_path)

?

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

I think if the fd is a tty, I would always expect this env var to be populated for clean retrieval on the other side - so yeah.

@stuhood
Copy link
Member

stuhood left a comment

Thanks Kris... looks good to me.

I think this is small enough to cherry-pick to the 1.4.x branch today.

@@ -253,3 +258,12 @@ def str_int_bool(i):
return i.isdigit() and bool(int(i)) # Environment variable values should always be strings.

return tuple(str_int_bool(env.get(cls.TTY_ENV_TMPL.format(fd_id), '0')) for fd_id in range(3))

@classmethod
def ttynames_from_env(cls, env):

This comment has been minimized.

@stuhood

stuhood Jan 26, 2018

Member

Can we open an upstream ticket about discussing getting this added to the protocol to replace the existing semi-standard TTY env vars?

# character device for output redirection - eliminating the need to directly marshall any
# interactive stdio back/forth across the socket and permitting full, correct tty control with
# no middle-man.
if all((stdin_isatty, stdout_isatty, stderr_isatty)):

This comment has been minimized.

@stuhood

stuhood Jan 26, 2018

Member

The two branches of this if are large/nested enough that I think it would be justified to break them into separate methods.

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

yeah, perhaps. any issue if I tackle this as a follow-on?

This comment has been minimized.

@stuhood

stuhood Jan 26, 2018

Member

Nah, no problem.

@@ -185,8 +201,9 @@ def post_fork_child(self):
# Set context in the process title.
set_process_title('pantsd-runner [{}]'.format(' '.join(self._args)))

# Broadcast our pid to the remote client so they can send us signals (i.e. SIGINT).
NailgunProtocol.send_pid(self._socket, bytes(os.getpid()))
# Broadcast our process group ID (in PID form - i.e. negated) to the remote client so

This comment has been minimized.

@stuhood

stuhood Jan 26, 2018

Member

If this is negated into a process group, is it referred to as something else? A PGID or something?

This comment has been minimized.

@kwlzn

kwlzn Jan 26, 2018

Member

no, the vanilla form would be a PGID. the negated form is PID form.

os.kill(PGID) -> error
os.kill(PGID_IN_PID_FORM) -> works
os.killpg(PGID) -> works

we could invert to PGID form throughout the stack, but this seemed slightly less intrusive in general (and my assumption is that killpg just negates and runs kill anyway).

@@ -98,6 +99,15 @@ def stdio_as(stdout_fd, stderr_fd, stdin_fd):
yield


@contextmanager
def signal_handler_as(sig, handler):

This comment has been minimized.

@stuhood

@stuhood stuhood added this to the 1.4.x milestone Jan 26, 2018

@kwlzn kwlzn merged commit 4c21f01 into pantsbuild:master Jan 27, 2018

1 check passed

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

stuhood added a commit that referenced this pull request Jan 27, 2018

[pantsd] Repair console interactivity in pantsd runs. (#5352)
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

kwlzn added a commit that referenced this pull request Jan 30, 2018

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