fs: uv_fs_{open,read,close}_dir #416

whitlockjc · 2015-06-30T22:42:46Z

This PR is for taking the efforts by @misterdjules in #175 and applying those changes to master instead of v1.x per @saghul's suggestion. You should be able to close #175 in favor of this but I'll leave that decision up to you.

Note: This commit also updates uv__fs_readdir to use readdir instead of the deprecated readdir_r.

whitlockjc · 2015-07-01T15:55:33Z

cc'ing the required people...

/cc @saghul @piscisaureus @bnoordhuis

Fishrock123 · 2015-08-10T18:22:28Z

Pinging @saghul @piscisaureus @bnoordhuis again :)

saghul · 2015-08-11T09:49:22Z

Sorry, but I won't be able to review this anytime soon.

whitlockjc · 2015-08-19T16:23:39Z

I have rebased this PR due to master deviation. Test suite runs great, although fs_event_watch_dir and fs_event_watch_dir_recursive can randomly fail due to #30 (Fixed in #480).

rsms · 2015-08-24T18:23:24Z

+1 — nice work.

eljefedelrodeodeljefe · 2016-05-11T12:50:24Z

@whitlockjc I opened a related issue on the nodejs bug tracker. Without having a stake in this, can you rebase this, so it will be possible to review?

whitlockjc · 2016-05-11T13:51:54Z

You got it. I didn't realize this hadn't been merged.

eljefedelrodeodeljefe · 2016-05-11T14:05:40Z

@saghul marked it v2 - maybe that was the reason. Not sure if it is though. If possible try not breaking ABI, then I guess it'll be easier to land. Would be really useful to land this, imo!

txdv · 2016-05-11T14:06:54Z

It looks like its only adding additional API calls, how is it breaking anything?

eljefedelrodeodeljefe · 2016-05-11T14:08:11Z

I am sure @saghul will clarify. Maybe it was just for roadmap.

whitlockjc · 2016-05-11T14:11:11Z

It's been through a ton of review, there are tests and the ABI was not broken so that's why I was surprised. I'm not sure why it's marked as v2 but it shouldn't be because of the ABI. As for trying not to break the ABI, I don't think people do it unless they have to (at least not after a full pass of code review), as I had to in #590.

Maybe @saghul can chime in.

whitlockjc · 2016-05-11T14:12:29Z

@eljefedelrodeodeljefe Do you mind linking this issue and the one in node? That would be very useful.

eljefedelrodeodeljefe · 2016-05-11T14:14:40Z

You mean me probably, right? It's about providing glob, where those APIs would be necessary nodejs/node#6677

whitlockjc · 2016-05-11T14:19:11Z

Sorry about that, I see it was linked higher in the discussion but I didn't see it.

saghul · 2016-05-11T14:22:40Z

This PR modifies the uv_fs_t structure: https://github.com/libuv/libuv/pull/416/files#diff-56343bd7ad8bf449c2693082fb3e4081R1107 That breaks the ABI, and the reason why I tagged it v2.

eljefedelrodeodeljefe · 2016-05-11T14:25:37Z

include/uv.h

