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

http: support generic Duplex streams #16267

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 17, 2017

Support generic Duplex streams through more duck typing
on the server and client sides.

Since HTTP is, as a protocol, independent of its underlying transport
layer, Node.js should not enforce any restrictions on what streams
its HTTP parser 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)

http

@addaleax addaleax added http Issues or PRs related to the http 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 the http Issues or PRs related to the http subsystem. label Oct 17, 2017
@addaleax
Copy link
Member Author

addaleax commented Oct 17, 2017

@jasnell jasnell requested a review from mcollina Oct 17, 2017
@jasnell
Copy link
Member

jasnell commented Oct 17, 2017

I like it. At first read through on the code nothing strikes me as problematic but I've learned the hard way to be conservative when it comes to the http code. If express and hapi continue to work with these changes, then ++.

@addaleax
Copy link
Member Author

addaleax commented Oct 17, 2017

@jasnell If github would allow, I’d react to your comment with 💯 – I am hopeful that this is fine, because it turns errors into non-errors, but… guess we’ll see if CITGM comes up with anything.

@@ -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-helper)
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 17, 2017

Choose a reason for hiding this comment

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

#duplex-pair-module?

Copy link
Member Author

@addaleax addaleax Oct 17, 2017

Choose a reason for hiding this comment

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

Yes. :)

@@ -453,6 +454,14 @@ Reads a Domain String and returns a Buffer containing the domain.
Takes in a parsed Object and writes its fields to a DNS packet as a Buffer
object.

## Duplex pair Module

The `common/duplexpair` module exports a single function `MakeDuplexPair`,
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 17, 2017

Choose a reason for hiding this comment

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

MakeDuplexPair()?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 17, 2017

Choose a reason for hiding this comment

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

Another nit: is it OK to name a not constructor function with a first upper case letter?

Copy link
Member Author

@addaleax addaleax Oct 17, 2017

Choose a reason for hiding this comment

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

Fwiw, I would have written it as a constructor/ES6 class if that wouldn’t have made the code more complicated, and that’s why it’s uppercase. I am happy to change it to whatever you prefer :)

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 17, 2017

Choose a reason for hiding this comment

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

If this will not make users try to use it with new, I am OK with any name)

Copy link
Contributor

@cjihrig cjihrig Oct 18, 2017

Choose a reason for hiding this comment

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

I would prefer this not be capitalized.

Copy link
Member

@mcollina mcollina left a comment

Does this interfer anyhow with async_hooks?

@addaleax
Copy link
Member Author

addaleax commented Oct 18, 2017

Does this interfer anyhow with async_hooks?

This really doesn’t do much more besides loosening requirements on the stream type, so I’d go with “no”

Copy link
Member

@mcollina mcollina left a comment

LGTM, with the caveat that I will let this bake for a bit in 9 before backporting to 8 as semver-minor.

It needs a green CITGM.

@addaleax
Copy link
Member Author

addaleax commented Oct 18, 2017

@mcollina I’ve looked through the failures in the linked CI run, there’s nothing in this that’s related to this PR. (The q and ffi failures are real though and I’ll look into them when I get the chance – just not coming from this PR.)

@mcollina mcollina added baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v8.x labels Oct 18, 2017
@mcollina
Copy link
Member

mcollina commented Oct 18, 2017

LGTM and let's get this in 9!

@@ -453,6 +454,14 @@ Reads a Domain String and returns a Buffer containing the domain.
Takes in a parsed Object and writes its fields to a DNS packet as a Buffer
object.

## Duplex pair Module

The `common/duplexpair` module exports a single function `MakeDuplexPair`,
Copy link
Contributor

@cjihrig cjihrig Oct 18, 2017

Choose a reason for hiding this comment

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

I would prefer this not be capitalized.

Copy link
Member

@jasnell jasnell left a comment

LGTM, but I agree with the nit about s/MakeDuplexPair/makeDuplexPair

addaleax added 2 commits Oct 22, 2017
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 more duck typing
on the server and client sides.

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

Ref: nodejs#16256
PR-URL: nodejs#16267
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Oct 22, 2017

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

This should be ready.

@addaleax
Copy link
Member Author

addaleax commented Oct 23, 2017

Landed in 3e25e4d

@addaleax addaleax closed this Oct 23, 2017
@addaleax addaleax deleted the http-generic-streams branch Oct 23, 2017
addaleax added a commit that referenced this pull request Oct 23, 2017
Support generic `Duplex` streams through more duck typing
on the server and client sides.

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

