FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: fix writeFile AbortSignal does not close file by Linkgoron · Pull Request #37402 · nodejs/node · GitHub

/ node Public

fs: fix writeFile AbortSignal does not close file - #37402

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:fs-cb-signal-no-close
Feb 26, 2021
ConversationCommits (1)ChecksFiles changed
Merged

fs: fix writeFile AbortSignal does not close file#37402
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:fs-cb-signal-no-close

Conversation

Linkgoron commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

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 added the fs Issues and PRs related to the fs subsystem / file system. label Feb 16, 2021

aduh95 commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

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 commented Feb 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

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 commented Feb 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Feb 18, 2021

Copy link
Copy Markdown
Collaborator

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

Linkgoron force-pushed the fs-cb-signal-no-close branch from 6e6229e to a7f00e3 Compare February 18, 2021 18:50

nodejs-github-bot commented Feb 18, 2021

Copy link
Copy Markdown
Collaborator

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

Linkgoron commented Feb 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 commented Feb 20, 2021

Copy link
Copy Markdown
Contributor Author

Reverted tests.

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 reviewed Feb 20, 2021
View reviewed changes
Comment thread lib/fs.js Outdated
Comment thread lib/fs.js Outdated

nodejs-github-bot commented Feb 20, 2021

Copy link
Copy Markdown
Collaborator

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

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 commented Feb 25, 2021

Copy link
Copy Markdown
Contributor Author

Is there anything else that I need to do here?

nodejs-github-bot commented Feb 26, 2021

Copy link
Copy Markdown
Collaborator

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

RaisinTen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 26, 2021
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 <ronagy@icloud.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 force-pushed the fs-cb-signal-no-close branch from cb2f35a to 503d6cc Compare February 26, 2021 15:31

aduh95 commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

Landed in 503d6cc

aduh95 merged commit 503d6cc into nodejs:master Feb 26, 2021
targos pushed a commit that referenced this pull request Feb 28, 2021
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 <ronagy@icloud.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos mentioned this pull request Mar 2, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
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 <ronagy@icloud.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
ronag ronag approved these changes
aduh95 aduh95 approved these changes
marsonya marsonya approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL