GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: pass the error if directory already closed by Lxxyx · Pull Request #36243 · nodejs/node

Conversation

@Lxxyx

@Lxxyx Lxxyx commented Nov 24, 2020

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Nov 24, 2020
targos
targos reviewed Nov 24, 2020
View reviewed changes
Comment thread lib/internal/fs/dir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/fs/dir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/fs/dir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Lxxyx
Lxxyx force-pushed the fix/dir-close-throws branch from 65a8561 to cb6b8a2 Compare November 24, 2020 09:33
@Lxxyx

Lxxyx commented Nov 24, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Lxxyx
Lxxyx requested review from aduh95 and targos November 24, 2020 09:35
aduh95
aduh95 reviewed Nov 24, 2020
View reviewed changes
Comment thread lib/internal/fs/dir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Lxxyx
Lxxyx force-pushed the fix/dir-close-throws branch from cb6b8a2 to b0b499f Compare November 24, 2020 10:53
@Lxxyx

Lxxyx commented Nov 24, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Lxxyx
Lxxyx requested a review from aduh95 November 24, 2020 10:55
aduh95
aduh95 approved these changes Nov 24, 2020
View reviewed changes
joyeecheung
joyeecheung approved these changes Nov 24, 2020
View reviewed changes
aduh95
aduh95 reviewed Nov 24, 2020
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-fs-opendir.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Lxxyx
Lxxyx force-pushed the fix/dir-close-throws branch from b0b499f to 1fb210c Compare November 25, 2020 03:15
@Lxxyx

Lxxyx commented Nov 25, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95
aduh95 approved these changes Nov 25, 2020
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Nov 25, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 25, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
joyeecheung
joyeecheung approved these changes Nov 25, 2020
View reviewed changes

@joyeecheung joyeecheung left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Still LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot

nodejs-github-bot commented Nov 25, 2020 •
edited by aduh95
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott approved these changes Nov 26, 2020
View reviewed changes
ZYSzys
ZYSzys approved these changes Nov 26, 2020
View reviewed changes
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 27, 2020
@github-actions github-actions Bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Nov 27, 2020
@github-actions

github-actions Bot commented Nov 27, 2020

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 [email protected]
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Yongsheng Zhang [email protected]

[master d662e3e511] fs/dir: pass the error to the callback or Promise.catch if already closed
Author: Zijian Liu [email protected]
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Lxxyx commented Nov 27, 2020

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

Landed in b938f88

Lxxyx reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 closed this Nov 27, 2020
@aduh95
aduh95 force-pushed the fix/dir-close-throws branch from 1fb210c to b938f88 Compare November 27, 2020 17:47
@aduh95
aduh95 merged commit b938f88 into nodejs:master Nov 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
@Lxxyx @danielleadams
fs: pass ERR_DIR_CLOSED asynchronously to dir.close
744b8aa
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 <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
v15.4.0 proposal #36435
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@Lxxyx @targos
fs: pass ERR_DIR_CLOSED asynchronously to dir.close
1bffd8d
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 <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@targos targos removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@aduh95 aduh95 aduh95 approved these changes
@ZYSzys ZYSzys ZYSzys approved these changes
@targos targos Awaiting requested review from targos

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. fs Issues and PRs related to file-system APIs and the fs module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@Lxxyx @nodejs-github-bot @aduh95 @Trott @targos @joyeecheung @ZYSzys
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.