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: introduce `opendir()` and `fs.Dir` #29349

Closed

Conversation

@Fishrock123
Copy link
Member

Fishrock123 commented Aug 28, 2019

This adds long-requested methods for asynchronously interacting and iterating through directory entries by using uv_fs_opendir, uv_fs_readdir, and uv_fs_closedir.

const fs = require('fs');

async function print(path) {
  const dir = await fs.promises.opendir(path);
  for await (const dirent of dir) {
    console.log(dirent.name);
  }
}
print('./').catch(console.error);

fs.opendir() and friends return an fs.Dir, which contains methods for doing reads and cleanup. fs.Dir also has the async iterator symbol exposed.

The read() method and friends only return fs.Dirents for this API.
Having a entry type or doing a stat call is deemed to be necessary in the majority of cases, so just returning dirents seems like the logical choice for a new api.

Reading when there are no more entries returns null instead of a dirent. However the async iterator hides that (and does automatic cleanup).

The code lives in separate files from the rest of fs, this is done partially to prevent over-pollution of those (already very large) files, but also in the case of js allows loading into fsPromises.

Due to async_hooks, this introduces a new handle type of DIRHANDLE.

This PR does not attempt to make complete optimization of this feature. Notable future improvements include:

  • Moving promise work into C++ land like FileHandle.
  • Making uv_dir_t keep a record of its path (or a way to retrieve it).
  • Possibly adding readv() to do multi-entry directory reads.
  • Aliasing fs.readdir to fs.scandir and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: #583
Refs: libuv/libuv#2057

Checklist
  • 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

I am open to discussing the api shapes, I found that this fs.Dir api made it easiest to reconcile the callback and promise apis into "one" reasonably nice (hopefully future proof) api, but I feel folks might have strong feelings about that. It does make a bit of mess in the docs though.

Also please tell me any C++ nonsense I got terribly wrong, haha.

lib/internal/fs/dir.js Outdated Show resolved Hide resolved
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
src/node_dir.h Show resolved Hide resolved
src/node_file.h Outdated Show resolved Hide resolved
test/parallel/test-fs-opendir.js Show resolved Hide resolved
lib/internal/fs/dir.js Show resolved Hide resolved
src/node_dir.h Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Aug 28, 2019

Also: Very nice work! I’m excited to see this happen :)

@addaleax addaleax added semver-minor and removed lib / src labels Aug 28, 2019
src/node_dir.cc Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
src/node_file.h Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/node_dir.cc Outdated Show resolved Hide resolved
@devsnek devsnek dismissed their stale review Aug 28, 2019

requested changes handled

doc/api/fs.md Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Show resolved Hide resolved
doc/api/fs.md Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@devnexen

This comment has been minimized.

Copy link
Contributor

devnexen commented Aug 30, 2019

Few missing commas in documentation spottable with make lint

Copy link
Member

addaleax left a comment

LGTM with a few comments

src/node_dir.cc Outdated Show resolved Hide resolved
src/node_dir.cc Outdated Show resolved Hide resolved
src/node_dir.cc Outdated Show resolved Hide resolved
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 5, 2019
Addresses most of addaleax's 3rd(?) review:
nodejs#29349 (review)
Copy link
Member

addaleax left a comment

Looks good! :shipit:

@nodejs-github-bot

This comment has been minimized.

@nodejs nodejs deleted a comment from nodejs-github-bot Oct 7, 2019
lib/internal/fs/dir.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: #583
Refs: libuv/libuv#2057
@Fishrock123 Fishrock123 force-pushed the Fishrock123:async-readdir-iteration branch from cf9574e to 018fc76 Oct 7, 2019
@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Oct 7, 2019

Anyone else want to before I land this tomorrow? Last CI was all green.

Fishrock123 added a commit that referenced this pull request Oct 8, 2019
This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: #583
Refs: libuv/libuv#2057

PR-URL: #29349
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Oct 8, 2019

Landed in cbd8d71! 😄

@Fishrock123 Fishrock123 closed this Oct 8, 2019
@saghul

This comment has been minimized.

Copy link
Member

saghul commented Oct 9, 2019

Outstanding work @Fishrock123 👏

BridgeAR added a commit that referenced this pull request Oct 9, 2019
This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: #583
Refs: libuv/libuv#2057

PR-URL: #29349
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
addaleax added a commit to nodejs/quic that referenced this pull request Oct 9, 2019
This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: nodejs/node#583
Refs: libuv/libuv#2057

PR-URL: nodejs/node#29349
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
addaleax added a commit to nodejs/quic that referenced this pull request Oct 9, 2019
This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: nodejs/node#583
Refs: libuv/libuv#2057

PR-URL: nodejs/node#29349
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: TBD
@BridgeAR BridgeAR referenced this pull request Oct 10, 2019
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
addaleax added a commit to nodejs/quic that referenced this pull request Oct 11, 2019
This adds long-requested methods for asynchronously interacting and
iterating through directory entries by using `uv_fs_opendir`,
`uv_fs_readdir`, and `uv_fs_closedir`.

`fs.opendir()` and friends return an `fs.Dir`, which contains methods
for doing reads and cleanup. `fs.Dir` also has the async iterator
symbol exposed.

The `read()` method and friends only return `fs.Dirent`s for this API.
Having a entry type or doing a `stat` call is deemed to be necessary in
the majority of cases, so just returning dirents seems like the logical
choice for a new api.

Reading when there are no more entries returns `null` instead of a
dirent. However the async iterator hides that (and does automatic
cleanup).

The code lives in separate files from the rest of fs, this is done
partially to prevent over-pollution of those (already very large)
files, but also in the case of js allows loading into `fsPromises`.

Due to async_hooks, this introduces a new handle type of `DIRHANDLE`.

This PR does not attempt to make complete optimization of
this feature. Notable future improvements include:
- Moving promise work into C++ land like FileHandle.
- Possibly adding `readv()` to do multi-entry directory reads.
- Aliasing `fs.readdir` to `fs.scandir` and doing a deprecation.

Refs: nodejs/node-v0.x-archive#388
Refs: nodejs/node#583
Refs: libuv/libuv#2057

PR-URL: nodejs/node#29349
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Oct 15, 2019
introduced in nodejs/node#29349
codebytere added a commit to electron/electron that referenced this pull request Oct 15, 2019
introduced in nodejs/node#29349
codebytere added a commit to electron/electron that referenced this pull request Oct 15, 2019
introduced in nodejs/node#29349
codebytere added a commit to electron/electron that referenced this pull request Oct 16, 2019
introduced in nodejs/node#29349
codebytere added a commit to electron/electron that referenced this pull request Oct 16, 2019
codebytere added a commit to electron/electron that referenced this pull request Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.