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

Fix flaky parallel/test-child-process-exec-kill-throws #12053

Closed
gibfahn opened this issue Mar 26, 2017 · 4 comments
Closed

Fix flaky parallel/test-child-process-exec-kill-throws #12053

gibfahn opened this issue Mar 26, 2017 · 4 comments
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@gibfahn
Copy link
Member

gibfahn commented Mar 26, 2017 •
edited
Loading

  • Version: master
  • Platform: Windows
  • Subsystem: test

Creating this issue to document that we need to work out and fix the issue. Will mark as flaky in a separate PR. EDIT: #12054

See discussion in #11038.

I think there might be a genuine bug with kill() on Windows, so maybe marking it as flaky will keep it on the radar.

cc/ @nodejs/platform-windows
@gibfahn gibfahn added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Mar 26, 2017
@gibfahn gibfahn mentioned this issue Mar 26, 2017
Merged
2 tasks
gibfahn added a commit to gibfahn/node that referenced this issue Mar 27, 2017
@gibfahn
test: mark child-process-exec-kill-throws flaky
66e7dc5 
PR-URL: nodejs#12054
Ref: nodejs#12053
Ref: nodejs#11038
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this issue Mar 28, 2017
@gibfahn @MylesBorins
test: mark child-process-exec-kill-throws flaky
8b2b93f 
PR-URL: #12054
Ref: #12053
Ref: #11038
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@Trott
Copy link
Member

Trott commented Mar 29, 2017

PR to fix coming in a few minutes.

The child process isn't failing because of exceeding maxBuffer. It's failing because it's trying to load internal/child_process but it is not being passed the expose-internals flag. Only the parent gets that flag.

Fixed by moving the require() for that module to where the child process won't execute it.

I'm running a stress test now to make sure these changes actually get rid of the flakiness. (UPDATE: Looks good. https://ci.nodejs.org/job/node-stress-single-test/1146/nodes=win-vs2015/console)

As for how this bug triggers flakiness on Windows (assuming the flakiness on Windows is related to it), I'm not 100% sure, but I do see this: On Windows, it's getting UV_ESRCH a lot, indicating the process is already exited by the time ChildProcess._handle.kill() is called. (This is why .killed is never set. It is only set if the return code is 0 but it's often getting UV_ESRCH on Windows.) Interestingly, @sam-github brought up ESRCH in a comments on the original PR. Maybe it would be clear and obvious to him what the problem is here? Seems like a race condition, of course.

@Trott Trott mentioned this issue Mar 29, 2017
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Mar 29, 2017

Here it is: #12111

@gibfahn
Copy link
Member Author

gibfahn commented Mar 29, 2017

Cross-posting from #12111 (comment)

@Trott this seems like it could be related to #11052, also a child-process-exec test that has issues on windows.

@Trott
Copy link
Member

Trott commented Mar 29, 2017

@Trott this seems like it could be related to #11052, also a child-process-exec test that has issues on windows.

Possibly, but the thing we're seeing there isn't Windows-specific. The bug in the test that that PR fixes seemed Windows specific. The new issue it exposes now that bug is fixed.. that's on all sorts of platforms. Check the CI results.

Trott added a commit to Trott/io.js that referenced this issue Mar 30, 2017
@Trott
test: fix flaky child-process-exec-kill-throws
5e7dbdb 
This is a fix for test-child-process-exec-kill-throws which is currently
flaky on Windows.

A bug in the test was causing the child process to fail for reasons
other than those intended by the test. Instead of failing for exceeding
the `maxBuffer` setting, the test was failing because it was trying to
load `internal/child_process` without being passed the
`expose-internals` flag. Move that module to where only the parent
process (which gets the flag) loads it.

Additionally, improve an assertion message to help debug problems like
this.

Fixes: nodejs#12053
@Trott Trott closed this as completed in a10e657 Mar 31, 2017
italoacasas pushed a commit to italoacasas/node that referenced this issue Apr 10, 2017
@Trott @italoacasas
test: fix flaky child-process-exec-kill-throws
87ca9a6 
This is a fix for test-child-process-exec-kill-throws which is currently
flaky on Windows.

A bug in the test was causing the child process to fail for reasons
other than those intended by the test. Instead of failing for exceeding
the `maxBuffer` setting, the test was failing because it was trying to
load `internal/child_process` without being passed the
`expose-internals` flag. Move that module to where only the parent
process (which gets the flag) loads it.

Additionally, improve an assertion message to help debug problems like
this.

PR-URL: nodejs#12111
Fixes: nodejs#12053
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: fix flaky child-process-exec-kill-throws Trott/io.js
2 participants
@Trott @gibfahn