GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http2: support generic `Duplex` streams by addaleax · Pull Request #16269 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Oct 17, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

apapirovski, TimothyGu, and bricss reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@addaleax addaleax added http2 Issues and 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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Oct 17, 2017
@addaleax addaleax mentioned this pull request Oct 17, 2017
We "need" a way to create an Http2Session from a socket #16256
Closed
@addaleax

addaleax commented Oct 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

/cc @nodejs/http2

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax
addaleax force-pushed the http2-generic-streams branch from a0b1a2e to fe69dca Compare October 17, 2017 23:14
jasnell
jasnell reviewed Oct 17, 2017
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@jasnell jasnell Oct 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell reviewed Oct 17, 2017
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@jasnell jasnell Oct 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should likely type check the return value

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@apapirovski apapirovski Oct 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell
jasnell requested a review from mcollina October 17, 2017 23:43
jasnell
jasnell reviewed Oct 17, 2017
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@jasnell jasnell Oct 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell reviewed Oct 17, 2017
View reviewed changes
Comment thread test/parallel/test-http2-generic-streams.js Outdated

@jasnell jasnell Oct 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mcollina
mcollina reviewed Oct 18, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@addaleax

addaleax commented Oct 18, 2017

Copy link
Copy Markdown
Member Author

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

apapirovski
apapirovski approved these changes Oct 20, 2017
View reviewed changes

@apapirovski apapirovski left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM 👍 Just very minor nits.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/http2/core.js Outdated

@apapirovski apapirovski Oct 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@apapirovski apapirovski Oct 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-http2-generic-streams-sendfile.js Outdated

@apapirovski apapirovski Oct 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: I think this is unused in this test?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Huh. Yup, done!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-http2-generic-streams-sendfile.js Outdated

@apapirovski apapirovski Oct 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: ideally a mustCall?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Oct 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@apapirovski apapirovski Oct 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell and apapirovski reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread test/common/README.md Outdated

@apapirovski apapirovski Oct 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@apapirovski apapirovski Oct 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That seems good 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@addaleax addaleax Oct 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ok, done!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell approved these changes Oct 20, 2017
View reviewed changes
@addaleax
addaleax force-pushed the http2-generic-streams branch from 5bb1fa5 to 621d2d7 Compare October 20, 2017 18:58
@addaleax

addaleax commented Oct 22, 2017

Copy link
Copy Markdown
Member Author

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

This should be ready.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added 4 commits October 22, 2017 17:05
@addaleax
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
src: turn JS stream into a full duplex
4c674f7
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
test: add makeDuplexPair() helper
ac1b475
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: nodejs#16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
http2: support generic Duplex streams
966e621
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
addaleax force-pushed the http2-generic-streams branch from 2a666de to 966e621 Compare October 22, 2017 15:07
addaleax added a commit to addaleax/node that referenced this pull request Oct 22, 2017
@addaleax
test: add makeDuplexPair() helper
f54889a
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: nodejs#16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax

addaleax commented Oct 23, 2017

Copy link
Copy Markdown
Member Author

Landed in ba4a0a6...ab16eec

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax closed this Oct 23, 2017
@addaleax
addaleax deleted the http2-generic-streams branch October 23, 2017 15:11
addaleax added a commit that referenced this pull request Oct 23, 2017
@addaleax
src: allow top-level calls into JSStream
127f83a
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Oct 23, 2017
@addaleax
src: turn JS stream into a full duplex
170bc31
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Oct 23, 2017
@addaleax
test: add makeDuplexPair() helper
e340a66
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: #16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Oct 23, 2017
@addaleax
http2: support generic Duplex streams
ab16eec
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@addaleax @MylesBorins
src: allow top-level calls into JSStream
2788cb7
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@addaleax @MylesBorins
src: turn JS stream into a full duplex
d356022
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@addaleax @MylesBorins
test: add makeDuplexPair() helper
80b0dcf
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: #16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@addaleax @MylesBorins
http2: support generic Duplex streams
63036a8
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins added a commit that referenced this pull request Oct 24, 2017
@MylesBorins
2017-10-24, Node.js Version 8.8.0 (Current)
9aa31bc
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
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@addaleax
src: allow top-level calls into JSStream
162da27
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@addaleax
src: turn JS stream into a full duplex
1d3ec4a
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@addaleax
test: add makeDuplexPair() helper
4b1f83a
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: nodejs/node#16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@addaleax
http2: support generic Duplex streams
dbef020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@MylesBorins @addaleax
2017-10-24, Node.js Version 8.8.0 (Current)
e0a3e55
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
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
src: allow top-level calls into JSStream
6cbef5a
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
src: turn JS stream into a full duplex
4e57be1
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
test: add makeDuplexPair() helper
862d9c4
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: nodejs/node#16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
http2: support generic Duplex streams
39ef416
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@MylesBorins @addaleax
2017-10-24, Node.js Version 8.8.0 (Current)
40cbb25
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
@gibfahn gibfahn added dont-land-on-v6.x labels Jan 15, 2018
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
@addaleax @MylesBorins
test: add makeDuplexPair() helper
ca667b9
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: #16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
v6.13.0 proposal #18342
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@addaleax @MylesBorins
test: add makeDuplexPair() helper
b43fe60
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: #16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@addaleax @MylesBorins
test: add makeDuplexPair() helper
9d2c604
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: #16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@addaleax @MylesBorins
test: add makeDuplexPair() helper
af3e074
Add a utility for adding simple, streams-API based duplex pairs.

PR-URL: #16269
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
uannoare pushed a commit to uannoare/node that referenced this pull request Oct 8, 2025
@MylesBorins
2017-10-24, Node.js Version 8.8.0 (Current)
b9cdef5
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.
    https://github.com/nodejs-private/node-private/pull/95

PR-URL: https://github.com/nodejs-private/node-private/pull/98
@pimterry pimterry mentioned this pull request Sep 14, 2026
quic: add Http3Session, so you can explicitly pick the app protocol #65993
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina left review comments
@jasnell jasnell jasnell approved these changes
+1 more reviewer
@apapirovski apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues and PRs related to the http2 subsystem. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@addaleax @apapirovski @mcollina @jasnell @gibfahn @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.