@@ -1096,6 +1104,9 @@ struct uv_fs_s {
  void* ptr;
  const char* path;
  uv_stat_t statbuf;  /* Stores the result of uv_fs_stat() and uv_fs_fstat(). */
+  const uv_dir_t* dir; /* Stores the result of uv_fs_opendir() */
+  uv_dirent_t* dirents;
+  size_t nentries;


I guess it's possible without that addition, say with an own struct right? That of course breaks ABI compatibility.

Yup, I didn't see this. (I actually overlooked it twice because of the styling of the GitHub diff viewer...and this PR is pretty old so I just forgot.)

And where would you put those members or the new struct? FYI, I'll be -1 on bending over backwards for ABI compatibility in new features. That way we'd never start working on v2.

Can you add methods similar to uv_fs_get_statbuf etc. from 0d6525a ?

whitlockjc · 2016-05-11T14:37:23Z

Doh...I stand corrected. (It's been quite a while since this was authored so I guess I forgot, and GitHub's diff viewer didn't help too much as I overlooked it.) Well, it all makes sense now. Mark that up as two v2 PRs from me that require ABI changes. ;) My bad @saghul.

eljefedelrodeodeljefe · 2016-05-11T14:39:50Z

That's fine. We'll get there. Give me a ping when I can support.

whitlockjc · 2016-05-11T14:42:28Z

You got it. I don't mind helping on the node side of things either if you need. I'm already prepared for the node changes related to for #590 when the time comes. :)

guymguym · 2018-04-16T08:30:37Z

@whitlockjc One last comment - I believe that for Linux you need to have a mutex per dir since readdir() is not thread safe and as far as I understand the libuv threadpool will be doing these readdir calls, right?

See what the readdir(3) docs has to say:

In the current POSIX.1 specification (POSIX.1-2008), readdir() is not required to be thread-safe. However, in modern implementations (including the glibc implementation), concurrent calls to readdir() that specify different directory streams are thread-safe. In cases where multiple threads must read from the same directory stream, using readdir() with external synchronization is still preferable to the use of the deprecated readdir_r(3) function. It is expected that a future version of POSIX.1 will require that readdir() be thread-safe when concurrently employed on different directory streams.

whitlockjc · 2018-04-16T20:52:16Z

I thought something similar but figured it would come up in review. Glad you brought it up. I'll get it sorted once I've given enough soak time for further review.

bnoordhuis

Not quite ready yet but getting closer. Thanks for picking this up again.

Question: the use of req->ptr seems unnecessary. Can that be dropped?

include/uv.h

src/unix/fs.c

test/test-fs-readdir.c

src/win/fs.c

bnoordhuis · 2018-04-30T11:02:46Z

test/test-fs-readdir.c

+                  non_empty_readdir_cb);
+  }
+
+  uv_fs_req_cleanup(req);


This isn't safe, is it? req == &readdir_req and that's (re)initialized on line 375.

test/test-fs-readdir.c

cjihrig · 2018-05-26T18:22:14Z

Ping @whitlockjc. This is so close to finally being ready 😄

whitlockjc · 2018-05-29T22:30:55Z

I'll get on it. I was waiting on #590 to be merged and forgot about this one.

rijnhard · 2018-06-19T14:41:45Z

@whitlockjc ping, just making sure this hasn't slipped through the cracks

whitlockjc · 2018-06-19T14:55:30Z

Thanks for the bump @rijnhard, I've just been super busy. I'll make time for this by EOW.

rijnhard · 2018-07-10T14:20:09Z

@whitlockjc ping

whitlockjc · 2018-07-19T14:57:41Z

I'm back, sorry for the delay. Work, life and other OSS. :)

whitlockjc · 2018-07-30T19:40:52Z

Would anyone else like to pick this up and drive it? The review feedback from @guymguym and @bnoordhuis is going to take more time than I can seem to come up with right now.

cjihrig · 2018-08-02T00:42:24Z

Would anyone else like to pick this up and drive it?

I'll pick this up.

guymguym · 2018-08-02T06:56:45Z

@cjihrig Let me know if I can help too

Fishrock123 · 2018-09-21T14:59:53Z

Do we know where master is going yet? If someone were to update this, is master still a reasonable target, or will that never make it into a release?

rijnhard · 2018-10-12T15:02:31Z

This died again didn't it

cjihrig · 2018-10-12T15:08:47Z

It hasn't died again. I promise.

This commit also updates `uv__fs_readdir` to use `readdir` instead of the deprecated `readdir_r`. Co-authored-by: Julien Gilli <jgilli@nodejs.org> Co-authored-by: Jeremy Whitlock <jwhitlock@apache.org> 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 commit also updates `uv__fs_readdir()` to use `readdir()` instead of the deprecated `readdir_r()`. Co-authored-by: Julien Gilli <jgilli@nodejs.org> Co-authored-by: Jeremy Whitlock <jwhitlock@apache.org> 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

cjihrig · 2018-10-29T13:15:28Z

Moving to #2057 with most of @bnoordhuis nits addressed from the last review, as well as some comments of my own.

This commit also updates `uv__fs_readdir()` to use `readdir()` instead of the deprecated `readdir_r()`. Co-authored-by: Julien Gilli <jgilli@nodejs.org> Co-authored-by: Jeremy Whitlock <jwhitlock@apache.org> 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 <jgilli@nodejs.org> Co-authored-by: Jeremy Whitlock <jwhitlock@apache.org> 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 <jgilli@nodejs.org> Co-authored-by: Jeremy Whitlock <jwhitlock@apache.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> 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

