Skip to content

doc: add note about fs.close undefined behavior#30966

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
Closed
ronag wants to merge 6 commits into from
Closed

doc: add note about fs.close undefined behavior #30966

ronag wants to merge 6 commits into from

Conversation

@ronag
Copy link
Member

@ronagronag commented Dec 14, 2019

Basically what the close(2) man page says – if it’s possible that the fd is being used for a syscall in one thread of the threadpool, we should not be calling close() concurrently.

Refs: #30864

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

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Dec 14, 2019
@ronag
Copy link
MemberAuthor

ronag commented Dec 14, 2019

@addaleax

addaleax
addaleax approved these changes Dec 14, 2019
View reviewed changes
@ronag
Copy link
MemberAuthor

ronag commented Dec 14, 2019

We might want to consider adding a ref count on fs ops and provide an unsafe version that does not do so (i.e. use at own risk but with better perf).

@ronag
Copy link
MemberAuthor

ronag commented Dec 14, 2019

Is that disagreement or confusion on the previous comment?

@addaleax
Copy link
Member

addaleax commented Dec 14, 2019

@ronag It’s both … I feel like the suggestion misses a compelling reason to be implemented?

@ronag
Copy link
MemberAuthor

ronag commented Dec 14, 2019

I feel like the suggestion misses a compelling reason to be implemented?

Consider the amount of users that are not aware of this quirk and are ending up unknowingly with code with undefined behaviour.

I'm also sceptical whether that's compelling enough. Probably isn't as bad in practice as I make it sound. Just thought it might be worth to consider.

Trott
Trott reviewed Dec 15, 2019
View reviewed changes
Trott
Trott reviewed Dec 15, 2019
View reviewed changes
Trott
Trott approved these changes Dec 15, 2019
View reviewed changes
Copy link
Member

@TrottTrott left a comment

Choose a reason for hiding this comment

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

LGTM with two small suggestions applied.

ronagand others added 2 commits December 15, 2019 01:22
@ronag@Trott
Update doc/api/fs.md
bc751b3
Co-Authored-By: Rich Trott <[email protected]>
@ronag@Trott
Update doc/api/fs.md
6fa8257
Co-Authored-By: Rich Trott <[email protected]>
@ronag
Copy link
MemberAuthor

ronag commented Dec 15, 2019•
edited
Loading

Added note to closeSync as well.

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 15, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Dec 15, 2019

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4108/

ronag
ronag commented Dec 15, 2019
View reviewed changes
ronag
ronag commented Dec 15, 2019
View reviewed changes
Trott pushed a commit that referenced this pull request Dec 17, 2019
@ronag@Trott
doc: add note about fs.close() about undefined behavior
b376965
Add notes to fs.close and fs.closeSync() about udnefined behavior. PR-URL: #30966 Refs: #30864 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 17, 2019

Landed in b376965

@TrottTrott closed this Dec 17, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@ronag@MylesBorins
doc: add note about fs.close() about undefined behavior
3abcb69
Add notes to fs.close and fs.closeSync() about udnefined behavior. PR-URL: #30966 Refs: #30864 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 17, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@ronag@targos
doc: add note about fs.close() about undefined behavior
42ccf22
Add notes to fs.close and fs.closeSync() about udnefined behavior. PR-URL: #30966 Refs: #30864 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@ronag@BethGriggs
doc: add note about fs.close() about undefined behavior
95eb1c2
Add notes to fs.close and fs.closeSync() about udnefined behavior. PR-URL: #30966 Refs: #30864 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@TrottTrottTrott approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@lpincalpincalpinca approved these changes

@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.fsIssues 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.

7 participants

@ronag@addaleax@BridgeAR@Trott@jasnell@lpinca@nodejs-github-bot