[backport v10.x] test: fix pty test hangs on aix by Trott · Pull Request #28826 · nodejs/node · GitHub

Original PRs: #28600 #28516 #28469

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: nodejs/build#1820
Fixes: #28489

PR-URL: #28600
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Sam Roberts vieuxtech@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Noticed more than one 10.x backport PRs with perma-fail AIX tests. Hoping backporting this fixes it....

/ping @bnoordhuis (original author) @BethGriggs (person I'm hoping will get this on v10.x-staging quickly if it ends up fixing the issues)

CI: https://ci.nodejs.org/job/node-test-pull-request/24645/

Looks like AIX tests all passed but then some build/JUnit stuff blew up. Trying again...

CI: https://ci.nodejs.org/job/node-test-pull-request/24646/

AIX tests passed again but then there was an Java/Jenkins failure. 😞

CI: https://ci.nodejs.org/job/node-test-pull-request/24648/

Trying again, but forcing a different AIX host to see if it makes a difference....

see: nodejs/build#1849 (comment)

OK, so this will have to also backport 1245aca to pass, it would seem.

OK, added a second backport commit per above. Let's try CI again....

CI: https://ci.nodejs.org/job/node-test-pull-request/24649/

CI: https://ci.nodejs.org/job/node-test-pull-request/24650/

OK, so tests are passing now and the build problems from the stringbytes tests are no longer a thing...but now there are leftover node processes after the tests and that's resulting in the suite be marked red....

CI: https://ci.nodejs.org/job/node-test-pull-request/24652/

CI: https://ci.nodejs.org/job/node-test-pull-request/24653/

Let's see if adding 3c9c89d from #28469 fixes it...

Hooray! That fixed it! (Will need to re-run because of a spurious failure on another platform, but AIX is green.)

@nodejs/releasers Is it OK to pile up three commits from three different PRs like this? I hope so...

Can you perhaps apply them in order? Or at least check against master because none of the pseudo-tty tests are skipped on AIX on master.

CI: https://ci.nodejs.org/job/node-test-pull-request/24655/

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: - nodejs/build#1820 (comment) - nodejs/build#1847 (comment) PR-URL: nodejs#28469 Backport-PR-URL: nodejs#28826 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Add SKIP status for more tests in stringbytes-external-exceed-max that are failing on AIX. PR-URL: nodejs#28516 Backport-PR-URL: nodejs#28826 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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: nodejs/build#1820 Fixes: nodejs#28489 PR-URL: nodejs#28600 Backport-PR-URL: nodejs#28826 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Can you perhaps apply them in order?

Rebased and applied in the order in which they landed.

CI: https://ci.nodejs.org/job/node-test-pull-request/24657/ ✔️

I'm OK to fast-track

LGTM

+1 to fast track

/ping @nodejs/backporters. AIX tests on v10.x-staging will be red until this lands.

+1 to fast track.

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: - nodejs/build#1820 (comment) - nodejs/build#1847 (comment) PR-URL: #28469 Backport-PR-URL: #28826 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Add SKIP status for more tests in stringbytes-external-exceed-max that are failing on AIX. PR-URL: #28516 Backport-PR-URL: #28826 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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: nodejs/build#1820 Fixes: #28489 PR-URL: #28600 Backport-PR-URL: #28826 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Landed on v10.x-staging

nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. v10.x labels Jul 23, 2019

Trott force-pushed the backport-28600w branch from f55a97c to 91715fe Compare July 23, 2019 21:53

This comment has been minimized.

Sign in to view

sam-github mentioned this pull request Jul 23, 2019

Failures due to java.nio.channels.ClosedChannelException, AIX specific? nodejs/build#1849

Closed

Trott force-pushed the backport-28600w branch from 6ee1342 to c5f7616 Compare July 23, 2019 23:10

Trott force-pushed the backport-28600w branch from c5f7616 to eecd8bd Compare July 24, 2019 04:35

sam-github and others added 3 commits July 23, 2019 23:05

Trott force-pushed the backport-28600w branch from eecd8bd to dc4b64f Compare July 24, 2019 06:07

This comment has been minimized.

Sign in to view

richardlau approved these changes Jul 24, 2019

View reviewed changes

BethGriggs approved these changes Jul 24, 2019

View reviewed changes

sam-github approved these changes Jul 24, 2019

View reviewed changes

mhdawson approved these changes Jul 24, 2019

View reviewed changes

Trott mentioned this pull request Jul 24, 2019

[v10.x backport] test: remove util.inherits() usage #28795

Closed

3 tasks

ZYSzys approved these changes Jul 25, 2019

View reviewed changes

Trott mentioned this pull request Jul 25, 2019

[v10.x backport] tools: replace rollup with ncc #28736

Closed

ZYSzys added the fast-track PRs that do not need to wait for 48 hours to land. label Jul 25, 2019

BethGriggs closed this Jul 25, 2019

Trott deleted the backport-28600w branch January 13, 2022 22:51

[backport v10.x] test: fix pty test hangs on aix by Trott · Pull Request #28826 · nodejs/node · GitHub

Uh oh!

Uh oh!

[backport v10.x] test: fix pty test hangs on aix - #28826

[backport v10.x] test: fix pty test hangs on aix#28826Trott wants to merge 3 commits intonodejs:v10.x-stagingnodejs/node:v10.x-stagingfrom Trott:backport-28600wTrott/io.js:backport-28600wCopy head branch name to clipboard

Conversation

Trott commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

This comment has been minimized.

Trott commented Jul 23, 2019

Uh oh!

Trott commented Jul 23, 2019

Uh oh!

nodejs-github-bot commented Jul 23, 2019

Uh oh!

Trott commented Jul 23, 2019

Uh oh!

nodejs-github-bot commented Jul 23, 2019

Uh oh!

Trott commented Jul 23, 2019

Uh oh!

nodejs-github-bot commented Jul 23, 2019

Uh oh!

Trott commented Jul 23, 2019

Uh oh!

sam-github commented Jul 23, 2019

Uh oh!

Trott commented Jul 23, 2019

Uh oh!

Trott commented Jul 23, 2019

Uh oh!

nodejs-github-bot commented Jul 23, 2019

Uh oh!

nodejs-github-bot commented Jul 24, 2019

Uh oh!

Trott commented Jul 24, 2019

Uh oh!

nodejs-github-bot commented Jul 24, 2019

Uh oh!

nodejs-github-bot commented Jul 24, 2019

Uh oh!

Trott commented Jul 24, 2019

Uh oh!

Trott commented Jul 24, 2019

Uh oh!

richardlau commented Jul 24, 2019

Uh oh!

nodejs-github-bot commented Jul 24, 2019

Uh oh!

Trott commented Jul 24, 2019

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jul 24, 2019 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jul 24, 2019

Uh oh!

Trott commented Jul 25, 2019

Uh oh!

ZYSzys commented Jul 25, 2019

Uh oh!

BethGriggs commented Jul 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[backport v10.x] test: fix pty test hangs on aix#28826
Trott wants to merge 3 commits into
nodejs:v10.x-stagingfrom
Trott:backport-28600w

Trott commented Jul 23, 2019 •
edited
Loading

nodejs-github-bot commented Jul 24, 2019 •
edited by Trott
Loading