Fishrock123 mentioned this pull request Jun 30, 2015

streaming / iterative fs.readdir nodejs/node#583

Closed

misterdjules mentioned this pull request Jun 30, 2015

fs: uv_fs_{open,read,close}_dir #175

Closed

saghul added the pending-review label Jul 10, 2015

domenic mentioned this pull request Jul 16, 2015

Observable is not appropriate for directory listing; async iterable would be better w3c/filesystem-api#4

Open

whitlockjc force-pushed the pr-175-to-master branch from 10b03ef to 99ef9fe Compare August 19, 2015 16:22

saghul mentioned this pull request Sep 25, 2015

Include opendir jprichardson/node-fs-extra#179

Closed

bnoordhuis mentioned this pull request Oct 7, 2015

fs.readdir and fs.readdirSync file order is always sorted on Linux (but not Windows) nodejs/node#3232

Closed

piranna mentioned this pull request Nov 19, 2015

[fix] futimes only works for owned files with write access gulpjs/vinyl-fs#128

Closed

saghul added the v2 label Jan 25, 2016

clee mentioned this pull request May 4, 2016

fs.readdir takes too damn long clee/node-maildir#2

Closed

eljefedelrodeodeljefe mentioned this pull request May 11, 2016

discuss: have glob function is fs or path API nodejs/node#6677

Closed

eljefedelrodeodeljefe reviewed May 11, 2016
View reviewed changes

bnoordhuis reviewed Apr 30, 2018

View reviewed changes

vtjnash mentioned this pull request Jun 6, 2018

RFC: Change 'readdir' to return an iterator. JuliaLang/julia#27450

Closed

silverwind mentioned this pull request Aug 1, 2018

fs: readdir optionally returning type information nodejs/node#22020

Closed

4 tasks

cjihrig mentioned this pull request Oct 29, 2018

unix,win: add uv_fs_{open,read,close}_dir() #2057

Merged

cjihrig closed this Oct 29, 2018

achingbrain mentioned this pull request Dec 2, 2018

OOM when importing large directories ipfs/js-ipfs#1736

Closed

fs: uv_fs_{open,read,close}_dir #416

fs: uv_fs_{open,read,close}_dir #416

Conversation

whitlockjc commented Jun 30, 2015 • edited Loading

whitlockjc commented Jul 1, 2015

Fishrock123 commented Aug 10, 2015

saghul commented Aug 11, 2015

whitlockjc commented Aug 19, 2015

rsms commented Aug 24, 2015

eljefedelrodeodeljefe commented May 11, 2016

whitlockjc commented May 11, 2016

eljefedelrodeodeljefe commented May 11, 2016

txdv commented May 11, 2016

eljefedelrodeodeljefe commented May 11, 2016

whitlockjc commented May 11, 2016 • edited Loading

whitlockjc commented May 11, 2016 • edited Loading

eljefedelrodeodeljefe commented May 11, 2016

whitlockjc commented May 11, 2016

saghul commented May 11, 2016

eljefedelrodeodeljefe May 11, 2016

Choose a reason for hiding this comment

whitlockjc May 11, 2016

Choose a reason for hiding this comment

saghul May 11, 2016

Choose a reason for hiding this comment

addaleax Apr 15, 2018

Choose a reason for hiding this comment

whitlockjc commented May 11, 2016 • edited Loading

eljefedelrodeodeljefe commented May 11, 2016

whitlockjc commented May 11, 2016

guymguym commented Apr 16, 2018

whitlockjc commented Apr 16, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Apr 30, 2018

Choose a reason for hiding this comment

cjihrig commented May 26, 2018

whitlockjc commented May 29, 2018

rijnhard commented Jun 19, 2018

whitlockjc commented Jun 19, 2018

rijnhard commented Jul 10, 2018

whitlockjc commented Jul 19, 2018

whitlockjc commented Jul 30, 2018

cjihrig commented Aug 2, 2018

guymguym commented Aug 2, 2018

Fishrock123 commented Sep 21, 2018

rijnhard commented Oct 12, 2018

cjihrig commented Oct 12, 2018

cjihrig commented Oct 29, 2018

whitlockjc commented Jun 30, 2015 •

edited

Loading

whitlockjc commented May 11, 2016 •

edited

Loading

whitlockjc commented May 11, 2016 •

edited

Loading

whitlockjc commented May 11, 2016 •

edited

Loading