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

fs: check closing_ in FileHandle::Close #39472

Closed
wants to merge 1 commit into from
Closed

fs: check closing_ in FileHandle::Close #39472

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 20, 2021

Fix possible flaky failure. Keep uv_fs_close from being called twice
on the same fd.

Since I'm not able to reproduce the failure locally, I don't yet know if this fixes the flaky failure in CI.

Refs: #39464
Signed-off-by: James M Snell jasnell@gmail.com

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 20, 2021
@jasnell jasnell mentioned this pull request Jul 20, 2021
Closed
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnell jasnell changed the title fs: closing closing_ in FileHandle::Close fs: check closing_ in FileHandle::Close Jul 20, 2021
@jasnell jasnell force-pushed the maybe-fix-flaky-filehandle-readablestream-test branch from e77b5fd to 71343ea Compare July 20, 2021 15:30
@nodejs-github-bot

This comment has been minimized.

Sign in to view
mcollina
mcollina approved these changes Jul 20, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member Author

jasnell commented Jul 20, 2021

Ok, I think I got it properly identified. The FileHandle::DoShutdown was not checking to see if the FileHandle was already closed or closing.

@jasnell
fs: check closing_ in FileHandle::Close
39ea214 
Fix possible flaky failure. Keep uv_fs_close from being called twice
on the same fd.

Refs: nodejs#39464
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell force-pushed the maybe-fix-flaky-filehandle-readablestream-test branch from 72a4621 to 39ea214 Compare July 20, 2021 21:19
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jul 20, 2021
danbev pushed a commit that referenced this pull request Jul 22, 2021
@jasnell @danbev
fs: check closing_ in FileHandle::Close
f1d3533 
Fix possible flaky failure. Keep uv_fs_close from being called twice
on the same fd.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39472
Refs: #39464
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@danbev
Copy link
Contributor

danbev commented Jul 22, 2021

Landed in f1d3533.

@danbev danbev closed this Jul 22, 2021
targos pushed a commit that referenced this pull request Jul 25, 2021
@jasnell @targos
fs: check closing_ in FileHandle::Close
a04f9a6 
Fix possible flaky failure. Keep uv_fs_close from being called twice
on the same fd.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39472
Refs: #39464
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
Merged
richardlau pushed a commit that referenced this pull request Jul 29, 2021
@jasnell @richardlau
fs: check closing_ in FileHandle::Close
f8a3c4a 
Fix possible flaky failure. Keep uv_fs_close from being called twice
on the same fd.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39472
Refs: #39464
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
@jasnell @BethGriggs
fs: check closing_ in FileHandle::Close
72ad6d3 
Fix possible flaky failure. Keep uv_fs_close from being called twice
on the same fd.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39472
Refs: #39464
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@mcollina mcollina mcollina 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. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasnell @nodejs-github-bot @danbev @mcollina @tniessen @targos