streams: introduce StreamWrap and JSStream #926

indutny · 2015-02-23T20:12:52Z

Introduce a way to wrap plain-js stream.Duplex streams into C++
StreamBase's child class. With such method at hand it is now possible to
pass stream.Duplex instance as a socket parameter to
tls.connect().

cc @iojs/collaborators @iojs/streams @chrisdickinson

Introduce a way to wrap plain-js `stream.Duplex` streams into C++ StreamBase's child class. With such method at hand it is now possible to pass `stream.Duplex` instance as a `socket` parameter to `tls.connect()`.

indutny · 2015-02-23T20:15:42Z

test/parallel/test-tls-js-stream.js

+  var raw = net.connect(common.PORT);
+
+  var pending = false;
+  raw.on('readable', function() {


This one bothers me a bit, I wonder if it could be improved?

@indutny What about it bothers you?

Whole pending thing. I think there might be a way to do it in a way more grounded in a Streams API

chrisdickinson · 2015-02-23T20:51:16Z

lib/_stream_wrap.js

+  });
+}
+util.inherits(StreamWrap, Socket);
+exports.StreamWrap = StreamWrap;


Tiny nit: may as well module.exports = here.

Yeah, good idea.

chrisdickinson · 2015-02-23T21:00:10Z

lib/_stream_wrap.js

+  this.stream.once('end', function() {
+    self._handle.emitEOF();
+  });
+


What happens when the underlying stream emits an error? Per this diagram I don't think the stream will emit "end" or "finish". Is there a way to recover from an error in this state?

I guess this should be handled by the code that created this stream... it could be propagated to the StreamWrap object, though, and handled in _tls_wrap.js. Does this sound plausible?

I think that sound plausible – my expectation would be that a StreamWrap'd stream would expose the inner stream's errors on the outer stream.

I guess this should be handled by the code that created this stream...

This could be a situation where a user gets a stream from package X, instantiates it and passes it to TLSSocket Y; I think the user assumes responsibility for adding error handlers to TLSSocket Y, while TLSSocket Y would be responsible for shutting down / closing / cleaning up once an error on stream X happens.

But this is not the way it works for piping the streams right now, right?

An error downstream will unpipe and clean up listeners on the upstream stream, but will not shut down or destroy the source stream. This has caused a bit of confusion for stream users.

In addition, the act of nesting streams is distinct from piping streams, though piping can be involved in nesting. We don't nest streams often in core (TLS and arguably http are the only places we do something like this.) In userland usually the wrapping stream assumes responsibility for shutting itself down on inner stream error and exposing those errors to userland. Illustrated:

// user assumes responsibility for streams a, b, and c a = stream() b = stream() c = stream() a.pipe(b).pipe(c).pipe(b).pipe(a) // user assumes responsibility for streams a and b, but not stream c a = stream() c = stream() b = stream(c) a.pipe(b).pipe(a).

piscisaureus · 2015-02-24T14:20:28Z

@indutny

I think this is a good idea in general. One comment would be that in the not too distant future read_start/read_stop will be replaced by an asynchronous read() version. Hopefully this feature doesn't get in the way.

indutny · 2015-02-24T14:22:48Z

@piscisaureus we'll figure it out by then, thanks!

chrisdickinson · 2015-02-24T18:15:09Z

I'm +1 on this addition, this is cool – I want to make sure it all works with the streams state machine. If possible, I'd like to see this applied to http streams as well before making the API publicly available, just to make sure the abstraction is broadly applicable – that isn't a blocker for this PR though. The only blocking comment I have is the error handling behavior.

@piscisaureus Out of curiosity, would the async read be at handle level or JS stream level?

chrisdickinson · 2015-02-24T18:59:29Z

lib/_stream_wrap.js

