Skip to content
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.

Sign up for GitHub

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

Jump to bottom

AIX CI failure: pseudo-tty/test-stderr-stdout-handle-sigwinch #11541

Closed
gibfahn opened this issue Feb 24, 2017 · 10 comments
Closed

AIX CI failure: pseudo-tty/test-stderr-stdout-handle-sigwinch #11541

gibfahn opened this issue Feb 24, 2017 · 10 comments
Labels
aix Issues and PRs related to the AIX platform. test Issues and PRs related to the tests.

Comments

@gibfahn
Copy link
Member

gibfahn commented Feb 24, 2017 •
edited
Loading

  • Version: master
  • Platform: AIX
  • Subsystem: test

pseudo-tty/test-stderr-stdout-handle-sigwinch failed the last two CI runs on the v4.x-staging branch (started by node-daily-v4.x-staging.

https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix61-ppc64/4025/

Failed Build: https://ci.nodejs.org/job/node-test-commit/8092/
Failed Platform: https://ci.nodejs.org/job/node-test-commit-aix/4025
Actual Job: https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix61-ppc64/4025/

not ok 1097 pseudo-tty/test-stderr-stdout-handle-sigwinch
  ---
  duration_ms: 0.211
  severity: fail
  stack: |-
  ...

cc/ @mhdawson @nodejs/platform-aix
@gibfahn gibfahn added aix Issues and PRs related to the AIX platform. test Issues and PRs related to the tests. labels Feb 24, 2017
@Fishrock123
Copy link
Contributor

I still think ptys just don't work right on AIX

@gibfahn
Copy link
Member Author

gibfahn commented Feb 24, 2017

Possibly related pseudo-tty issues:

cc/ @gireeshpunathil who has been looking at these other tests

@Fishrock123
Copy link
Contributor

either that or TTYs don't work right on AIX

maybe just ignore them altogether for AIX? idk

@mhdawson
Copy link
Member

Is this a newly backported test ? Just wondering why it would be failing consistently now versus before (if in fact 2 in a row is consistent :))

@gibfahn
Copy link
Member Author

gibfahn commented Feb 25, 2017

Yes, it was first added in v4.8.0:

https://github.com/nodejs/node/blob/v4.8.0/test/pseudo-tty/test-stderr-stdout-handle-sigwinch.js

@gireeshpunathil
Copy link
Member

@Fishrock123 - other than the intermittent hang issue reported in python bug 29545 and the race condition as described in #9728, I don't see any functional issue for TTYs in AIX.

I am debugging this SIGWINCH issue to see what caused the issue.

@gireeshpunathil
Copy link
Member

I guess the idea of the test case is to make sure the _refreshSize() of the standard streams gets called when SIGWINCH is issued on the process. This is indeed happening, consistently. What is missing again, is the race condition between the child write and the parent read.

I am in the process of experimenting by customizing the child spawn logic in the python parent to see if we can sync up the write-read logic.

@mhdawson
Copy link
Member

Do this test fail every time on 4.x ?

@gireeshpunathil
Copy link
Member

@mhdawson - yes, it is pretty consistent in v4.x The test passes in master, though there is a slight change in the test code (even with nullifying the change, I see the master passing).

Whereas, if I add a small delay in the v4.x case, it is consistently passing, supporting my previous update - that is, there is no new issue here, other than the race condition.

I will come up with a PR to exclude this for v4.x and work on addressing the race condition

@gireeshpunathil
Copy link
Member

For the time exclude request raised through PR #11602

gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Feb 28, 2017
@gireeshpunathil
test: exclude pseudo-tty test pertinent to nodejs#11541
2869949 
This test is newly added to v4.x stream and is consistently failing.
We have a couple of issues with pseudo-tty tests in AIX, and while
the investigation is going on, need to skip this test to make CI green.
MylesBorins pushed a commit that referenced this issue Mar 3, 2017
@gireeshpunathil @MylesBorins
test: exclude pseudo-tty test pertinent to #11541
3b11d89 
This test is newly added to v4.x stream and is consistently failing.
We have a couple of issues with pseudo-tty tests in AIX, and while
the investigation is going on, need to skip this test to make CI green.

PR-URL: #11602
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
@gireeshpunathil @MylesBorins
test: exclude pseudo-tty test pertinent to #11541
4ce9bfb 
This test is newly added to v4.x stream and is consistently failing.
We have a couple of issues with pseudo-tty tests in AIX, and while
the investigation is going on, need to skip this test to make CI green.

PR-URL: #11602
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this issue Mar 9, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this issue Mar 20, 2017
@gireeshpunathil @italoacasas
test: delay child exit in AIX for pseudo-tty tests
560d8ee 
The tests in pseudo-tty takes the form of child node writing some data
and exiting, while parent python consume them through pseudo tty
implementations, and validate the result.

While there is no synchronization between child and parent, this works
for most platforms, except AIX, where the child exits even before the
parent could setup the read loop, under race conditions

Fixing the race condition is ideally done through sending ACK messages
to and forth, but involves massive changes and have side effect. The
workaround is to address them in AIX alone, by adding a reasonable
delay.

PR-URL: nodejs#11715
Fixes: nodejs#7973
Fixes: nodejs#9765
Fixes: nodejs#11541
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
@gireeshpunathil @jungx098
test: delay child exit in AIX for pseudo-tty tests
eb0f5c0 
The tests in pseudo-tty takes the form of child node writing some data
and exiting, while parent python consume them through pseudo tty
implementations, and validate the result.

While there is no synchronization between child and parent, this works
for most platforms, except AIX, where the child exits even before the
parent could setup the read loop, under race conditions

Fixing the race condition is ideally done through sending ACK messages
to and forth, but involves massive changes and have side effect. The
workaround is to address them in AIX alone, by adding a reasonable
delay.

PR-URL: nodejs#11715
Fixes: nodejs#7973
Fixes: nodejs#9765
Fixes: nodejs#11541
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
aix Issues and PRs related to the AIX platform. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Fishrock123 @gireeshpunathil @mhdawson @gibfahn