From 84e4d056ca172c09bf79fe98e9be7c8995c20e06 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 28 Jun 2019 09:36:30 -0700 Subject: [PATCH 1/5] test: skip tests related to CI failures on AIX These tests seem to trigger failures in the entire CI job (not just the test) on AIX. Skip them to see if that helps alleviate spurious failures in node-test-commit-aix (and the upstream PR and commit test jobs). See: - https://github.com/nodejs/build/issues/1820#issuecomment-505998851 - https://github.com/nodejs/build/issues/1847#issuecomment-504210708 PR-URL: https://github.com/nodejs/node/pull/28469 Backport-PR-URL: https://github.com/nodejs/node/pull/29599 Reviewed-By: Michael Dawson Reviewed-By: Daniel Bevenius Reviewed-By: Rich Trott --- .../test-stringbytes-external-exceed-max.js | 5 +++++ test/message/message.status | 3 +++ test/pseudo-tty/pseudo-tty.status | 4 ++++ test/sequential/sequential.status | 3 +++ 4 files changed, 15 insertions(+) diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js index 319a8a93558c45..87c963e23af20d 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js @@ -5,6 +5,11 @@ const skipMessage = 'intensive toString tests due to memory confinements'; if (!common.enoughTestMem) common.skip(skipMessage); +// See https://github.com/nodejs/build/issues/1820#issuecomment-505998851 +// See https://github.com/nodejs/node/pull/28469 +if (process.platform === 'aix') + common.skip('flaky on AIX'); + const binding = require(`./build/${common.buildType}/binding`); const assert = require('assert'); diff --git a/test/message/message.status b/test/message/message.status index 1a4a0e3adc2727..d7bd7f48c9faf8 100644 --- a/test/message/message.status +++ b/test/message/message.status @@ -16,3 +16,6 @@ prefix message [$system==freebsd] +[$system==aix] +# https://github.com/nodejs/node/pull/28469 +vm_dont_display_syntax_error: SKIP diff --git a/test/pseudo-tty/pseudo-tty.status b/test/pseudo-tty/pseudo-tty.status index 5c8114692d9447..a945d532fb5937 100644 --- a/test/pseudo-tty/pseudo-tty.status +++ b/test/pseudo-tty/pseudo-tty.status @@ -3,6 +3,10 @@ prefix pseudo-tty [$system==aix] # being investigated under https://github.com/nodejs/node/issues/9728 test-tty-wrap : FAIL, PASS +# https://github.com/nodejs/build/issues/1820#issuecomment-505998851 +# https://github.com/nodejs/node/pull/28469 +console-dumb-tty: SKIP +test-fatal-error: SKIP [$system==solaris] # https://github.com/nodejs/node/pull/16225 - `ioctl(fd, TIOCGWINSZ)` seems diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index 1af493c3a052a6..e4f92aa96bb261 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -25,3 +25,6 @@ test-http2-settings-flood: PASS,FLAKY [$system==freebsd] [$system==aix] +# https://github.com/nodejs/node/pull/28469 +test-async-wrap-getasyncid: SKIP +test-buffer-creation-regression: SKIP From 3ebdd7035119202aa24d37fda6fe200ae4ac5ee9 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Tue, 2 Jul 2019 14:35:18 -0700 Subject: [PATCH 2/5] test: skip stringbytes-external-exceed-max on AIX Add SKIP status for more tests in stringbytes-external-exceed-max that are failing on AIX. PR-URL: https://github.com/nodejs/node/pull/28516 Backport-PR-URL: https://github.com/nodejs/node/pull/29599 Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Colin Ihrig Reviewed-By: Michael Dawson --- test/addons/addon.status | 19 +++++++++++++++++++ .../test-stringbytes-external-exceed-max.js | 5 ----- 2 files changed, 19 insertions(+), 5 deletions(-) create mode 100644 test/addons/addon.status diff --git a/test/addons/addon.status b/test/addons/addon.status new file mode 100644 index 00000000000000..ee094f4e514cb0 --- /dev/null +++ b/test/addons/addon.status @@ -0,0 +1,19 @@ +prefix addons + +[true] # This section applies to all platforms + +[$system==aix] +# https://github.com/nodejs/build/issues/1820#issuecomment-505998851 +# https://github.com/nodejs/node/pull/28469 +# https://github.com/nodejs/node/pull/28516 +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js: SKIP + +# https://github.com/nodejs/node/pull/28516 +stringbytes-external-exceed-max/test-stringbytes-external-at-max: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-2: SKIP +stringbytes-external-exceed-max/test-stringbytes-external-exceed-max: SKIP diff --git a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js index 87c963e23af20d..319a8a93558c45 100644 --- a/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js +++ b/test/addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js @@ -5,11 +5,6 @@ const skipMessage = 'intensive toString tests due to memory confinements'; if (!common.enoughTestMem) common.skip(skipMessage); -// See https://github.com/nodejs/build/issues/1820#issuecomment-505998851 -// See https://github.com/nodejs/node/pull/28469 -if (process.platform === 'aix') - common.skip('flaky on AIX'); - const binding = require(`./build/${common.buildType}/binding`); const assert = require('assert'); From f8b2ffda63691779275e4d2584a19c872f89b88b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 8 Jul 2019 20:52:37 +0200 Subject: [PATCH 3/5] 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 Backport-PR-URL: https://github.com/nodejs/node/pull/29599 Reviewed-By: Richard Lau Reviewed-By: Sam Roberts Reviewed-By: Anna Henningsen --- test/pseudo-tty/pseudo-tty.status | 8 --- 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 | 15 +++-- tools/test.py | 78 ++++------------------ 6 files changed, 123 insertions(+), 78 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 a945d532fb5937..0a302b2d56d107 100644 --- a/test/pseudo-tty/pseudo-tty.status +++ b/test/pseudo-tty/pseudo-tty.status @@ -1,13 +1,5 @@ prefix pseudo-tty -[$system==aix] -# being investigated under https://github.com/nodejs/node/issues/9728 -test-tty-wrap : FAIL, PASS -# https://github.com/nodejs/build/issues/1820#issuecomment-505998851 -# https://github.com/nodejs/node/pull/28469 -console-dumb-tty: SKIP -test-fatal-error: 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 c6c93c13b98340..2593e07fdfaa9a 100644 --- a/test/pseudo-tty/testcfg.py +++ b/test/pseudo-tty/testcfg.py @@ -27,11 +27,13 @@ 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 FLAGS_PATTERN = re.compile(r"//\s+Flags:(.*)") +PTY_HELPER = join(dirname(__file__), 'pty_helper.py') class TTYTestCase(test.TestCase): @@ -105,17 +107,18 @@ def GetSource(self): + open(self.expected).read()) def RunCommand(self, command, env): - input = None + fd = None if self.input is not None and exists(self.input): - input = 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) - self.Cleanup() + 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 a51b475b1f23cd..e3c759372a5061 100755 --- a/tools/test.py +++ b/tools/test.py @@ -640,15 +640,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 @@ -660,28 +655,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): @@ -695,7 +668,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): @@ -717,29 +690,12 @@ def CheckedUnlink(name): PrintError("os.unlink() " + str(e)) break -def Execute(args, context, timeout=None, env={}, faketty=False, disable_core_files=False, input=None): - 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 +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 = {} env_copy = os.environ.copy() # Remove NODE_PATH @@ -758,28 +714,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 = file(outname).read() - errors = file(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) From 3c5a25f4e789692bd18d244fbbf451621bfd7e14 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 24 Jul 2019 14:07:55 -0700 Subject: [PATCH 4/5] test: specialize OOM check for AIX Assumption that if memory can be malloc()ed it can be used is not true on AIX. Later access of the allocated pages can trigger SIGKILL if there are insufficient VM pages. Use psdanger() to better estimate available memory. Fixes: https://github.com/nodejs/build/issues/1849 More info: - https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/generalprogramming/sys_mem_alloc.html - https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/p_bostechref/psdanger.html Related to: - https://github.com/nodejs/build/issues/1820#issuecomment-505998851 - https://github.com/nodejs/node/pull/28469 - https://github.com/nodejs/node/pull/28516 PR-URL: https://github.com/nodejs/node/pull/28857 Backport-PR-URL: https://github.com/nodejs/node/pull/29599 Reviewed-By: Michael Dawson Reviewed-By: Beth Griggs Reviewed-By: Rich Trott --- test/addons/addon.status | 14 -------- .../binding.cc | 32 ++++++++++++++++++- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/test/addons/addon.status b/test/addons/addon.status index ee094f4e514cb0..6a3e3519c15741 100644 --- a/test/addons/addon.status +++ b/test/addons/addon.status @@ -3,17 +3,3 @@ prefix addons [true] # This section applies to all platforms [$system==aix] -# https://github.com/nodejs/build/issues/1820#issuecomment-505998851 -# https://github.com/nodejs/node/pull/28469 -# https://github.com/nodejs/node/pull/28516 -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max.js: SKIP - -# https://github.com/nodejs/node/pull/28516 -stringbytes-external-exceed-max/test-stringbytes-external-at-max: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-ascii: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-base64: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-binary: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-hex: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-1-utf8: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max-by-2: SKIP -stringbytes-external-exceed-max/test-stringbytes-external-exceed-max: SKIP diff --git a/test/addons/stringbytes-external-exceed-max/binding.cc b/test/addons/stringbytes-external-exceed-max/binding.cc index 628a6b691376b3..3584ba04746f99 100644 --- a/test/addons/stringbytes-external-exceed-max/binding.cc +++ b/test/addons/stringbytes-external-exceed-max/binding.cc @@ -2,12 +2,42 @@ #include #include +#ifdef _AIX +// AIX allows over-allocation, and will SIGKILL when the allocated pages are +// used if there is not enough VM. Check for available space until +// out-of-memory. Don't allow more then some (large) proportion of it to be +// used for the test strings, so Node & V8 have some space for allocations. +#include +#include + +static void* Allowed(size_t size) { + blkcnt_t allowedBlocks = psdanger(SIGKILL); + + if (allowedBlocks < 1) { + // Already OOM + return nullptr; + } + const size_t BYTES_PER_BLOCK = 512; + size_t allowed = (allowedBlocks * BYTES_PER_BLOCK * 4) / 5; + if (size < allowed) { + return malloc(size); + } + return nullptr; +} +#else +// Other systems also allow over-allocation, but this malloc-and-free approach +// seems to be working for them. +static void* Allowed(size_t size) { + return malloc(size); +} +#endif // _AIX + void EnsureAllocation(const v8::FunctionCallbackInfo &args) { v8::Isolate* isolate = args.GetIsolate(); uintptr_t size = args[0]->IntegerValue(); v8::Local success; - void* buffer = malloc(size); + void* buffer = Allowed(size); if (buffer) { success = v8::Boolean::New(isolate, true); free(buffer); From 9209f480dbe485ca5269b005a54c2f3d4bab3198 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 8 Aug 2019 11:01:42 -0700 Subject: [PATCH 5/5] test: unskip tests that now pass on AIX One skipped test remains, it creates very large Buffer objects, triggering the AIX OOM to kill node and its parent processes. See: https://github.com/nodejs/build/issues/1849#issuecomment-514414165 PR-URL: https://github.com/nodejs/node/pull/29054 Backport-PR-URL: https://github.com/nodejs/node/pull/29599 Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: Rich Trott Reviewed-By: Richard Lau --- test/message/message.status | 2 -- test/sequential/sequential.status | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/test/message/message.status b/test/message/message.status index d7bd7f48c9faf8..bd3f488b7bedd6 100644 --- a/test/message/message.status +++ b/test/message/message.status @@ -17,5 +17,3 @@ prefix message [$system==freebsd] [$system==aix] -# https://github.com/nodejs/node/pull/28469 -vm_dont_display_syntax_error: SKIP diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index e4f92aa96bb261..7b06493f48f0d2 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -25,6 +25,5 @@ test-http2-settings-flood: PASS,FLAKY [$system==freebsd] [$system==aix] -# https://github.com/nodejs/node/pull/28469 -test-async-wrap-getasyncid: SKIP +# https://github.com/nodejs/node/pull/29054 test-buffer-creation-regression: SKIP