http2: support generic `Duplex` streams #16269

addaleax · 2017-10-17T22:54:10Z

Commits:

src: allow top-level calls into JSStream
Allow JSStream instances to be used more flexibly by explicitly
enabling calls that have no JS stack below them.
src: turn JS stream into a full duplex
Remove unused methods for reading data from JSStream and add
those required for emitting data or an EOF event to the JS side,
in essentially the same way that LibuvStreamWrap does it.
http2: support generic Duplex streams
Support generic Duplex streams through using StreamWrap
on the server and client sides, and adding a createConnection
method option similar to what the HTTP/1 API provides.

Since HTTP2 is, as a protocol, independent of its underlying transport
layer, Node.js should not enforce any restrictions on what streams
its internals may use.

Ref: #16256

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http2

addaleax · 2017-10-17T23:01:56Z

CI: https://ci.nodejs.org/job/node-test-commit/13238/

/cc @nodejs/http2

jasnell · 2017-10-17T23:33:21Z

lib/internal/http2/core.js

  constructor(type, options, socket) {
    super();

+    if (!(socket instanceof net.Socket)) {


hmm... I'd almost prefer that this would check to see if the socket has an ._external ... that is, rather than checking if it is a net.Socket, that it would check if it is a StreamBase somehow. Yes, generally it should always be socket but...

Also, I've purposefully avoided using instanceof in the constructor here to avoid the performance hit. It would be good if we could find a way of avoiding adding it here.

Fwiw, for tls we also check instanceof net.Socket, although I think I generally agree

I’m going to sleep soon but feel free to push to this branch :)

I'm working on a few other items so cannot break away to push on this branch.

One thing to keep in mind is that if this mechanism is only going to be used for the client side, the instanceof check can be moved into the ClientHttp2Session constructor rather than the base Http2Session in order to keep the performance hit isolated just to the client side of things.

Actually... scratch that... I see you're using this for server side also... hmmm let me stew on it a bit.

Okay, I’ve switched to checking for the presence of socket._handle._externalStream

jasnell · 2017-10-17T23:37:32Z

lib/internal/http2/core.js

-    default:
-      throw new errors.Error('ERR_HTTP2_UNSUPPORTED_PROTOCOL', protocol);
+  if (typeof options.createConnection === 'function') {
+    socket = options.createConnection(authority, options);


should likely type check the return value

I think all we could do here is doing some duck-type checking, e.g. verifying that .write() and .on() are present…

Do we really need to? This is very much opt-in... What does http do for createConnection?

It doesn’t do any explicit typechecking either, yes.

Ok. I won't block on that. I much prefer the APIs to be stricter here and not gate based on what we do in http1 but I can live with this.

jasnell · 2017-10-17T23:52:07Z

lib/internal/http2/core.js

@@ -724,7 +729,8 @@ class Http2Session extends EventEmitter {
    this[kSocket] = socket;

    // Do not use nagle's algorithm
-    socket.setNoDelay();
+    if (typeof socket.setNoDelay === 'function')
+      socket.setNoDelay();

    // Disable TLS renegotiation on the socket


A bit further down in this code there is a check for socket.connecting that explicitly defers actions until the socket is connected. With this change, the assumption would be that the non-socket Stream passed in is immediately available for use, which is a perfectly fine assumption to make, IMHO, but I want to make sure that is actually a safe assumption here.

I’m not sure what the alternative would be. We could document socket.connecting/socket.on('connect') as part of the implicit API here?

That would certainly be an option, but given that we're wrapping the custom socket, the likelihood of it even being an issue is pretty low. Let's go with this for now :-)

jasnell · 2017-10-17T23:56:23Z

test/parallel/test-http2-generic-streams.js

+    clientSide.end();
+  }));
+  req.end();
+}


It would be good to include a limited selection of additional variations of other http2 tests, especially those that deal with multiplexing and flow-control just to make sure there are no hidden gotchas. It should be fine, but over-abundance of caution and all that.

I don’t have that much of an overview over the HTTP/2 tests, it would be good if you could give pointers as to which test files I should be looking at.

