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: fix flaky test-fs-stream-construct #34203

Merged
merged 25 commits into from
Jul 7, 2020
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 4, 2020

The test is marked flaky on ARM because it times out on Raspberry Pi
devices in CI. Split the single test file into four separate test files
to avoid timing out.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 4, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member Author

Trott commented Jul 4, 2020

I tried using debuglog('test') to figure out what is going on with this test, but it looks like maybe NODE_DEBUG=test is not set up on the Raspberry Pi devices in CI?

@Trott
Copy link
Member Author

Trott commented Jul 4, 2020

Here's what I'm seeing with console.log and some date stamps:

15:58:04   duration_ms: 240.89
15:58:04   severity: fail
15:58:04   exitcode: -15
15:58:04   stack: |-
15:58:04     timeout
15:58:04     7/4/2020, 10:54:05 PM start test
15:58:04     7/4/2020, 10:54:05 PM waiting for callbacks
15:58:04     7/4/2020, 10:54:05 PM fs open() callback
15:58:04     7/4/2020, 10:54:05 PM WriteStream constructor
15:58:04     7/4/2020, 10:54:05 PM WriteStream open() callback
15:58:04     7/4/2020, 10:54:05 PM inner fs open() callback
15:58:04     7/4/2020, 10:54:05 PM error event callback
15:58:04     7/4/2020, 10:54:06 PM close event callback
15:58:04   ...

So the write-error test runs completely but then doesn't exit and times out 4 minutes later.

Source code for the test as it stands now with those console.log statements: https://github.com/nodejs/node/blob/ae1e6e323ded98796c43f46cdcd8cd1312152840/test/parallel/test-fs-stream-construct-compat-error-write.js

/ping @ronag Any idea what might be going wrong on Raspberry Pi only apparently?

@ronag
Copy link
Member

ronag commented Jul 5, 2020

That seems rather weird. timeout means that it is still stuck in the event loop? What is it waiting for?

@Trott
Copy link
Member Author

Trott commented Jul 5, 2020

That seems rather weird. timeout means that it is still stuck in the event loop? What is it waiting for?

Yes, it would seem to be stuck in the event loop, and I don't know what it's waiting for.... Guess I can add more debug statements and/or try some different things (like explicitly closing files).

@ronag
Copy link
Member

ronag commented Jul 5, 2020

Explicitly closing the files sounds like a good idea. Though, I wonder if there is a deeper bug here somewhere? I'm not sure at what level this kind of thing lives at. libuv?

@Trott
Copy link
Member Author

Trott commented Jul 5, 2020

Explicitly closing the files sounds like a good idea. Though, I wonder if there is a deeper bug here somewhere? I'm not sure at what level this kind of thing lives at. libuv?

The fact that it works everywhere else seems to suggest a bug, although it could have something to do with the tmp dir on these machines being...I forget...a symlink? NFS-mounted? Both?

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Jul 5, 2020

a symlink? NFS-mounted? Both?

Might explain the difference. Though, it would still be a bug, no?

@nodejs-github-bot
Copy link
Collaborator

@@ -31,7 +31,7 @@ tmpdir.refresh();
});
});

fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err) => {
fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => {
debuglog('fs open() callback');
assert.ifError(err);
Copy link
Member

Choose a reason for hiding this comment

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

I think you want the close here, before creating the write stream

Copy link
Member Author

@Trott Trott Jul 5, 2020

Choose a reason for hiding this comment

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

FWIW, putting it where I put it causes the test to pass, so it's definitely that open fd that's keeping the event loop open. I'll move it here and run again.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 5, 2020

ERR_FEATURE_UNAVAILABLE_ON_PLATFORM is incorrectly included in the list
of errors that have never been released. It was added in
67e067e and included in every release
in the 14.x line.

PR-URL: nodejs#34196
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jul 5, 2020

@ronag Still a bug, though, right? If the underlying write stream emits an error, the file handle should not hold the event loop open?

branisha and others added 7 commits July 6, 2020 07:56
C++ linter fails because of unused ArrayBuffer namespace member

PR-URL: nodejs#34212
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs#34167
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
PR-URL: nodejs#34198
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
test-repl-history-navigation fails with NODE_PENDING_DEPRECATION=1.
Replace deprecated repl.inputStream with repl.input.

PR-URL: nodejs#34199
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#33764 (comment)

PR-URL: nodejs#34197
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#34158
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#34158
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott marked this pull request as ready for review July 6, 2020 17:23
@Trott
Copy link
Member Author

Trott commented Jul 6, 2020

I think this is ready for review.

I'll try to create a minimal test case that fails on the Raspberry Pi devices when the close() is missing, and then add that to known_issues.

@Trott
Copy link
Member Author

Trott commented Jul 6, 2020

I'll also remove the wonky console.log() stuff once Raspberry Pi devices have NODE_DEBUG=test set up in CI.

The test is marked flaky on ARM because it times out on Raspberry Pi
devices in CI. Split the single test file into four separate test files
to ease debugging. Add fs.close() to avoid timing out.

Fixes: nodejs#33796

PR-URL: nodejs#34203
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jul 7, 2020

Landed in 772fdb0

@MylesBorins
Copy link
Contributor

This change is failing on v14.x. Would you be willing to manually backport?

@Trott
Copy link
Member Author

Trott commented Jul 15, 2020

This change is failing on v14.x. Would you be willing to manually backport?

Do you mean it's failing to cherry-pick cleanly or that tests are failing or something else? It's cherry-picking cleanly onto v14.x-staging on fa51ac3, so hopefully it's just that something else that needed to land has now landed there and this is good to go...

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

Successfully merging this pull request may close these issues.

10 participants