Decouple opening and closing of file descriptors from src and dest

[phated]

This could be done in a separate utility that might close descriptors lazily (having it ready in case a subsequent pipeline wants it).

Ref #151 (comment)

[erikkemperman]

@phated I've been looking into this a bit, thought I'd share:

• Implementing this will require considerable refactoring, especially of src which currently doesn't do FDs, at all;
• It will be tricky to get it right, I think, especially in combination with external tools or internal plugins interacting with the filesystem outside of vinyl-fs' control;
• I'm curious if this issue ought to be in the 3.0 milestone, the way I see this it would be a transparent change to end users (so long as it works, natch);
• Wondering if there will actually be a noticeable performance boost here, in common scenarios, that are not already benefiting from similar tricks in lower-level layers (caches).

I'm now thinking probably the last of these would be best to look at first, if that turns up a negative then I'd say we're done here. I'll not have a lot of time but would like to set up a simple experiment around this when I have the chance.

Thoughts?

[erikkemperman]

BTW, in working up courage to attack this issue, I did some cosmetic stuff, for instance to make src and dest more (anti)symmetric in terms of things like module names. Might as well link to it here, if there's anything about it you like I can make a PR with a subset:
commits
travis passes

[erikkemperman]

Forgot to mention: one little annoyance here is that in Node 0.10 we can't pre-open an FD if we want to use it to make readable/writable streams. That was only introduced in 0.12.

For some reason I have the impression that bumping minimum require node version is not really an option, have I got that right? If so, that'd mean an extra little complication for this issue.

[phated]

• I'm excited about the refactoring, it is something I've wanted to happen since the .dest() stuff was being worked on.
• I totally understand that we might not want to hold them open due to people interacting (against our guidelines) with the filesystem.
• The reason I wanted this in the 3.0 milestone was just out of safety. I'm scared of any sweeping changes to land in 2.x after what happened with utimes
• I actually have no idea if there will be a performance boost (and I don't even know if there was a performance decrease when utimes was added). I'm interested in vinyl-fs being as fast as possible but I don't see it causing problems right now.

I'm really liking your work and interested in much of the refactor (commented on the commits). Would you want to send it over in some PRs? Does it make sense to be a single refactor PR or multiple smaller ones?

I definitely want to keep supporting node 0.10 through the gulp 4 release. We can consider dropping it in future releases but so many people still use it today.

[erikkemperman]

Thanks for the quick reply!

Actually my feeling was the extra metadata operations in dest should not impact performance that much because we're re-using the same fd... But I guess that's from ancient C intuition and might not really apply here -- or even in C anymore with modern hardware and file systems.

I am aware of the guideline against plugins interacting with the fs directly, but my impression is lots of commonly used plugins do this, if only because they're just wrappers around other libs, like uglify or sass. Also, there's no vfs api to unlink files, right?

So while there still might be an argument for having a single utility for opening and closing fd's, and then use that also from src, my original goal in doing that (lazy closing) may cause some issues while not having much benefits other than cosmetic?

Glad you like some of those commits, I'll address your comments later today and make a PR. A single PR would be most convenient I think, not sure if I should squash the commits (maybe easier for others to rebase if I don't?).

[phated]

We do merge + squash now so they'd all turn into a single commit. I agree that the lazy closing will probably cause more problems than it solves.

[erikkemperman]

All right, so would that mean you'd prefer multiple PRs? Or won't people mind (I'm thinking authors of currently open PRs) a single commit which bunches together some more or less unrelated changes?

[phated]

Probably send them as multiple PRs. I think that is better for documenting the changes.

[erikkemperman]

Sure thing, I'll split the commits over several branches and make PRs for each of those.

[erikkemperman]

@phated I've made five PRs, there's a single commit in my aesthetix branch ("Consistent callback names") which I figured is probably better to submit after some or all of these are merged, because conflicts.

[phated]

Closing this because we determined that lazy closing descriptors is a bad idea.