+    self.stream.write(buf, done);
+  });
+
+  function done() {


We may want to take error here.

Actually, ignore that – if we're listening on error on the inner stream we should be okay.

chrisdickinson · 2015-02-24T19:07:42Z

lib/_stream_wrap.js

+StreamWrap.prototype.shutdown = function shutdown(req) {
+  var self = this;
+
+  this.stream.end(function() {


I think we can get an error here, too.

No, we can't :(

I am mistaken!

chrisdickinson · 2015-02-24T19:26:07Z

lib/_stream_wrap.js

+
+  bufs.forEach(function(buf) {
+    self.stream.write(buf, done);
+  });


Might be useful to sniff for self.stream._writev, and if present, surround the forEach with cork() / uncork()?

Good suggestion.

I think it should be enough to just use .cork()/.uncork() and hope for the best.

chrisdickinson · 2015-02-24T19:29:01Z

A few things:

Out of band errors. These are unlikely to happen on core streams, but are totally allowable for userland streams. Eventually we should handle these here. I would not consider this blocking for introducing this feature internally.
There's a potential cork / uncork optimization. Not a blocker, just something to look into.
Does finishWrite with an error eventually shutdown the stream? This is a bit of a blocker and I'm looking into it now to make sure.

Otherwise LGTM pending the last bullet point.

chrisdickinson · 2015-02-24T19:37:56Z

LGTM.

Introduce a way to wrap plain-js `stream.Duplex` streams into C++ StreamBase's child class. With such method at hand it is now possible to pass `stream.Duplex` instance as a `socket` parameter to `tls.connect()`. PR-URL: #926 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>

indutny · 2015-02-24T19:39:14Z

Landed in 1738c77, thank you!

Introduce a way to wrap plain-js `stream.Duplex` streams into C++ StreamBase's child class. With such method at hand it is now possible to pass `stream.Duplex` instance as a `socket` parameter to `tls.connect()`. PR-URL: nodejs#926 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>

dougwilson · 2015-02-26T21:44:33Z

This feature is amazing :O

@julianduque

Notable changes: * stream: Fixed problems for platforms without `writev()` support, particularly Windows. Changes introduced in 1.4.1, via #926, broke some functionality for these platforms, this has now been addressed. #1008 (Fedor Indutny) * arm: We have the very beginnings of ARMv8 / ARM64 / AARCH64 support. An upgrade to OpenSSL 1.0.2 is one requirement for full support. #1028 (Ben Noordhuis) * Add new collaborator: Julian Duque @julianduque

streams: introduce StreamWrap and JSStream

5e57578

Introduce a way to wrap plain-js `stream.Duplex` streams into C++ StreamBase's child class. With such method at hand it is now possible to pass `stream.Duplex` instance as a `socket` parameter to `tls.connect()`.

indutny reviewed Feb 23, 2015
View reviewed changes

fix

d9711b1

chrisdickinson reviewed Feb 23, 2015
View reviewed changes

fixes

866e957

chrisdickinson reviewed Feb 23, 2015
View reviewed changes

indutny mentioned this pull request Feb 24, 2015

Release proposal: v1.4.0 #932

Closed

chrisdickinson reviewed Feb 24, 2015
View reviewed changes

indutny added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 24, 2015

fixes

3310b48

chrisdickinson reviewed Feb 24, 2015
View reviewed changes

fix

b9c004b

chrisdickinson reviewed Feb 24, 2015
View reviewed changes

fix

cef4c46

indutny closed this Feb 24, 2015

indutny deleted the feature/js-stream branch February 24, 2015 19:39

rvagg mentioned this pull request Feb 24, 2015

Release proposal: v1.3.0 #872

Closed

bnb mentioned this pull request Feb 27, 2015

Evangelism: Weekly Update Feb 27th nodejs/evangelism#4

Closed

yosuke-furukawa mentioned this pull request Feb 28, 2015

io.js weekly translation 2015-02-27 nodejs/nodejs-ja#42

Merged

rvagg mentioned this pull request Mar 2, 2015

Release proposal: v1.4.3 #1017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streams: introduce StreamWrap and JSStream #926

streams: introduce StreamWrap and JSStream #926

indutny commented Feb 23, 2015

indutny Feb 23, 2015

ralphtheninja Feb 24, 2015

indutny Feb 24, 2015

chrisdickinson Feb 23, 2015

indutny Feb 23, 2015

chrisdickinson Feb 23, 2015

indutny Feb 23, 2015

chrisdickinson Feb 23, 2015

indutny Feb 24, 2015

chrisdickinson Feb 24, 2015

piscisaureus commented Feb 24, 2015

indutny commented Feb 24, 2015

chrisdickinson commented Feb 24, 2015

chrisdickinson Feb 24, 2015

chrisdickinson Feb 24, 2015

chrisdickinson Feb 24, 2015

indutny Feb 24, 2015

chrisdickinson Feb 24, 2015

chrisdickinson Feb 24, 2015

indutny Feb 24, 2015

indutny Feb 24, 2015

indutny Feb 24, 2015

chrisdickinson commented Feb 24, 2015

chrisdickinson commented Feb 24, 2015

indutny commented Feb 24, 2015

dougwilson commented Feb 26, 2015

streams: introduce StreamWrap and JSStream #926

streams: introduce StreamWrap and JSStream #926

Conversation

indutny commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented Feb 24, 2015

indutny commented Feb 24, 2015

chrisdickinson commented Feb 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdickinson commented Feb 24, 2015

chrisdickinson commented Feb 24, 2015

indutny commented Feb 24, 2015

dougwilson commented Feb 26, 2015