Anyone have any thoughts on the points in the OP?

@cjihrig I think it would be more libuv-style to pass uv_dir_t to uv_fs_opendir()?

which impacts all other fs operations.

Impacts in what way?

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 uv_dir_t "extend" (not that C has "extend" functionality) uv_fs_t? (Or have the new struct have it as a field.)

It may be worthwhile to have this return/use a non-uv_fs_t struct, based on what you said.

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 dir, dirents, and nentries to the uv_fs_t, maybe we could just add the uv_dir_t* to uv_fs_t, and add everything else to the uv_dir_t. That would prevent the problem of adding a bunch of things to uv_fs_t, while keeping a similar API to the rest of the fs functions.

Maybe the uv_dir_t* could even go in the UV_FS_PRIVATE_FIELDS to avoid breaking ABI.

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()...

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.

Updated status:

• Both Unix and Windows code updated.
• No related CI failures (https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1319/).
• ABI compatible, and targeting v1.x branch.
• Documentation written.

Thanks for the review Ben. I'll follow up on the code comments later today.

I think it's been discussed already but just in case, we're okay with it being a bit of an odd duck? It's a streaming API whereas everything else in fs.c is one-shot.

I'm OK with the odd duck status. I think it makes the most sense to include this functionality with the other fs functions instead of making it a lone wolf odd duck. I guess I just assumed that detail had been ironed out at some point in the past 5 years 😄

I think it's been discussed already but just in case, we're okay with it being a bit of an odd duck? It's a streaming API whereas everything else in fs.c is one-shot.

You mean something more in the shape of uv_fs_scandir*...?

Docs added. Removing the WIP label.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1324/. Just unrelated failures on centos6.

Hey @bnoordhuis @cjihrig

Thanks for your replies!

That isn't so different from multiple uv_fs_write() requests for the same file descriptor, for example. We don't make any promises about correctness or order there, either.

IMHO there is quite a difference with concurrent uv_fs_write() calls - the worst outcome is that the caller will not know which write won - which is expected for any unsynchronized write operations in any storage API known to man, and also the case between different processes. With concurrent uv_fs_readdir(), which is an unsynchronized read operation, the effect is unspecified and will likely include memory corruptions and crash depending on the underlying implementation.

Note that the reason that readdir_r() was deprecated by POSIX is that struct dirent is poorly defined in a cross platform way and the caller cannot reliably send a length for the provided d_name buffer. I think that this POSIX behavior is unfortunate to have been created in the first place (just needed to standardize d_reclen?), and a modern library like libuv should not expose these kind of unneeded POSIX complexities.

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 :)

Because the code currently isn't equipped to deal with that. The mutex guards the dir->dir handle but not the dir->dirents array. Two concurrent threads will overwrite each other's entries in that array.

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!

You mean something more in the shape of uv_fs_scandir*...?

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 uv_fs_readdir() would probably not make sense from a usage perspective because it's a streaming API that maintains the dir position inside the DIR structure (there is no POSIX API for a "stateless" call to readdir(dir, position) like we have for files read(file, offset)). So the caller will inevitably have to serialize the readdir calls to collect all the entries, and therefore the mutex overhead is redundant.

Anyway, I also think the PR looks good.
Too bad we can't just protect the calling code from making that mistake.

Thanks!

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.