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

flaky test-regress-GH-814_2 #34527

Closed
Trott opened this issue Jul 27, 2020 · 6 comments
Closed

flaky test-regress-GH-814_2 #34527

Trott opened this issue Jul 27, 2020 · 6 comments
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@Trott
Copy link
Member

Trott commented Jul 27, 2020

  • Version: 15.0.0-pre (current master branch)
  • Platform: fedora-last-latest-x64
  • Subsystem: test, fs, libuv

What steps will reproduce the bug?

Run test/pummel/test-regress-GH-814_2.js on fedora-last-latest-x64 in CI.

How often does it reproduce? Is there a required condition?

Not every time. Running https://ci.nodejs.org/job/node-stress-single-test/151/ right now to see how often.

What is the expected behavior?

The test should pass.

What do you see instead?

The test crashes with -9, which I believe means libuv is complaining about a bad file descriptor.

21:31:32 not ok 2805 pummel/test-regress-GH-814_2
21:31:32   ---
21:31:32   duration_ms: 11.27
21:31:32   severity: crashed
21:31:32   exitcode: -9
21:31:32   stack: |-
21:31:32     /home/iojs/node-tmp/.tmp.2804/GH-814_test.txt
21:31:32     
21:31:32   ...

Additional information

Something similar to this was going on with readstream last year and I think @addaleax fixed it. Maybe fs has the same/similar issue, perhaps when explicit fs.close() is not used?
@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jul 27, 2020
@Trott
Copy link
Member Author

Trott commented Jul 27, 2020

@nodejs/testing @nodejs/fs @nodejs/libuv

@richardlau
Copy link
Member

Looks like a copy-paste error with the RUN_TESTS parameter for that stress run. I've corrected it and started a rebuild: https://ci.nodejs.org/job/node-stress-single-test/152/

@addaleax
Copy link
Member

The test crashes with -9, which I believe means libuv is complaining about a bad file descriptor.

Signal 9 is SIGKILL. Looking at the test, this is most likely due to the process running out of memory.

@Trott
Copy link
Member Author

Trott commented Jul 27, 2020

The test crashes with -9, which I believe means libuv is complaining about a bad file descriptor.

Signal 9 is SIGKILL. Looking at the test, this is most likely due to the process running out of memory.

Yes, I'm more-than-mildly embarrassed that my brain didn't realize that a crash wouldn't be a uv error code but the signal code instead.

So if it's probably running out of memory, then I guess the thing to do might be to add a free memory check before running like we do for some other tests.

@Trott
Copy link
Member Author

Trott commented Jul 27, 2020

So if it's probably running out of memory, then I guess the thing to do might be to add a free memory check before running like we do for some other tests.

But not if we don't/can't replicate the problem, I guess....

@addaleax addaleax mentioned this issue Jul 27, 2020
Closed
3 tasks
@addaleax
Copy link
Member

PR to replace the test(s) with a more modern one: #34530

Trott pushed a commit to addaleax/node that referenced this issue Jul 30, 2020
@addaleax @Trott
test: replace flaky pummel regression tests
475796a 
These tests were written a long time ago, and use the allocation of
large amounts of unused memory as a way to detect use-after-free
problems with Buffers. As a result, the tests are resource-intensive
and may crash because of that.

Replace them with a more modern test. We don’t explicitly try to
*detect* use-after-free conditions, and instead rely on e.g. ASAN
(or the process just crashing hard) to do that for us.

Fixes: nodejs#34527
codebytere pushed a commit that referenced this issue Aug 6, 2020
@addaleax @codebytere
test: replace flaky pummel regression tests
54a4c6a 
These tests were written a long time ago, and use the allocation of
large amounts of unused memory as a way to detect use-after-free
problems with Buffers. As a result, the tests are resource-intensive
and may crash because of that.

Replace them with a more modern test. We don’t explicitly try to
*detect* use-after-free conditions, and instead rely on e.g. ASAN
(or the process just crashing hard) to do that for us.

Fixes: #34527

PR-URL: #34530
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
codebytere pushed a commit that referenced this issue Aug 11, 2020
@addaleax @codebytere
test: replace flaky pummel regression tests
d6b0a40 
These tests were written a long time ago, and use the allocation of
large amounts of unused memory as a way to detect use-after-free
problems with Buffers. As a result, the tests are resource-intensive
and may crash because of that.

Replace them with a more modern test. We don’t explicitly try to
*detect* use-after-free conditions, and instead rely on e.g. ASAN
(or the process just crashing hard) to do that for us.

Fixes: #34527

PR-URL: #34530
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Reviewed-By: Jiawen Geng <[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
flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: replace flaky pummel regression tests addaleax/node
3 participants
@Trott @addaleax @richardlau