unix,win: add uv_fs_{open,read,close}_dir() #2057
Conversation
|
Anyone have any thoughts on the points in the OP? |
|
@cjihrig I think it would be more libuv-style to pass
Impacts in what way? |
I didn't mean anything too drastic. Adding fields to a struct that is shared between all fs operations means increased memory on all operations. If we initialize the new fields to some value, then it's also a (very minor) performance hit. |
|
Not to nag but any update? |
|
@cjihrig Can It may be worthwhile to have this return/use a non- |
|
Not 100% sure this would work but this is what I had in mind: Fishrock123/libuv@91e5a09...06fa01b (Compiles but tests have some failures.) |
|
@cjihrig Any thoughts? |
|
@Fishrock123 sorry for the delay. Looking at this again I'm thinking... Instead of adding the fields Maybe the I'll take another look at this tonight with fresh eyes. |
|
Hey @cjihrig I still don't see mutex around the call to linux readdir()... |
cjihrig
force-pushed
the
readdir
branch
3 times, most recently
from
99c1f24 to
2fb2d05
Compare
|
I've refactored the unix implementation (including adding the mutex), and I think I've got this to a place where it can land on the v1.x branch. I still want to do a bit more cleanup, and I haven't updated the Windows implementation yet. |
cjihrig
force-pushed
the
readdir
branch
11 times, most recently
from
9050b07 to
18fc857
Compare
|
CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1324/. Just unrelated failures on centos6. |
|
Hey @bnoordhuis @cjihrig Thanks for your replies!
IMHO there is quite a difference with concurrent Note that the reason that Anyway, if you only care for how Node.js uses this API then I am sure it can handle synchronization on its own, but as a general library this is a TMI API :)
Oh I just noticed that dirents are passed on the dir structure itself. I would actually expect that dirents and ndirents be passed to the uv_fs_readdir call as arguments, and store it in req->ptr as a request scope struct. And actually, since the underlying impl is producing entries one by one anyway, isn't it best to design the API to return a single entry at a time? that would simplify it even further for the caller. Thanks! |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Nice, LGTM. Since Colin wrote it, I'll defer to him on whether or not to make allowances for thread-safety, but I think it's fine the way it is.
uv_fs_scandir() has an iterator interface for pulling out the results but the actual work is done in a single round-trip through the thread pool. uv_readdir() is different because it segments the work (i.e., does multiple round-trips.) After thinking about it some more, I think the uv_readdir() interface is fine. I can live with it being slightly different from everything else. |
|
Hey @bnoordhuis @cjihrig I had a second thought about what I wrote above. I still think it is safer and simpler to the caller to implement with a mutex internally, but I also understood that concurrent calls to Anyway, I also think the PR looks good. Thanks! |
Co-authored-by: Julien Gilli <[email protected]> Co-authored-by: Jeremy Whitlock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: libuv#2057 Refs: joyent/libuv#1430 Refs: joyent/libuv#1521 Refs: joyent/libuv#1574 Refs: libuv#175 Refs: nodejs/node#583 Refs: libuv#416 Refs: libuv#170
Co-authored-by: Julien Gilli <[email protected]> Co-authored-by: Jeremy Whitlock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: libuv#2057 Refs: joyent/libuv#1430 Refs: joyent/libuv#1521 Refs: joyent/libuv#1574 Refs: libuv#175 Refs: nodejs/node#583 Refs: libuv#416 Refs: libuv#170
|
Squashed the commits down. Final CI before landing with no related failures: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1327/ Landing. Finally 🎉. Thanks everyone involved in this PR and all of its previous iterations. |
Co-authored-by: Julien Gilli <[email protected]> Co-authored-by: Jeremy Whitlock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: libuv#2057 Refs: joyent/libuv#1430 Refs: joyent/libuv#1521 Refs: joyent/libuv#1574 Refs: libuv#175 Refs: nodejs/node#583 Refs: libuv#416 Refs: libuv#170
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#583 Refs: libuv/libuv#2057
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
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 <[email protected]> Reviewed-By: David Carlier <[email protected]>
Co-authored-by: Julien Gilli <[email protected]> Co-authored-by: Jeremy Whitlock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: libuv/libuv#2057 Refs: joyent/libuv#1430 Refs: joyent/libuv#1521 Refs: joyent/libuv#1574 Refs: libuv/libuv#175 Refs: nodejs/node#583 Refs: libuv/libuv#416 Refs: libuv/libuv#170
Co-authored-by: Julien Gilli <[email protected]> Co-authored-by: Jeremy Whitlock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: libuv/libuv#2057 Refs: joyent/libuv#1430 Refs: joyent/libuv#1521 Refs: joyent/libuv#1574 Refs: libuv/libuv#175 Refs: nodejs/node#583 Refs: libuv/libuv#416 Refs: libuv/libuv#170
6 participants
WIP continuation of #416
Details
Some questions about the current API design:uv_fs_opendir()currently allocates auv_dir_tthat is used when reading and closing the directory. It's not clear to me where the matchingfree()should live (uv_fs_req_cleanup(),uv_fs_close_dir(), both, other). It might be better to pass theuv_dir_ttouv_fs_opendir()to avoid the issue.How should errors inuv__fs_readdir()be handled? Memory is allocated viauv__strdup(). It can be freed viauv_fs_req_cleanup(). But, what condition should the outputs be left in if one of thereaddir()calls fails in the middle of the call touv__fs_readdir()?In general, I think this attaches too much new information touv_fs_s, which impacts all otherfsoperations.