Skip to content

fs: pass the error if directory already closed - #36243

Merged
aduh95 merged 0 commit into
nodejs:masterfrom
Lxxyx:fix/dir-close-throws
Nov 27, 2020
Merged

fs: pass the error if directory already closed#36243
aduh95 merged 0 commit into
nodejs:masterfrom
Lxxyx:fix/dir-close-throws

Conversation

@Lxxyx

Copy link
Copy Markdown
Member

Asynchronous functions should forward errors via callback.
In the case of Promise, errors can be caught with Promise.catch

Fixes: #36237

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 24, 2020
Comment threadlib/internal/fs/dir.js Outdated
Comment threadlib/internal/fs/dir.js Outdated
Comment threadlib/internal/fs/dir.js Outdated
@Lxxyx
Lxxyxforce-pushed the fix/dir-close-throws branch from 65a8561 to cb6b8a2CompareNovember 24, 2020 09:33
@Lxxyx

Copy link
Copy Markdown
MemberAuthor

Thanks for the guide, the code has been updated. @targos@aduh95

@Lxxyx
Lxxyx requested review from aduh95 and targosNovember 24, 2020 09:35
Comment threadlib/internal/fs/dir.js Outdated
Comment threadtest/parallel/test-fs-opendir.js Outdated
Comment threadtest/parallel/test-fs-opendir.js Outdated
Comment threadtest/parallel/test-fs-opendir.js Outdated
Comment threadtest/parallel/test-fs-opendir.js Outdated
@Lxxyx
Lxxyxforce-pushed the fix/dir-close-throws branch from cb6b8a2 to b0b499fCompareNovember 24, 2020 10:53
@Lxxyx

Copy link
Copy Markdown
MemberAuthor

Thanks for the Review, changes have been pushed. @aduh95

@Lxxyx
Lxxyx requested a review from aduh95November 24, 2020 10:55

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On a second thought, I think the tests could be improved a bit:

Comment threadtest/parallel/test-fs-opendir.js Outdated
Comment threadtest/parallel/test-fs-opendir.js Outdated
Comment threadtest/parallel/test-fs-opendir.js Outdated
@Lxxyx
Lxxyxforce-pushed the fix/dir-close-throws branch from b0b499f to 1fb210cCompareNovember 25, 2020 03:15
@Lxxyx

Copy link
Copy Markdown
MemberAuthor

Thanks, learned a lot about how to test Node.js. The code has been updated @aduh95

@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 25, 2020
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still LGTM

@nodejs-github-bot

nodejs-github-bot commented Nov 25, 2020

Copy link
Copy Markdown
Collaborator

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2020
@github-actionsgithub-actionsBot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 27, 2020
@github-actions

Copy link
Copy Markdown
Contributor
Commit Queue failed
- Loading data for nodejs/node/pull/36243
✔ Done loading data for nodejs/node/pull/36243
----------------------------------- PR info ------------------------------------
Title fs/dir: pass the error to the callback or Promise.catch if already closed (#36243)
Author Lxxyx (@Lxxyx, first-time contributor)
Branch Lxxyx:fix/dir-close-throws -> nodejs:master
Labels author ready, fs
Commits 1
- fs/dir: pass the error to the callback or Promise.catch if already cl…
Committers 1
- Zijian Liu PR-URL: https://github.com/nodejs/node/pull/36243
Fixes: https://github.com/nodejs/node/issues/36237
Reviewed-By: Antoine du Hamel Reviewed-By: Joyee Cheung Reviewed-By: Rich Trott Reviewed-By: Yongsheng Zhang ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36243
Fixes: https://github.com/nodejs/node/issues/36237
Reviewed-By: Antoine du Hamel Reviewed-By: Joyee Cheung Reviewed-By: Rich Trott Reviewed-By: Yongsheng Zhang --------------------------------------------------------------------------------
✔ Last GitHub Actions successful
ℹ Last Full PR CI on 2020-11-25T15:23:50Z: https://ci.nodejs.org/job/node-test-pull-request/34556/
- Querying data for job/node-test-pull-request/34556/
✔ Build data downloaded
✔ Last Jenkins CI successful
ℹ This PR was created on Tue, 24 Nov 2020 06:32:49 GMT
✔ Approvals: 4
✔ - Antoine du Hamel (@aduh95): https://github.com/nodejs/node/pull/36243#pullrequestreview-538317002
✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/36243#pullrequestreview-538603019
✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36243#pullrequestreview-539312063
✔ - Yongsheng Zhang (@ZYSzys): https://github.com/nodejs/node/pull/36243#pullrequestreview-539441282
--------------------------------------------------------------------------------
✔ 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 36243
From https://github.com/nodejs/node
* branch refs/pull/36243/merge -> FETCH_HEAD
✔ Fetched commits as b4c2ff5a3bb5..1fb210c50356
--------------------------------------------------------------------------------
[master 7e448d0bca] fs/dir: pass the error to the callback or Promise.catch if already closed
Author: Zijian Liu Date: Tue Nov 24 14:30:26 2020 +0800
2 files changed, 32 insertions(+), 8 deletions(-)
✔ Patches applied
--------------------------------------------------------------------------------
⚠ Found Fixes: https://github.com/nodejs/node/issues/36237, skipping..
--------------------------------- New Message ----------------------------------
fs/dir: pass the error to the callback or Promise.catch if already closed

Asynchronous functions should forward errors via callback.
In the case of Promise, errors can be caught with Promise.catch

Fixes: #36237

PR-URL: #36243
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Yongsheng Zhang zyszys98@gmail.com

[master d662e3e511] fs/dir: pass the error to the callback or Promise.catch if already closed
Author: Zijian Liu Lxxyxzj@gmail.com
Date: Tue Nov 24 14:30:26 2020 +0800
2 files changed, 32 insertions(+), 8 deletions(-)
✖ d662e3e51170c112b172f51bc028412b83a71ed7
✔ 4:7 Valid 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
✔ 6:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Invalid subsystem: "fs/dir" subsystem
✔ 0:0 Title is formatted correctly. title-format
✖ 0:72 Title must be <= 72 columns. title-length
ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/387516680

@LxxyxLxxyx changed the title fs/dir: pass the error to the callback or Promise.catch if already closedfs: pass the error if directory already closedNov 27, 2020
@Lxxyx

Copy link
Copy Markdown
MemberAuthor

Tried to update the name of Pull Request to avoid "Commit Queue failed" error. @aduh95

@aduh95

Copy link
Copy Markdown
Contributor

Landed in b938f88

@aduh95aduh95 closed this Nov 27, 2020
@aduh95
aduh95force-pushed the fix/dir-close-throws branch from 1fb210c to b938f88CompareNovember 27, 2020 17:47
@aduh95
aduh95 merged commit b938f88 into nodejs:masterNov 27, 2020
@Lxxyx
Lxxyx deleted the fix/dir-close-throws branch December 4, 2020 04:11
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
Pass the error to the callback or returns a rejected Promise instead of
throwing a synchonous error.
Fixes: #36237
PR-URL: #36243
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Dec 7, 2020
targos pushed a commit that referenced this pull request May 1, 2021
Pass the error to the callback or returns a rejected Promise instead of
throwing a synchonous error.
Fixes: #36237
PR-URL: #36243
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
@targostargos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Asynchronous dir.close() throws immediately, if already closed

7 participants

@Lxxyx@nodejs-github-bot@aduh95@Trott@targos@joyeecheung@ZYSzys