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: Worker initialization failure test case #31929

Closed

Conversation

HarshithaKP
Copy link
Member

Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:

  • open a child process with ulimit restriction on file descriptors
  • in the child process, start few workers - more than the fd limit
  • make sure some workers fail, with the expected error type.
  • skip the test in windows, as there is no ulimit there.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Cover the scenario fixed through
nodejs#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 24, 2020
}

// Limit the number of open files, to force workers to fail
let testCmd = 'ulimit -n 128 && ';
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to go with something much smaller than 128, like maybe 8?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott, When I ran a simple node.js code, it created 30+ file descriptors. If I reduce the fd count to 8, it fails at arbitrary point without / before reaching worker creation.

} else {

if (common.isWindows) {
common.skip('ulimit does not work on Windows.');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of skipping the test, just skip the ulimit prefix? As written, I don't think the test won't fail if the error event never happens, so the test might still be runnable under Windows?

Copy link
Member

Choose a reason for hiding this comment

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

don't think the test won't fail if the error event never happens

I think this is an error in the test and therefore after it is resolved it will no longer be runnable on windows.
Also, it doesn't look like there is any equivalent for ulimit on windows so skip looks like the only way.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott , with respect to Windows - is there any merit in running a test that doesn't make any assertion ?

Trott
Trott previously requested changes Feb 24, 2020
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

The test still passes when the child process errors.

test/parallel/test-worker-init-failure.js Outdated Show resolved Hide resolved
test/parallel/test-worker-init-failure.js Outdated Show resolved Hide resolved
test/parallel/test-worker-init-failure.js Outdated Show resolved Hide resolved
test/parallel/test-worker-init-failure.js Show resolved Hide resolved
test/parallel/test-worker-init-failure.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Feb 25, 2020

Locally, this test is failing for me. The error message I'm getting is EMFILE: too many open files, uv_cwd and not Worker initialization failure: EMFILE.

Running CI to see if there are similar problems there or not.....

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott dismissed their stale review February 25, 2020 15:06

issue addressed

@Trott
Copy link
Member

Trott commented Feb 25, 2020

AIX failed in CI (and there may be others as CI isn't finished at the time of this writing) and sure enough it failed with the 1 != 0 message, so maybe we can make the error message more informative and then re-run.

test/parallel/test-worker-init-failure.js Show resolved Hide resolved
test/parallel/test-worker-init-failure.js Outdated Show resolved Hide resolved
test/parallel/test-worker-init-failure.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

} else {
// Limit the number of open files, to force workers to fail.
let testCmd = `ulimit -n ${OPENFILES} && `;

Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change

Copy link
Member Author

Choose a reason for hiding this comment

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

@lundibundi, thanks. Removed the extra line.

@lundibundi
Copy link
Member

ping @Trott @nodejs/testing @nodejs/workers.

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 5, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

addaleax commented Apr 5, 2020

Landed in 5b899d6

@addaleax addaleax closed this Apr 5, 2020
addaleax pushed a commit that referenced this pull request Apr 5, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 12, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 22, 2020
Cover the scenario fixed through
#31621
Unfortunately there is no easy way to test this, in a
cross-platform manner. So the approach is:
 - open a child process with ulimit restriction on file descriptors
 - in the child process, start few workers - more than the fd limit
 - make sure some workers fail, with the expected error type.
 - skip the test in windows, as there is no ulimit there.

Refs: #31621

PR-URL: #31929
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants