Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

fs: uv_fs_{open,read}_dir for unix #1521

Closed
wants to merge 1 commit into from

Conversation

misterdjules
Copy link

Tested on Linux, MacOS X, SmartOS and Windows.

Fixes #1430

@misterdjules
Copy link
Author

This is a work in progress for implementing @tjfontaine's proposal that solves #1430. I chose @tjfontaine's approach because, to me, conceptually as well as in terms of Unix abstractions, a directory is a stream of data that we hold on to from the moment it is open until we close it. However, I am of course open to suggestions and other point of views.

Currently, it does not work on Windows. I'd like to get your general feedback on this initial implementation before moving on to implement it on Windows.

There are also some important implementation issues with this PR that I have already planned to fix (handling of errors in some places, etc.) and that I will push in the next few hours. I'm more interested in some general feedback early on to determine if this PR is going into the right direction for everyone.

Thank you!

@saghul
Copy link
Contributor

saghul commented Oct 9, 2014

@misterdjules Thanks for working on this! I won't have time to thoroughly review this for some days (swamped right now) but here is a general comment on the approach.

uv_fs_* are requests, making uv_dir_t a handle makes the API inconsistent. I outlined how I envision the design here: #1430 (comment) after the conversations we had.

#if defined(PATH_MAX)
len = PATH_MAX;
#else
len = 4096;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what Unix platforms wouldn't define PATH_MAX, but 4096 seems a bit large. BSD, for example, can only hold 1024.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this code was already present in earlier versions, I just refactored it a little so that we can compute the maximum directory entry length from a file descriptor or a path.

@bnoordhuis From the repo's history, it seems that this commit introduced this code. Do you remember the rationale behind these tests and the default value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually about a year older, I added it in commit d3f60da. :-)

It's been a while but I think I may have added it as a workaround for a Cygwin header bug (which we still supported at that time.) 4096 is the PATH_MAX on Linux, by the way.

@misterdjules
Copy link
Author

@splinterofchaos Thanks for a pre-review of this work in progress!

@misterdjules
Copy link
Author

@saghul Thank you for taking a look at it!

I agree that this implementation is inconsistent with the rest of the file system API.

However, the directory that we stream data from is composed of the following resources:

  • The directory stream (DIR* or HANDLE depending on the platform).
  • The directory entries.

These resources need to be allocated when the streaming starts, and deallocated when it ends. It seemed to me that handles were a good abstraction that fit these needs without having to manually handle these resources separately.

Also, having a directory stream handle active keeps the loop active, which to me seems like it could give an incentive for developers to manage these resources more carefully.

Maybe such a directory streaming feature could be implemented outside of the FS API as a separate handle type, in a way similar to uv_fs_poll*?

I'm new to libuv and still learning a lot, so this is essentially me challenging the original proposition to get a better understanding of the big picture. I'm going to work on finishing the Windows implementation, and at that point it should be easy to switch to your original proposal if you still want to go this way.

Thank you very much again!

@misterdjules misterdjules force-pushed the fix-issue-1430 branch 4 times, most recently from 18cb045 to 75a5f92 Compare October 14, 2014 18:27
Tested on Linux, MacOS X, SmartOS and Windows.

Fixes joyent#1430
@misterdjules
Copy link
Author

@saghul The windows implementation is done. Could you please ping me on #libuv when you have some time to discuss the design? Thank you!

@saghul
Copy link
Contributor

saghul commented Oct 15, 2014

Sorry, I've been quite swamped lately. Just in case we miss each other on IRC, I'm typing this here, FTR.

First, I would have preferred if we have had this discussion before you jumped to the implementation. Now it looks like I need to convince you that the design discussed in #1430 is right, and you'll be biased to your own implementation, because well, you wrote it.

I agree that this implementation is inconsistent with the rest of the file system API.

This should be a good enough reason not to go down this road.

However, the directory that we stream data from is composed of the following resources:

The directory stream (DIR* or HANDLE depending on the platform).
The directory entries.

These resources need to be allocated when the streaming starts, and deallocated when it ends. It seemed to me that handles were a good abstraction that fit these needs without having to manually handle these resources separately.

Also, having a directory stream handle active keeps the loop active, which to me seems like it could give an incentive for developers to manage these resources more carefully.

This is not the case for any other fs request. If you want to keep the loop alive you can chain requests by creating more of them in the callback of the first one. I guess you think this way because of node, but this is libuv, and at this level, the loop exiting is just fine, it can be restarted. Do we need to create a handle for every fs function which takes the result of uv_fs_open as an argument? Surely not.

Handles represent entities which do domething. Timers kick every now and then, async handles can trigger callbacks, tcp handles represent clients and servers... a directory... is a directory.

Maybe such a directory streaming feature could be implemented outside of the FS API as a separate handle type, in a way similar to uv_fs_poll*?

I don't like that idea. The API I proposed in #1430 looks flexible enough to me, as the usre can consume entries at his own pace and then close the resources once done. What don't you like about it?

I'm new to libuv and still learning a lot, so this is essentially me challenging the original proposition to get a better understanding of the big picture. I'm going to work on finishing the Windows implementation, and at that point it should be easy to switch to your original proposal if you still want to go this way.

Please do.

@indutny, any thoughts on this?

@misterdjules
Copy link
Author

@saghul Thank you very much for taking the time to respond. I also want to apologize for not discussing the design before jumping in the implementation. It was clearly not the way to go, and I'm sorry it caused some confusion. In the future, I will make sure to discuss such design questions beforehand.

I agree with your comments, and they help me understand libuv's design a bit better. I will work on implementing the API you proposed, and submit a separate PR when it's done.

@saghul
Copy link
Contributor

saghul commented Oct 15, 2014

@misterdjules Thanks for understanding and taking the time to help make libuv better. ❤️

@misterdjules
Copy link
Author

@saghul I haven't had time to work on it in the last couple weeks, but I didn't forget it ;-) Hopefully I'll be able to work on it this week-end. I'll keep you posted.

@misterdjules
Copy link
Author

This issue has been closed because I overwrote the fix-issue-1430 branch with a new branch, and GitHub doesn't let me reopen this PR with my updated changes. I created #1574 with changes that follow @saghul's recommendation more closely.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants