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.

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

fs/promises: fix fd resource cleanup #35208

Closed

Conversation

bnoordhuis
Copy link
Member

Untested, but the existing code looks just plain wrong - the more so because line 403 does call fs.close.bind(fd).

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Sep 15, 2020
@cjihrig
Copy link
Contributor

cjihrig commented Sep 15, 2020

Looking at git blame, it looks like d00d81e intentionally removed the bind() calls.

@@ -522,7 +522,7 @@ async function lchmod(path, mode) {
throw new ERR_METHOD_NOT_IMPLEMENTED('lchmod()');

const fd = await open(path, O_WRONLY | O_SYMLINK);
return fchmod(fd, mode).finally(fd.close);
return fchmod(fd, mode).finally(fd.close.bind(fd));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fchmod(fd, mode).finally(fd.close.bind(fd));
return fchmod(fd, mode).finally(FunctionPrototypeBind(fd.close, fd));

@mscdex
Copy link
Contributor

mscdex commented Sep 15, 2020

Is there a way we can add a test for this?

@bnoordhuis
Copy link
Member Author

Looking at git blame, it looks like d00d81e intentionally removed the bind() calls.

Right, I did see that commit and I understand how it works but it doesn't look Obviously Right to me. But if other people feel it's great idea, then the thing to do is to at least be consistent. I've changed it to remove the .bind() call on line 403.

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2020
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35208
✔  Done loading data for nodejs/node/pull/35208
----------------------------------- PR info ------------------------------------
Title      fs/promises: fix fd resource cleanup (#35208)
Author     Ben Noordhuis  (@bnoordhuis)
Branch     bnoordhuis:fix-fs-promises-fd-close -> nodejs:master
Labels     fs
Commits    1
 - fs/promise: remove unnecessary Function#bind()
Committers 1
 - Ben Noordhuis 
PR-URL: https://github.com/nodejs/node/pull/35208
Reviewed-By: Colin Ihrig 
Reviewed-By: Masashi Hirano 
Reviewed-By: Anna Henningsen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35208
Reviewed-By: Colin Ihrig 
Reviewed-By: Masashi Hirano 
Reviewed-By: Anna Henningsen 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-04T09:58:54Z: https://ci.nodejs.org/job/node-test-pull-request/33368/
- Querying data for job/node-test-pull-request/33368/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Tue, 15 Sep 2020 13:02:12 GMT
   ✔  Approvals: 3
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/35208#pullrequestreview-489637752
   ✔  - Masashi Hirano (@shisama): https://github.com/nodejs/node/pull/35208#pullrequestreview-490204047
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35208#pullrequestreview-493011764
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 35208
From https://github.com/nodejs/node
 * branch                  refs/pull/35208/merge -> FETCH_HEAD
✔  Fetched commits as d3969003b564..ae6aea04b839
--------------------------------------------------------------------------------
Auto-merging lib/internal/fs/promises.js
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 2818 and retry the command.
[master 3c2a1e9af5] fs/promise: remove unnecessary Function#bind()
 Author: Ben Noordhuis 
 Date: Tue Sep 15 18:40:39 2020 +0200
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
--------------------------------------------------------------------------------
--------------------------------- New Message ----------------------------------
fs/promise: remove unnecessary Function#bind()

PR-URL: #35208
Reviewed-By: Colin Ihrig [email protected]
Reviewed-By: Masashi Hirano [email protected]
Reviewed-By: Anna Henningsen [email protected]

[master b25ae7178e] fs/promise: remove unnecessary Function#bind()
Author: Ben Noordhuis [email protected]
Date: Tue Sep 15 18:40:39 2020 +0200
1 file changed, 1 insertion(+), 1 deletion(-)
✖ b25ae7178e8188d6b3ff3a0841d7aa5b54532e6d
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Invalid subsystem: "fs/promise" subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

Commit Queue action: https://github.com/nodejs/node/actions/runs/351509796

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 7, 2020
aduh95 pushed a commit that referenced this pull request Nov 7, 2020
PR-URL: #35208
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Nov 7, 2020

Landed in f06f3c0

@aduh95 aduh95 closed this Nov 7, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #35208
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #35208
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #35208
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #35208
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants