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: fix writeFile AbortSignal does not close file #37402

Merged
merged 1 commit into from
Feb 26, 2021
Merged

fs: fix writeFile AbortSignal does not close file #37402

merged 1 commit into from
Feb 26, 2021

Conversation

Linkgoron
Copy link
Member

This PR fixes an issue where the callback-style writeFile does not close the fd when the AbortSignal is aborted
(readFile handles this correctly).

I've also "optimized" the read-path to not open the file if the signal is aborted before the file is even opened.

(this PR is different than my other PR, which fixed an issue in promisified writeFile)

  • [] 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-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 16, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 17, 2021

So that's the same bug as in #37393, except in the callback version, correct? It'd be nice to have a test also.

@Linkgoron
Copy link
Member Author

Linkgoron commented Feb 17, 2021 •
edited
Loading

So that's the same bug as in #37393, except in the callback version, correct? It'd be nice to have a test also.

Not exactly the same bug. In the promsified version non-"pre-aborted" are fine (the file closes correctly in the promisifed version if the signal is aborted after the file is opened, and writing has begun).

The issue here is when the write doesn't write everything in one-go, I'm not sure how to simulate it (I tried and couldn't get it to work consistently even with large buffers).

@aduh95
Copy link
Contributor

aduh95 commented Feb 17, 2021

Yeah, indeed, writeAll is always called with a length of data.byteLength to write in one go; I guess this might happen only if there are some OS or hardware limitations in place. Maybe that's something that can be set up a CI?

//cc @nodejs/fs

@Linkgoron
Copy link
Member Author

Linkgoron commented Feb 17, 2021 •
edited
Loading

Yeah, indeed, writeAll is always called with a length of data.byteLength to write in one go; I guess this might happen only if there are some OS or hardware limitations in place. Maybe that's something that can be set up a CI?

@aduh95 going over things again, you're right that another case that had a problem before was if the controller was aborted between the file.open and writeAll - which was also fixed by this PR. This can be seen by executing test-fs-write-file and adding setTimeout(()=>fs.readdir('/dev/fd',(err,list)=>console.log(list.length)),1000) it's 28 after my PR, and 30 if I revert it. Removing the last two tests in the file brings it to 28/28 (as they both abort the controller before the internal open finishes).

However, I'm just not sure how to get all of the file handles in a cross-platform and consistent way through node, I tried using _getActiveHandles and _getActiveRequests which I thought might work, but they didn't.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Linkgoron
Copy link
Member Author

Linkgoron commented Feb 18, 2021 •
edited
Loading

@aduh95 I've added tests. I'm not 100% happy about them, as it's hard for me to know if they'll work consistently... However, on my machine they do work consistently on this branch, and fail consistently on master. I'd be happy to improve them if you have any pointers.

EDIT: clearly doesn't work... I'll try to find another solution.

@Linkgoron
Copy link
Member Author

Reverted tests.

@Linkgoron Linkgoron force-pushed the fs-cb-signal-no-close branch 3 times, most recently from b2faef5 to 1cc6a55 Compare February 20, 2021 20:51
aduh95
aduh95 reviewed Feb 20, 2021
View reviewed changes
lib/fs.js Show resolved Hide resolved
lib/fs.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@Linkgoron Linkgoron force-pushed the fs-cb-signal-no-close branch 2 times, most recently from 1e85f15 to cb2f35a Compare February 25, 2021 15:43
@Linkgoron
Copy link
Member Author

Is there anything else that I need to do here?

@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2021
@aduh95
fs: fix writeFile signal does not close file
503d6cc 
Fix an issue where the writeFile does not close the file
when the signal is aborted.

PR-URL: nodejs#37402
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Feb 26, 2021

Landed in 503d6cc

@aduh95 aduh95 merged commit 503d6cc into nodejs:master Feb 26, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
@targos
fs: fix writeFile signal does not close file
c0cdb83 
Fix an issue where the writeFile does not close the file
when the signal is aborted.

PR-URL: #37402
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos mentioned this pull request Mar 2, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@targos
fs: fix writeFile signal does not close file
f2279f8 
Fix an issue where the writeFile does not close the file
when the signal is aborted.

PR-URL: #37402
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

@aduh95 aduh95 aduh95 approved these changes

@marsonya marsonya marsonya approved these changes

@RaisinTen RaisinTen RaisinTen 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. 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.
7 participants
@Linkgoron @aduh95 @nodejs-github-bot @benjamingr @ronag @marsonya @RaisinTen