Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

streams: introduce StreamWrap and JSStream #926

Closed
wants to merge 6 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 23, 2015

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()`.
var raw = net.connect(common.PORT);

var pending = false;
raw.on('readable', function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@indutny What about it bothers you?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

});
}
util.inherits(StreamWrap, Socket);
exports.StreamWrap = StreamWrap;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good idea.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@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
Copy link
Member Author

indutny commented Feb 24, 2015

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

@chrisdickinson
Copy link
Contributor

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?

@indutny indutny mentioned this pull request Feb 24, 2015
self.stream.write(buf, done);
});

function done() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to take error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@indutny indutny added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 24, 2015
StreamWrap.prototype.shutdown = function shutdown(req) {
var self = this;

this.stream.end(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can get an error here, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we can't :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I am mistaken!


bufs.forEach(function(buf) {
self.stream.write(buf, done);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@chrisdickinson
Copy link
Contributor

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
Copy link
Contributor

LGTM.

indutny added a commit that referenced this pull request Feb 24, 2015
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
Copy link
Member Author

indutny commented Feb 24, 2015

Landed in 1738c77, thank you!

@indutny indutny closed this Feb 24, 2015
@indutny indutny deleted the feature/js-stream branch February 24, 2015 19:39
@rvagg rvagg mentioned this pull request Feb 24, 2015
petkaantonov pushed a commit to petkaantonov/io.js that referenced this pull request Feb 25, 2015
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
Copy link
Member

This feature is amazing :O

@rvagg rvagg mentioned this pull request Mar 2, 2015
rvagg added a commit that referenced this pull request Mar 3, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants