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

test: add known issue for fs.open() keeping event loop open #34228

Merged
merged 0 commits into from
Jul 9, 2020
Merged

test: add known issue for fs.open() keeping event loop open #34228

merged 0 commits into from
Jul 9, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 6, 2020 •
edited
Loading

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 6, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott Trott marked this pull request as ready for review July 7, 2020 00:04
@Trott
Copy link
Member Author

Trott commented Jul 7, 2020 •
edited
Loading

This seems ready for review. /ping @ronag

@nodejs/fs: This is a bug, not expected behavior, right? Or is it possibly-expected behavior in certain conditions like NFS-mounted filesystems?

@nodejs/fs and @nodejs/build: Any idea why this keeps the event loop open and times out on Raspberry Pi devices but works fine everywhere else? I'm guessing it has nothing to do with the Pi devices per se but is because those devices are running the tests on NFS-mounted file systems or something like that.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
jasnell
jasnell approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM for landing the test. No idea on the issue itself but definitely looks like a bug.

@github-actions github-actions bot mentioned this pull request Jul 7, 2020
Open
29 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32231/

@rvagg
Copy link
Member

rvagg commented Jul 7, 2020

those devices are running the tests on NFS-mounted file systems

that would be my guess, although speed is another factor that regularly comes in to play - things not happening in the order you expect because everything is slowed down so much so the system gets tripped up. It could be that an unref is getting tripped up in the process and happens too late, or too soon, or something.

If you close the fd in the callback then it exits properly, I'm not sure if that helps at all:

fs.close(fd, function (err) { console.log('closed', err) })

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 7, 2020
bnoordhuis
bnoordhuis approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I'm trying to make sense of this:

  1. Callback to fs.open() is called but the test doesn't exit (on the RPi 1's)
  2. Call fs.close() from the callback and it exits?

If someone can get me access to one of the affected machines, I'll investigate. Are gdb and strace installed?

test/known_issues/test-fs-open-no-close.js Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

If someone can get me access to one of the affected machines, I'll investigate. Are gdb and strace installed?

@bnoordhuis You're a member of the Build WG so should have access to the secrets repo as documented in https://github.com/nodejs/build/blob/master/doc/ssh.md. The RPI's are behind a jumpbox -- the ansible script described in the ssh doc will write that into your ssh config file for easy access, otherwise the proxy command is templated in https://github.com/nodejs/build/blob/d4b074cd221cce329ee1f31292e3114845390a56/ansible/ansible.cfg#L24 with the hosts defined in https://github.com/nodejs/build/blob/master/ansible/inventory.yml.

test-requireio_williamkapke-debian10-arm64_pi3-1 (from https://ci.nodejs.org/job/node-test-binary-arm-12+/6210/RUN_SUBSET=0,label=pi3-docker/) looks like one of the machines to exhibit the issue. I just checked and that one seems to have gdb and strace installed.

ronag
ronag reviewed Jul 7, 2020
View reviewed changes
tools/lint-md.js Outdated Show resolved Hide resolved
@Trott Trott force-pushed the missing-fs-close branch from 387586a to 24674df Compare July 7, 2020 19:58
@github-actions github-actions bot mentioned this pull request Jul 8, 2020
Open
18 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32257/

@Trott Trott mentioned this pull request Jul 8, 2020
Closed
@github-actions github-actions bot mentioned this pull request Jul 9, 2020
Open
20 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32258/

@Trott
Copy link
Member Author

Trott commented Jul 9, 2020

Landed in 6ae1b9c

@Trott Trott closed this Jul 9, 2020
@Trott Trott force-pushed the missing-fs-close branch from ca3017f to 6ae1b9c Compare July 9, 2020 01:11
@Trott Trott merged commit 6ae1b9c into nodejs:master Jul 9, 2020
@Trott Trott deleted the missing-fs-close branch July 9, 2020 01:11
This was referenced Jul 10, 2020
Open
Open
Open
Open
Open
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
@Trott @MylesBorins
test: add known issue for fs.open() keeping event loop open
8811058 
PR-URL: #34228
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
@Trott @MylesBorins
test: add known issue for fs.open() keeping event loop open
ec9b49a 
PR-URL: #34228
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott @addaleax
test: add known issue for fs.open() keeping event loop open
a5553dc 
PR-URL: #34228
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
@addaleax
Copy link
Member

Fwiw, this test is flaky on v12.x in the sense that apparently, it does succeed there occasionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Trott @nodejs-github-bot @rvagg @richardlau @addaleax @bnoordhuis @jasnell @ronag