(But yes, this should be fine anyway – this doesn’t really deal with anything below the

We can do this separately, actually. Don't worry about it in this PR

mcollina

Good work. Does this affects benchmarks anyhow?
Can you add a unit test for sendFile as well?

addaleax · 2017-10-18T09:20:08Z

Does this affects benchmarks anyhow?

I will run them but I’d go with “no” – there really shouldn’t be any measurable perf impact if you don’t use this feature (and even if you do, it’s only during session construction).

Can you add a unit test for sendFile as well?

That sounds reasonable, yes. Done!

apapirovski

LGTM 👍 Just very minor nits.

apapirovski · 2017-10-20T13:37:04Z

lib/internal/http2/core.js

  constructor(type, options, socket) {
    super();

+    if (!socket._handle || !socket._handle._externalStream) {


I know this is super nit picky but could we compare to undefined? Anytime we're dealing with an object, because of the existence of document.all (at least as far as V8 is concerned), these truthy/falsey checks are unusually expensive.

Some things in Node’s source explicitly set ._handle on sockets to null (e.g. after disconnecting)… so I’d like to be consistent with TLS here, just to be on the safe side.

these truthy/falsey checks are unusually expensive.

I doubt this makes a noticeable difference compared to the overall cost of setting up the HTTP2 session objects anyway, V8 seems to optimize a function containing only this line reasonable well to me.

For some reason I thought we were unsetting _handle but if we do null it then this is fine.

apapirovski · 2017-10-20T13:39:52Z

test/parallel/test-http2-generic-streams-sendfile.js

+const MakeDuplexPair = require('../common/duplexpair');
+
+{
+  const testData = '<h1>Hello World</h1>';


Nit: I think this is unused in this test?

Huh. Yup, done!

apapirovski · 2017-10-20T13:41:10Z

test/parallel/test-http2-generic-streams-sendfile.js

+
+  req.setEncoding('utf8');
+  let data = '';
+  req.on('data', (chunk) => {


Nit: ideally a mustCall?

This is just accumulating the data, and that’s checked in an assert (inside a mustCall) anyway

mustCall's on data events can be problematic. If anything, a mustCallAtLeast would be ok.

Sorry, this was strictly about the consistency of these calls. In other tests with this PR, data events have mustCall. It sets a good example for newer contributors when there's consistency to where we mustCall and where we don't. Or we could leave a comment explaining why not.

When I started writing tests and contributing to node I had a bit of confusion around this because it wasn't 100% consistent across tests.

But as I mentioned, this is just a very minor nit. :)

Fwiw, the other call is somewhat intentionally checking that the data is sent through in a single chunk :) I’ve added 2a666de1940b as a comment for that

apapirovski · 2017-10-20T13:45:01Z

test/common/README.md

@@ -8,6 +8,7 @@ This directory contains modules used to test the Node.js implementation.
 * [Common module API](#common-module-api)
 * [Countdown module](#countdown-module)
 * [DNS module](#dns-module)
+* [Duplex pair helper](#duplex-pair-module)


Nit: should this be Duplex pair Module to match heading below? Edit: Or I guess, to nit pick myself, Duplex pair module to match the rest of the list...

If you want this changed for consistency, I’d go with Duplex pair helper everywhere. Does that seem okay?

That seems good 👍

addaleax · 2017-10-22T14:38:26Z

Fresh CI: https://ci.nodejs.org/job/node-test-commit/13370/

This should be ready.

Allow `JSStream` instances to be used more flexibly by explicitly enabling calls that have no JS stack below them. PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unused methods for reading data from `JSStream` and add those required for emitting data or an EOF event to the JS side, in essentially the same way that `LibuvStreamWrap` does it. PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: nodejs#16256 PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-10-23T15:11:08Z

Landed in ba4a0a6...ab16eec

Allow `JSStream` instances to be used more flexibly by explicitly enabling calls that have no JS stack below them. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unused methods for reading data from `JSStream` and add those required for emitting data or an EOF event to the JS side, in essentially the same way that `LibuvStreamWrap` does it. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: #16256 PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Allow `JSStream` instances to be used more flexibly by explicitly enabling calls that have no JS stack below them. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unused methods for reading data from `JSStream` and add those required for emitting data or an EOF event to the JS side, in essentially the same way that `LibuvStreamWrap` does it. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: #16256 PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * crypto: - expose ECDH class #8188 * http2: - http2 is now exposed by defualt without the need for a flag #15685 - a new environment varible NODE\_NO\_HTTP2 has been added to allow userland http2 to be required #15685 - support has been added for generic `Duplex` streams #16269 * module: - resolve and instantiate loader pipeline hooks have been added to the ESM lifecycle #15445 * zlib: - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an error to be raised when a raw deflate stream is initialized with windowBits set to 8. On some versions this crashes Node and you cannot recover from it, while on some versions it throws an exception. Node.js will now gracefully set windowBits to 9 replicating the legacy behavior to avoid a DOS vector. https://github.com/nodejs-private/node-private/pull/95 PR-URL: https://github.com/nodejs-private/node-private/pull/98

Allow `JSStream` instances to be used more flexibly by explicitly enabling calls that have no JS stack below them. PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unused methods for reading data from `JSStream` and add those required for emitting data or an EOF event to the JS side, in essentially the same way that `LibuvStreamWrap` does it. PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: nodejs/node#16256 PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * crypto: - expose ECDH class nodejs/node#8188 * http2: - http2 is now exposed by defualt without the need for a flag nodejs/node#15685 - a new environment varible NODE\_NO\_HTTP2 has been added to allow userland http2 to be required nodejs/node#15685 - support has been added for generic `Duplex` streams nodejs/node#16269 * module: - resolve and instantiate loader pipeline hooks have been added to the ESM lifecycle nodejs/node#15445 * zlib: - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an error to be raised when a raw deflate stream is initialized with windowBits set to 8. On some versions this crashes Node and you cannot recover from it, while on some versions it throws an exception. Node.js will now gracefully set windowBits to 9 replicating the legacy behavior to avoid a DOS vector. nodejs-private/node-private#95 PR-URL: nodejs-private/node-private#98

Allow `JSStream` instances to be used more flexibly by explicitly enabling calls that have no JS stack below them. PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Remove unused methods for reading data from `JSStream` and add those required for emitting data or an EOF event to the JS side, in essentially the same way that `LibuvStreamWrap` does it. PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: nodejs/node#16256 PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * crypto: - expose ECDH class nodejs/node#8188 * http2: - http2 is now exposed by defualt without the need for a flag nodejs/node#15685 - a new environment varible NODE\_NO\_HTTP2 has been added to allow userland http2 to be required nodejs/node#15685 - support has been added for generic `Duplex` streams nodejs/node#16269 * module: - resolve and instantiate loader pipeline hooks have been added to the ESM lifecycle nodejs/node#15445 * zlib: - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an error to be raised when a raw deflate stream is initialized with windowBits set to 8. On some versions this crashes Node and you cannot recover from it, while on some versions it throws an exception. Node.js will now gracefully set windowBits to 9 replicating the legacy behavior to avoid a DOS vector. nodejs-private/node-private#95 PR-URL: nodejs-private/node-private#98

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added http2 Issues or PRs related to the http2 subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 17, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 17, 2017

addaleax mentioned this pull request Oct 17, 2017

We "need" a way to create an Http2Session from a socket #16256

Closed

addaleax force-pushed the http2-generic-streams branch from a0b1a2e to fe69dca Compare October 17, 2017 23:14

jasnell reviewed Oct 17, 2017

View reviewed changes

jasnell requested a review from mcollina October 17, 2017 23:43

jasnell reviewed Oct 17, 2017

View reviewed changes

mcollina reviewed Oct 18, 2017

View reviewed changes

apapirovski approved these changes Oct 20, 2017

View reviewed changes

jasnell approved these changes Oct 20, 2017

View reviewed changes

addaleax force-pushed the http2-generic-streams branch from 5bb1fa5 to 621d2d7 Compare October 20, 2017 18:58

addaleax added 4 commits October 22, 2017 17:05

src: allow top-level calls into JSStream

0b947ab

Allow `JSStream` instances to be used more flexibly by explicitly enabling calls that have no JS stack below them. PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test: add makeDuplexPair() helper

ac1b475

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax force-pushed the http2-generic-streams branch from 2a666de to 966e621 Compare October 22, 2017 15:07

addaleax closed this Oct 23, 2017

addaleax deleted the http2-generic-streams branch October 23, 2017 15:11

addaleax added a commit that referenced this pull request Oct 23, 2017

test: add makeDuplexPair() helper

e340a66

Add a utility for adding simple, streams-API based duplex pairs. PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added dont-land-on-v6.x labels Jan 15, 2018

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

This pull request was closed.

http2: support generic Duplex streams #16269

http2: support generic Duplex streams #16269

Conversation

addaleax commented Oct 17, 2017

Checklist

Affected core subsystem(s)

addaleax commented Oct 17, 2017 • edited Loading

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

jasnell Oct 17, 2017 • edited Loading

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

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

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Oct 18, 2017

apapirovski left a 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

apapirovski Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Oct 20, 2017 • edited Loading

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

addaleax commented Oct 22, 2017

addaleax commented Oct 23, 2017

http2: support generic `Duplex` streams #16269

http2: support generic `Duplex` streams #16269

addaleax commented Oct 17, 2017 •

edited

Loading

jasnell Oct 17, 2017 •

edited

Loading

apapirovski Oct 20, 2017 •

edited

Loading

apapirovski Oct 20, 2017 •

edited

Loading