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.

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

test,benchmark: stabilize child-process #13457

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 4, 2017

also some cleanup

Ref: #12817

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test,benchmark

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. labels Jun 4, 2017
@refack refack added the test Issues and PRs related to the tests. label Jun 4, 2017
@refack refack self-assigned this Jun 4, 2017
@refack
Copy link
Contributor Author

refack commented Jun 4, 2017

if (process.platform !== 'win32')
messagesLength.push(32768);
const bench = common.createBenchmark(main, {
// Windows does not support arguments that long
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, feel free to ignore: I would replace arguments with command lines as Windows does not really have a concept of arguments. You could even be more specific, Windows does not support command lines longer than 8191 characters (there is no reason to believe that Microsoft is going to raise that limit).

child_process.execSync(`taskkill /f /t /pid ${child.pid}`);
if (isWindows) {
// Sometimes there's a yes.exe process left hanging around on Windows.
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try should get rid of this error, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, just wrote #13457 (comment)

@refack
Copy link
Contributor Author

refack commented Jun 4, 2017

@nodejs/platform-windows @gireeshpunathil help me think about this
The offending code was introduced in anchnk@ec83210

Today in https://ci.nodejs.org/job/node-test-binary-windows/8986/RUN_SUBSET=2,VS_VERSION=vs2015,label=win2012r2/console I got:

not ok 370 sequential/test-benchmark-child-process
  ---
  duration_ms: 0.623
  severity: fail
  stack: |-
    
    child_process\child-process-exec-stdout.js
    child_process\child-process-exec-stdout.js dur=0 len=1: 0
    ERROR: The process with PID 3816 (child process of PID 3692) could not be terminated.
    Reason: There is no running instance of the task.
    child_process.js:611
        throw err;
        ^
    
    Error: Command failed: taskkill /f /t /pid 3816
    ERROR: The process with PID 3816 (child process of PID 3692) could not be terminated.
    Reason: There is no running instance of the task.
    
    
        at checkExecSyncError (child_process.js:568:13)
        at Object.execSync (child_process.js:608:13)
        at Timeout._onTimeout (c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015\label\win2012r2\benchmark\child_process\child-process-exec-stdout.js:36:21)
        at ontimeout (timers.js:488:11)
        at tryOnTimeout (timers.js:323:5)
        at Timer.listOnTimeout (timers.js:283:5)
    assert.js:60
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at ChildProcess.child.on (c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015\label\win2012r2\test\sequential\test-benchmark-child-process.js:19:10)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:197:12)
  ...

@refack
Copy link
Contributor Author

refack commented Jun 4, 2017

Kill stress after 1000 good iterations. I call "stable".

@refack
Copy link
Contributor Author

refack commented Jun 6, 2017

Prepare for landing: https://ci.nodejs.org/job/node-test-pull-request/8524/

also some cleanup

PR-URL: nodejs#13457
Refs: nodejs#12817
Reviewed-By: Tobias Nießen <[email protected]>
@refack refack force-pushed the more-stable-test-benchmark-child branch from b0fff66 to 8d2bd5f Compare June 7, 2017 20:27
@refack
Copy link
Contributor Author

refack commented Jun 7, 2017

Landed in 8d2bd5f

@refack refack merged commit 8d2bd5f into nodejs:master Jun 7, 2017
@refack
Copy link
Contributor Author

refack commented Jun 7, 2017

Extra sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6463/

@refack refack deleted the more-stable-test-benchmark-child branch June 7, 2017 20:29
addaleax pushed a commit that referenced this pull request Jun 10, 2017
also some cleanup

PR-URL: #13457
Refs: #12817
Reviewed-By: Tobias Nießen <[email protected]>
@addaleax addaleax mentioned this pull request Jun 10, 2017
@refack refack removed their assignment Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants