From 9f6600ac1cda89f9e28bf1a2dbc4a51d9e866c55 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 8 Jul 2019 20:52:37 +0200 Subject: [PATCH] test: fix pty test hangs on aix Some pty tests persistently hung on the AIX CI buildbots. Fix that by adding a helper script that properly sets up the pty before spawning the script under test. On investigation I discovered that the test runner hung when it tried to close the slave pty's file descriptor, probably due to a bug in AIX's pty implementation. I could reproduce it with a short C program. The test runner also leaked file descriptors to the child process. I couldn't convince python's `subprocess.Popen()` to do what I wanted it to do so I opted to move the logic to a helper script that can do fork/setsid/etc. without having to worry about stomping on state in tools/test.py. In the process I also uncovered some bugs in the pty module of the python distro that ships with macOS 10.14, leading me to reimplement a sizable chunk of the functionality of that module. And last but not least, of course there are differences between ptys on different platforms and the helper script has to paper over that. Of course. Really, this commit took me longer to put together than I care to admit. Caveat emptor: this commit takes the hacky ^D feeding to the slave out of tools/test.py and puts it in the *.in input files. You can also feed other control characters to tests, like ^C or ^Z, simply by inserting them into the corresponding input file. I think that's nice. Fixes: https://github.com/nodejs/build/issues/1820 Fixes: https://github.com/nodejs/node/issues/28489 PR-URL: https://github.com/nodejs/node/pull/28600 Reviewed-By: Richard Lau Reviewed-By: Sam Roberts Reviewed-By: Anna Henningsen --- test/pseudo-tty/pseudo-tty.status | 35 ---------- test/pseudo-tty/pty_helper.py | 98 ++++++++++++++++++++++++++++ test/pseudo-tty/test-stdout-read.in | 1 + test/pseudo-tty/test-stdout-read.out | 1 + test/pseudo-tty/testcfg.py | 14 ++-- tools/test.py | 78 ++++------------------ 6 files changed, 122 insertions(+), 105 deletions(-) create mode 100644 test/pseudo-tty/pty_helper.py diff --git a/test/pseudo-tty/pseudo-tty.status b/test/pseudo-tty/pseudo-tty.status index 9d5a2a0d12b21f..0a302b2d56d107 100644 --- a/test/pseudo-tty/pseudo-tty.status +++ b/test/pseudo-tty/pseudo-tty.status @@ -1,40 +1,5 @@ prefix pseudo-tty -[$system==aix] -# https://github.com/nodejs/build/issues/1820#issuecomment-505998851 -# https://github.com/nodejs/node/pull/28469 -# https://github.com/nodejs/node/pull/28541 -console_colors: SKIP -console-dumb-tty: SKIP -no_dropped_stdio: SKIP -no_interleaved_stdio: SKIP -readline-dumb-tty: SKIP -ref_keeps_node_running: SKIP -repl-dumb-tty: SKIP -stdin-setrawmode: SKIP -test-assert-colors: SKIP -test-assert-no-color: SKIP -test-assert-position-indicator: SKIP -test-async-wrap-getasyncid-tty: SKIP -test-fatal-error: SKIP -test-handle-wrap-isrefed-tty: SKIP -test-readable-tty-keepalive: SKIP -test-set-raw-mode-reset-process-exit: SKIP -test-set-raw-mode-reset-signal: SKIP -test-set-raw-mode-reset: SKIP -test-stderr-stdout-handle-sigwinch: SKIP -test-stdin-write: SKIP -test-stdout-read: SKIP -test-tty-color-support: SKIP -test-tty-isatty: SKIP -test-tty-stdin-call-end: SKIP -test-tty-stdin-end: SKIP -test-tty-stdout-end: SKIP -test-tty-stdout-resize: SKIP -test-tty-stream-constructors: SKIP -test-tty-window-size: SKIP -test-tty-wrap: SKIP - [$system==solaris] # https://github.com/nodejs/node/pull/16225 - `ioctl(fd, TIOCGWINSZ)` seems # to fail with EINVAL on SmartOS when `fd` is a pty from python's pty module. diff --git a/test/pseudo-tty/pty_helper.py b/test/pseudo-tty/pty_helper.py new file mode 100644 index 00000000000000..d0a4b945d9d45e --- /dev/null +++ b/test/pseudo-tty/pty_helper.py @@ -0,0 +1,98 @@ +import errno +import os +import pty +import select +import signal +import sys +import termios + +STDIN = 0 +STDOUT = 1 +STDERR = 2 + + +def pipe(sfd, dfd): + try: + data = os.read(sfd, 256) + except OSError as e: + if e.errno != errno.EIO: + raise + return True # EOF + + if not data: + return True # EOF + + if dfd == STDOUT: + # Work around platform quirks. Some platforms echo ^D as \x04 + # (AIX, BSDs) and some don't (Linux). + filt = lambda c: ord(c) > 31 or c in '\t\n\r\f' + data = filter(filt, data) + + while data: + try: + n = os.write(dfd, data) + except OSError as e: + if e.errno != errno.EIO: + raise + return True # EOF + data = data[n:] + + +if __name__ == '__main__': + argv = sys.argv[1:] + + # Make select() interruptable by SIGCHLD. + signal.signal(signal.SIGCHLD, lambda nr, _: None) + + master_fd, slave_fd = pty.openpty() + assert master_fd > STDIN + + mode = termios.tcgetattr(slave_fd) + # Don't translate \n to \r\n. + mode[1] = mode[1] & ~termios.ONLCR # oflag + # Disable ECHOCTL. It's a BSD-ism that echoes e.g. \x04 as ^D but it + # doesn't work on platforms like AIX and Linux. I checked Linux's tty + # driver and it's a no-op, the driver is just oblivious to the flag. + mode[3] = mode[3] & ~termios.ECHOCTL # lflag + termios.tcsetattr(slave_fd, termios.TCSANOW, mode) + + pid = os.fork() + if not pid: + os.setsid() + os.close(master_fd) + + # Ensure the pty is a controlling tty. + name = os.ttyname(slave_fd) + fd = os.open(name, os.O_RDWR) + os.dup2(fd, slave_fd) + os.close(fd) + + os.dup2(slave_fd, STDIN) + os.dup2(slave_fd, STDOUT) + os.dup2(slave_fd, STDERR) + + if slave_fd > STDERR: + os.close(slave_fd) + + os.execve(argv[0], argv, os.environ) + raise Exception('unreachable') + + os.close(slave_fd) + + fds = [STDIN, master_fd] + while fds: + try: + rfds, _, _ = select.select(fds, [], []) + except select.error as e: + if e[0] != errno.EINTR: + raise + if pid == os.waitpid(pid, os.WNOHANG)[0]: + break + + if STDIN in rfds: + if pipe(STDIN, master_fd): + fds.remove(STDIN) + + if master_fd in rfds: + if pipe(master_fd, STDOUT): + break diff --git a/test/pseudo-tty/test-stdout-read.in b/test/pseudo-tty/test-stdout-read.in index 10ddd6d257e013..d7bda826cf5db5 100644 --- a/test/pseudo-tty/test-stdout-read.in +++ b/test/pseudo-tty/test-stdout-read.in @@ -1 +1,2 @@ Hello! + \ No newline at end of file diff --git a/test/pseudo-tty/test-stdout-read.out b/test/pseudo-tty/test-stdout-read.out index 3b7fda223d0e6c..e92928d17b1265 100644 --- a/test/pseudo-tty/test-stdout-read.out +++ b/test/pseudo-tty/test-stdout-read.out @@ -1 +1,2 @@ +Hello! diff --git a/test/pseudo-tty/testcfg.py b/test/pseudo-tty/testcfg.py index 96f596d8862b47..8f09bef2e95599 100644 --- a/test/pseudo-tty/testcfg.py +++ b/test/pseudo-tty/testcfg.py @@ -29,12 +29,14 @@ import test import os -from os.path import join, exists, basename, isdir +from os.path import join, exists, basename, dirname, isdir import re +import sys import utils from functools import reduce FLAGS_PATTERN = re.compile(r"//\s+Flags:(.*)") +PTY_HELPER = join(dirname(__file__), 'pty_helper.py') class TTYTestCase(test.TestCase): @@ -108,16 +110,18 @@ def GetSource(self): + open(self.expected).read()) def RunCommand(self, command, env): - input_arg = None + fd = None if self.input is not None and exists(self.input): - input_arg = open(self.input).read() + fd = os.open(self.input, os.O_RDONLY) full_command = self.context.processor(command) + full_command = [sys.executable, PTY_HELPER] + full_command output = test.Execute(full_command, self.context, self.context.GetTimeout(self.mode), env, - faketty=True, - input=input_arg) + stdin=fd) + if fd is not None: + os.close(fd) return test.TestOutput(self, full_command, output, diff --git a/tools/test.py b/tools/test.py index 2247db36359890..ff08749425eb5a 100755 --- a/tools/test.py +++ b/tools/test.py @@ -638,15 +638,10 @@ def RunProcess(context, timeout, args, **rest): prev_error_mode = Win32SetErrorMode(error_mode) Win32SetErrorMode(error_mode | prev_error_mode) - faketty = rest.pop('faketty', False) - pty_out = rest.pop('pty_out') - process = subprocess.Popen( args = popen_args, **rest ) - if faketty: - os.close(rest['stdout']) if utils.IsWindows() and context.suppress_dialogs and prev_error_mode != SEM_INVALID_VALUE: Win32SetErrorMode(prev_error_mode) # Compute the end time - if the process crosses this limit we @@ -658,28 +653,6 @@ def RunProcess(context, timeout, args, **rest): # loop and keep track of whether or not it times out. exit_code = None sleep_time = INITIAL_SLEEP_TIME - output = '' - if faketty: - while True: - if time.time() >= end_time: - # Kill the process and wait for it to exit. - KillTimedOutProcess(context, process.pid) - exit_code = process.wait() - timed_out = True - break - - # source: http://stackoverflow.com/a/12471855/1903116 - # related: http://stackoverflow.com/q/11165521/1903116 - try: - data = os.read(pty_out, 9999) - except OSError as e: - if e.errno != errno.EIO: - raise - break # EIO means EOF on some systems - else: - if not data: # EOF - break - output += data while exit_code is None: if (not end_time is None) and (time.time() >= end_time): @@ -693,7 +666,7 @@ def RunProcess(context, timeout, args, **rest): sleep_time = sleep_time * SLEEP_TIME_FACTOR if sleep_time > MAX_SLEEP_TIME: sleep_time = MAX_SLEEP_TIME - return (process, exit_code, timed_out, output) + return (process, exit_code, timed_out) def PrintError(str): @@ -715,31 +688,12 @@ def CheckedUnlink(name): PrintError("os.unlink() " + str(e)) break -def Execute(args, context, timeout=None, env=None, faketty=False, disable_core_files=False, input=None): +def Execute(args, context, timeout=None, env=None, disable_core_files=False, stdin=None): + (fd_out, outname) = tempfile.mkstemp() + (fd_err, errname) = tempfile.mkstemp() + if env is None: env = {} - if faketty: - import pty - (out_master, fd_out) = pty.openpty() - fd_in = fd_err = fd_out - pty_out = out_master - - if input is not None: - # Before writing input data, disable echo so the input doesn't show - # up as part of the output. - import termios - attr = termios.tcgetattr(fd_in) - attr[3] = attr[3] & ~termios.ECHO - termios.tcsetattr(fd_in, termios.TCSADRAIN, attr) - - os.write(pty_out, input) - os.write(pty_out, '\x04') # End-of-file marker (Ctrl+D) - else: - (fd_out, outname) = tempfile.mkstemp() - (fd_err, errname) = tempfile.mkstemp() - fd_in = 0 - pty_out = None - env_copy = os.environ.copy() # Remove NODE_PATH @@ -758,28 +712,22 @@ def disableCoreFiles(): resource.setrlimit(resource.RLIMIT_CORE, (0,0)) preexec_fn = disableCoreFiles - (process, exit_code, timed_out, output) = RunProcess( + (process, exit_code, timed_out) = RunProcess( context, timeout, args = args, - stdin = fd_in, + stdin = stdin, stdout = fd_out, stderr = fd_err, env = env_copy, - faketty = faketty, - pty_out = pty_out, preexec_fn = preexec_fn ) - if faketty: - os.close(out_master) - errors = '' - else: - os.close(fd_out) - os.close(fd_err) - output = open(outname).read() - errors = open(errname).read() - CheckedUnlink(outname) - CheckedUnlink(errname) + os.close(fd_out) + os.close(fd_err) + output = open(outname).read() + errors = open(errname).read() + CheckedUnlink(outname) + CheckedUnlink(errname) return CommandOutput(exit_code, timed_out, output, errors)