fs: Add DuplexStream and fs.createDuplexStream

[benjamincburns]

This adds a DuplexStream and matching createDuplexStream function to the fs module. Similar to how Duplex uses parasitic inheritance from the Writable class, I've implemented this by inheriting prototypically from Duplex and parasitically from ReadStream and WriteStream.

Please note that I had to modify slightly the ReadStream class to make this work. The only modification necessary was to prefix the end member (undocumented and used in a private context), so it is now _end. This prevents this field from clashing with the end method specified by the Duplex stream.

[benjamincburns]

This work was discussed briefly in issue #7707

[vkurchatkin]

@benjamincburns how is this useful?

[benjamincburns]

Makes it possible to do full duplex I/O on a character device, such as in http://github.com/benjamincburns/node-tuntap.

[benjamincburns]

Also see this StackOverflow question: http://stackoverflow.com/q/24464709/203705

[trevnorris]

@indutny / @tjfontaine Thoughts on this? I can do the review and merge, but want feedback on allowing the new feature?

[benjamincburns]

FWIW, I'd be happy to extend this PR to also include autoClose behaviour on WriteStream, so that the three stream types (including DuplexStream) behave consistently.

[indutny]

I think it is nice addition to the fs APIs, though, probably not really useful for many people. But if we will ensure that the implementation is minimal and relies on existing things - there shouldn't be any additional support burden for us.

[saghul]

Just a comment about the general idea behind this, as it has been mentioned that it would be useful for talking to character devices:

That is an API abuse. It relies on the fact that uv_fs_open is pretty liberal because "everything is a fd" on Unix. What would happen if we check if the fd belongs to an actual file instead? Hell would break loose. Windows does this, for example, and I consider it A Good Thing (TM).

We need to do this properly in libuv and have some uv_???_stream handle which is an actual uv_stream_t.

My 2 cents.

[benjamincburns]

@saghul I'm not sure I understand your objection. One of the core tenants of unix is that devices (character or otherwise) are actual files in the VFS. I'd agree that this would be API abuse if we were talking about a socket fd, but that's not the case here.

However I don't disagree that there might be a way to express the general purpose character device use case in terms of more appropriate multiplatform semantics. If you can propose something more specific I'd be happy to submit another PR if it's all well and good.

In the mean time this PR is a logical extension of the semantics which are already in place. We've got fs.ReadStream which extends Readable and takes an fd, fs.WriteStream which extends Writable and takes an fd, so assuming those have both been given appropriate consideration it's hard to say that fs.DuplexStream shouldn't exist.

[saghul]

(disclaimer: my involvement with Node is quite limited, I'm on libuv core, so look at my comments from that angle)

@saghul I'm not sure I understand your objection. One of the core tenants of unix is that devices (character or otherwise) are actual files in the VFS. I'd agree that this would be API abuse if we were talking about a socket fd, but that's not the case here.

While that is true, libuv is an abstraction layer. uv_fs_t is an abstraction over regular files, as I view it, so not all Unix "features" or philosophy apply, IMHO.

However I don't disagree that there might be a way to express the general purpose character device use case in terms of more appropriate multiplatform semantics. If you can propose something more specific I'd be happy to submit another PR if it's all well and good.

I haven't sat down to think about this, and unfortunately I don't have the time to do so. I believe libuv should expose that, so if you want to give it a shot, design the feature and open an issue so we can discuss it before jumping into writing code.

In the mean time this PR is a logical extension of the semantics which are already in place. We've got fs.ReadStream which extends Readable and takes an fd, fs.WriteStream which extends Writable and takes an fd, so assuming those have both been given appropriate consideration it's hard to say that fs.DuplexStream shouldn't exist.

Ultimately it's not my decision, I'm just John Doe complaining :-) I see this as increasing the techincal debt in fs streams, which I believe to be wrong in the first place.

The way I see it, 2 things are needed:

• a proper uv_stream_t for dealing with plain files, which Node fs streams would sit on top
• something to deal with those character devices

As you can see, I think fo them as separate things.

Those are my 2 cents.

[benjamincburns]

No worries - I think this debate is constructive. I'm new to the node community, but from reading other PRs and issues it seems like the node community/maintainers haven't really settled on how to handle files and file descriptors in a broad sense. I think the most common use cases are nailed down well (stat file, open file, read file, write file, close file; and again but s/file/socket/g), but there are a lot of corner cases which aren't accounted for. Again, as someone new to the community, it appears that these issues are preventing the js runtime from being as useful as other more mature high level language runtimes, such as python.

I think my knowledge of libuv internals puts me at a disadvantage here.

From the perspective of someone who writes a fair amount of code for Linux devices, I expect to be able to treat devices as though they are regular files, because they are regular files. All of the operations and caveats of regular file access should apply. I can't think of any other language where I wouldn't accomplish my goal by opening the /dev/net/tun file, doing some ioctl calls on its fd, and reading from/writing to it as needed. To be blunt, if the answer to this becomes "write a native module/extension," I think the node runtime has simply failed.

Put differently, I very much agree with the goal that the runtime should be as platform agnostic as possible, but it should not prevent me from writing platform-specific modules.

That said, I really like your idea. fs.ReadStream and fs.WriteStream (and again, by extension fs.DuplexStream) mapping to a uv_stream_t under the hood sounds all kinds of sane to me. I'd still want to be able to grab their file descriptors, however.

If you'd like to chat more out of band about the higher level semantics, feel free to contact me on Skype at benjamin.burns, or on Google hangouts at benjamin.c.burns.

[trevnorris]

@saghul fs.write() does seem to be abuse-able. For example:

> fs.write(1, new Buffer('hi\n'), 0, 3);
hi

But if you add any value > 0 for position then you get Error: ESPIPE, write.

[saghul]

@benjamincburns I understand your point. The porblem is that while libuv tries to abstract many things, some have "leaked". I consider the fs API to be one of them (on Unix).

I don't have a proposal for a change right now, but after the next libuv stable version is released we'll start working on some new design ideas. It will take time, but hopefully we can find a good and future proof approach.

FWIW, the current model is not going away in 0.12, so if it helps others maybe it's a good idea to merge this. We can replace the guts later while maintaining the API, (hopefully) making everyone happy :-)

[saghul]

@trevnorris ouch, it seems worse than I thought.

[benjamincburns]

but after the next libuv stable version is released we'll start working on some new design ideas.

Cheers. I'll definitely keep an eye out for this! I think there is room for all sorts of very interesting and nice clean abstraction here, and I hope that whatever the solution becomes it doesn't prevent me from "reaching down a layer" when I'd like to do so.

[jasnell]

Given that there's been no further motion on this and it looks to introduce a significant new feature, I'm inclined to close this here and ask that if it's still something that is wanted, please update and open a new PR against master on http://github.com/nodejs/node. /cc @joyent/node-tsc

[benjamincburns]

@jasnell - yes, this is something that is still wanted. I'll do as suggested.