Ref: #16256
PR-URL: #16267
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
Support generic `Duplex` streams through more duck typing
on the server and client sides.

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

Ref: nodejs/node#16256
PR-URL: nodejs/node#16267
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Support generic `Duplex` streams through more duck typing
on the server and client sides.

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

Ref: nodejs/node#16256
PR-URL: nodejs/node#16267
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

@addaleax if you want to see this on 6.x let us know (or raise a backport PR)

MylesBorins pushed a commit that referenced this pull request Jan 15, 2018
Support generic `Duplex` streams through more duck typing
on the server and client sides.

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

Ref: #16256
PR-URL: #16267
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn added a commit that referenced this pull request Mar 6, 2018
Notable changes:

* deps:
  * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413)
  * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413)
  * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
  * re land npm 5.6.0 (Myles Borins) [#18625](#18625)
  * ICU 60 bump (Steven R. Loomis) [#16876](#16876)
* crypto:
  * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130)
  * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566)
* async_hooks:
  * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004)
  * use typed array stack as fast path (Anna Henningsen) [#17780](#17780)
  * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273)
  * separate missing from default context (Andreas Madsen) [#17273](#17273)
  * rename initTriggerId (Andreas Madsen) [#17273](#17273)
  * deprecate undocumented API (Andreas Madsen) [#16972](#16972)
  * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998)
  * add trace events to async_hooks (Andreas Madsen) [#15538](#15538)
  * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003)
  * add provider types for net server (Andreas Madsen) [#17157](#17157)
* n-api:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
* cli:
  * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495)
* console:
  * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033)
* module:
  * add builtinModules (Jon Moss) [#16386](#16386)
  * replace default paths in require.resolve() (cjihrig) [#17113](#17113)
* src:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
  * add process.ppid (cjihrig) [#16839](#16839)
* http:
  * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267)
  * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672)
  * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772)
* net:
  * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662)
* process:
  * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463)
* tls:
  * unconsume stream on destroy (Anna Henningsen) [#17478](#17478)
* process:
  * improve unhandled rejection message (Madara Uchiha) [#17158](#17158)
* stream:
  * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860)
* trace_events:
  * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196)

PR-URL: #18336
gibfahn added a commit that referenced this pull request Mar 7, 2018
Notable changes:

* deps:
  * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413)
  * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413)
  * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
  * re land npm 5.6.0 (Myles Borins) [#18625](#18625)
  * ICU 60 bump (Steven R. Loomis) [#16876](#16876)
* crypto:
  * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130)
  * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566)
* async_hooks:
  * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004)
  * use typed array stack as fast path (Anna Henningsen) [#17780](#17780)
  * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273)
  * separate missing from default context (Andreas Madsen) [#17273](#17273)
  * rename initTriggerId (Andreas Madsen) [#17273](#17273)
  * deprecate undocumented API (Andreas Madsen) [#16972](#16972)
  * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998)
  * add trace events to async_hooks (Andreas Madsen) [#15538](#15538)
  * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003)
  * add provider types for net server (Andreas Madsen) [#17157](#17157)
* n-api:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
* cli:
  * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495)
* console:
  * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033)
* module:
  * add builtinModules (Jon Moss) [#16386](#16386)
  * replace default paths in require.resolve() (cjihrig) [#17113](#17113)
* src:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
  * add process.ppid (cjihrig) [#16839](#16839)
* http:
  * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267)
  * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672)
  * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772)
* net:
  * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662)
* process:
  * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463)
* tls:
  * unconsume stream on destroy (Anna Henningsen) [#17478](#17478)
* process:
  * improve unhandled rejection message (Madara Uchiha) [#17158](#17158)
* stream:
  * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860)
* trace_events:
  * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196)

PR-URL: #18336
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Notable changes:

* deps:
  * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413)
  * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413)
  * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260)
  * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625)
  * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876)
* crypto:
  * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130)
  * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566)
* async_hooks:
  * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004)
  * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780)
  * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273)
  * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273)
  * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273)
  * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972)
  * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998)
  * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538)
  * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003)
  * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157)
* n-api:
  * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109)
* cli:
  * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495)
* console:
  * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033)
* module:
  * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386)
  * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113)
* src:
  * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109)
  * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839)
* http:
  * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267)
  * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672)
  * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772)
* net:
  * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662)
* process:
  * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463)
* tls:
  * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478)
* process:
  * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158)
* stream:
  * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860)
* trace_events:
  * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196)

PR-URL: nodejs#18336
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. 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.

None yet

9 participants