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

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

Closed
valango opened this issue Nov 23, 2020 · 3 comments · Fixed by #36243
Closed

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

valango opened this issue Nov 23, 2020 · 3 comments · Fixed by #36243
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@valango
Copy link

valango commented Nov 23, 2020

  • Version: v12.12.0 - v14.15.1
  • Platform: platform-independent
  • Subsystem: lib/internal/fs/dir.js

What steps will reproduce the bug?

Calling asynchronous version of dir.close() with dir already closed, e.g. by it's async iterator.

const fs = require('fs')

const dir = fs.opendirSync('.')  // Using async function in this example just for brevity.

dir.closeSync()     //  In my actual code, here is an async loop over dir.entries() which may terminate in the middle.

try {
  dir.close((error) => {
    if(!error) return
    // We never get ERR_DIR_CLOSED here...
  })
} catch (error) {
    // ... but here instead (but we should see only ERR_INVALID_CALLBACK here).
}

How often does it reproduce? Is there a required condition?

Always, when dir.close() is called redundantly.

What is the expected behavior?

Asynchronous functions should forward errors via callback, unless the provided callback argument itself is invalid.

What do you see instead?

Error is thrown immediately (synchronously), This is especially embarrassing with promises API:

try {
  dir.close().then(() => console.log('closed the dir just now')
  ).catch(error => console.log('something wrong', error))
} catch (error) {
  if(error.code === 'ERR_DIR_CLOSED') console.log('Oh no... again!')
  else console.log('a real surprise!', e)
}

Additional information

If this behaviour is intentional, it should be documented; otherwise I would gladly provide a fix.

@aduh95
Copy link
Contributor

aduh95 commented Nov 23, 2020

That's a bug, it should pass the error to the callback. PR is welcome :)

@aduh95 aduh95 added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. labels Nov 23, 2020
@Lxxyx
Copy link
Member

Lxxyx commented Nov 24, 2020

I'm working on this issue.

@valango
Copy link
Author

valango commented Nov 25, 2020

You guys were quick - I opened this issue at 2am local time and it was solved by the noon, next day. :)
Thank you!

@aduh95 aduh95 closed this as completed in b938f88 Nov 27, 2020
danielleadams pushed a commit that referenced this issue 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>
targos pushed a commit that referenced this issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants