open event doesn't fire on fs.createWriteStream before finish event is fired

[phated]

All the tests that have streaming content have a setTimeout(..., 100) to account for this but that seems wrong. We need to figure out why this is happening.

@erikkemperman would you want to dig into this if you have some time? I've been looking at https://github.com/nodejs/node/blob/v0.10.46/lib/fs.js#L1625 but I haven't found anything yet.

[erikkemperman]

Is this happening for 0.10 specifically?

Interestingly I bound to "open" to get the fd because I read in the docs that 0.10 doesn't support an fd option, like later versions do mention explicitly.

But it looks like it actually does:
https://github.com/nodejs/node/blob/v0.10.46/lib/fs.js#L1636

If that is true, perhaps we don't need to listen for "open" at all but can just use fs.open and pass the resulting fd into createWriteStream.

[erikkemperman]

Looks like it was added back in 2012.
nodejs/node@44b308b
Not sure if it is safe to expect this to work consistently on all 0.10 versions, but if so just opening our own fd and passing it into createWriteStream would solve this problem I think.

[phated]

It's happening on every version, I just had my browser open on that tag. I think the issue has to be something vinyl-fs does with the readable stream but I'm not exactly sure.

[erikkemperman]

Right. I will have some time this weekend and will look into this (and the mkdirp/chmod thing as well).

[erikkemperman]

@phated
Found a little time to play with this today -- mildly interesting findings.

Even though the documentation for 0.10 doesn't mention supporting an fd option to createWriteStream, like later versions do:
https://nodejs.org/docs/latest-v0.10.x/api/fs.html#fs_fs_createwritestream_path_options
https://nodejs.org/docs/latest-v0.12.x/api/fs.html#fs_fs_createwritestream_path_options

... the commit I linked earlier actually is a part of the 0.10.0 release:
https://github.com/nodejs/node/blob/v0.10.0/lib/fs.js#L1608

So, seems like the 0.10 docs are incomplete in this regard.

I tried refactoring the write-stream module accordingly, pre-empting the need to listen for the "open" event to obtain the fd (actually "open" is not emitted by createWriteStream in this case, which makes sense):
erikkemperman@034699d

This passes Travis just fine, including 0.10:
https://travis-ci.org/erikkemperman/vinyl-fs/builds/143137217

So far, so good. Unfortunately I can't remove the setTimeout workarounds you mentioned, that generates lots of errors. Since "open" is now not emitted I am not sure the description of this issue is entirely accurate...? Or maybe I just misunderstood.

No time to look into this further just now, but thought I'd share what I found thus far.

[phated]

Thinking about this overnight, I realized (and just confirmed) that the problem is https://github.com/gulpjs/vinyl-fs/blob/master/lib/dest/write-contents/write-stream.js#L33-L35 which is being called before anything is written and calling complete before open is even emitted for the write stream.

[phated]

The solution seems to be to implement our own FileWriteStream as per https://twitter.com/mafintosh/status/751186046293991424 using https://github.com/mafintosh/flush-write-stream. I'm not really sure how to do this.

[phated]

@erikkemperman I think I have something working at https://github.com/gulpjs/vinyl-fs/tree/custom-write-stream - the custom stream implementation will need cleanup, tests and lots of other stuff (I added many TODOs). The test that I was refactoring when I encountered the timeouts is committed on a separate commit in that branch.

[erikkemperman]

So.. Nothing to do with what I found :-)

Looks like you've got this figured out!

To think all of these complications are all still due to having to operate on file descriptors, which was only needed because of that frankly terrible quirk with sub-second precision of utimes vs futimes...

I will look into the mkdirp/chmod thing tomorrow, and I noticed a minor oddity I might try to address (prepare-write is attaching a flag property to the vinyl which seems to me ought to be internal).

[phated]

Having trouble with the file descriptors not being closed on Windows. Still digging into that but everything else looks good for this.

[phated]

Tests passing across platforms. Pushed to master!

[erikkemperman]

Nice!