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

[8.x] http2: expose http2 by default, add NODE_NO_HTTP2 #15685

Closed
wants to merge 2 commits into
base: v8.x-staging
from

Conversation

Projects
None yet
@jasnell
Member

jasnell commented Sep 29, 2017

Make --expose-http2 a non-op,
Expose http2 by default.
Add NODE_NO_HTTP2=1 to suppress http2

Note that this flips the default in v8.x, turning http2 always on. For anyone currently using the http2 userland module, they would need to set the NODE_NO_HTTP2=1 environment variable to avoid breaking.

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Sep 29, 2017

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Sep 29, 2017

Member

This is something that is not part of our current semver policy. Is this a semver-major change or not? @nodejs/tsc, what do you think?

I fear this would also set a precedence for node core obscuring modules from the ecosystem without a semver-major change.

Considering the fact that the http2 maintainer is ok in giving up the name, I am less worried about the impact of this change. The new home of them module is https://www.npmjs.com/package/http2.js - if I understand the threads correctly.

@nwgh what do you think?

Member

mcollina commented Sep 29, 2017

This is something that is not part of our current semver policy. Is this a semver-major change or not? @nodejs/tsc, what do you think?

I fear this would also set a precedence for node core obscuring modules from the ecosystem without a semver-major change.

Considering the fact that the http2 maintainer is ok in giving up the name, I am less worried about the impact of this change. The new home of them module is https://www.npmjs.com/package/http2.js - if I understand the threads correctly.

@nwgh what do you think?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 29, 2017

Member

Given that http2 is experimental still, it definitely falls outside the semver rules. Fortunately, we're not introducing new modules very often so hopefully this won't be something we have to deal with that often :-)

Member

jasnell commented Sep 29, 2017

Given that http2 is experimental still, it definitely falls outside the semver rules. Fortunately, we're not introducing new modules very often so hopefully this won't be something we have to deal with that often :-)

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Sep 29, 2017

Member

I know its really late in the game to bring up things like this.... but wasrequire('http/2') considered as a name? It would have side-stepped the name conflict problem.

Member

sam-github commented Sep 29, 2017

I know its really late in the game to bring up things like this.... but wasrequire('http/2') considered as a name? It would have side-stepped the name conflict problem.

@MylesBorins

LGTM

Tiny language nit included, but not blocking. There are a handful of tests that kept empty comments at the top which seems a bit odd

@@ -281,6 +277,10 @@ with small\-icu support.
When set to \fI1\fR, process warnings are silenced.
.TP
.BR NODE_NO_HTTP2 =\fI1\fR
When set to \fI1\fR, the http2 module is suppressed.

This comment has been minimized.

@MylesBorins

MylesBorins Sep 29, 2017

Member

s/supressed/not included in the runtime?

Not 100% that "supressed" gets the message across

@MylesBorins

MylesBorins Sep 29, 2017

Member

s/supressed/not included in the runtime?

Not 100% that "supressed" gets the message across

This comment has been minimized.

@refack

refack Sep 29, 2017

Member

Suggestion: the 'http2' module is not exposed by the runtime
If decided to change should mirror in doc/api/cli.md:440 and src/node.cc:3832

@refack

refack Sep 29, 2017

Member

Suggestion: the 'http2' module is not exposed by the runtime
If decided to change should mirror in doc/api/cli.md:440 and src/node.cc:3832

@@ -1,4 +1,4 @@
// Flags: --expose-http2
//

This comment has been minimized.

@MylesBorins

MylesBorins Sep 29, 2017

Member

is there a reason the comment remains?

@MylesBorins

MylesBorins Sep 29, 2017

Member

is there a reason the comment remains?

@@ -1,8 +0,0 @@
// The --expose-http2 flag is not set

This comment has been minimized.

@MylesBorins

MylesBorins Sep 29, 2017

Member

should we inverse this test to ensure it works? Or is that superfluous

@MylesBorins

MylesBorins Sep 29, 2017

Member

should we inverse this test to ensure it works? Or is that superfluous

This comment has been minimized.

@jasnell

jasnell Sep 29, 2017

Member

largely superfluous, but that does remind me that I need to add a test for the env var :-)

@jasnell

jasnell Sep 29, 2017

Member

largely superfluous, but that does remind me that I need to add a test for the env var :-)

@nwgh

This comment has been minimized.

Show comment
Hide comment
@nwgh

nwgh Sep 29, 2017

@mcollina et al - I'd be totally fine with this change from a naming standpoint.

FTR, the http2.js npm module is a fork that has some bugfixes for newer nodes that I've been hesitant to take on the main node-http2 module (not because I think they're incorrect, but because I have some integration testing on that module that's all done manually, so the burden on taking pull requests is much, much higher - I'm trying to get that automated, but it's slow going).

nwgh commented Sep 29, 2017

@mcollina et al - I'd be totally fine with this change from a naming standpoint.

FTR, the http2.js npm module is a fork that has some bugfixes for newer nodes that I've been hesitant to take on the main node-http2 module (not because I think they're incorrect, but because I have some integration testing on that module that's all done manually, so the burden on taking pull requests is much, much higher - I'm trying to get that automated, but it's slow going).

@refack

refack approved these changes Sep 29, 2017

doc suggestion

@@ -281,6 +277,10 @@ with small\-icu support.
When set to \fI1\fR, process warnings are silenced.
.TP
.BR NODE_NO_HTTP2 =\fI1\fR
When set to \fI1\fR, the http2 module is suppressed.

This comment has been minimized.

@refack

refack Sep 29, 2017

Member

Suggestion: the 'http2' module is not exposed by the runtime
If decided to change should mirror in doc/api/cli.md:440 and src/node.cc:3832

@refack

refack Sep 29, 2017

Member

Suggestion: the 'http2' module is not exposed by the runtime
If decided to change should mirror in doc/api/cli.md:440 and src/node.cc:3832

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Sep 29, 2017

Member

General context:
Is this a precedent (introducing an interim runtime switch)? Can we formalize and document, so if a similar situation arises we'll be consistant.
There is a similar discussion in #15454 around adding --force-async-hooks-checks just for the v8.x LTS.

Member

refack commented Sep 29, 2017

General context:
Is this a precedent (introducing an interim runtime switch)? Can we formalize and document, so if a similar situation arises we'll be consistant.
There is a similar discussion in #15454 around adding --force-async-hooks-checks just for the v8.x LTS.

@@ -9,9 +9,6 @@ can be accessed using:
const http2 = require('http2');
```
*Note*: Node.js must be launched with the `--expose-http2` command line flag
in order to use the `'http2'` module.

This comment has been minimized.

@addaleax

addaleax Sep 29, 2017

Member

I think this might still be useful as information in a YAML changes: entry?

@addaleax

addaleax Sep 29, 2017

Member

I think this might still be useful as information in a YAML changes: entry?

{
std::string text;
config_expose_http2 =
!(SafeGetenv("NODE_NO_HTTP2", &text) && text[0] == '1');

This comment has been minimized.

@addaleax

addaleax Sep 29, 2017

Member

I think we had a similar discussion like this, but this would currently make a distinction between NODE_NO_HTTP2=19 and NODE_NO_HTTP2=20. Why not just … && text == "1"?

@addaleax

addaleax Sep 29, 2017

Member

I think we had a similar discussion like this, but this would currently make a distinction between NODE_NO_HTTP2=19 and NODE_NO_HTTP2=20. Why not just … && text == "1"?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 3, 2017

Member

@jasnell would you be able to rebase?

@nodejs/tsc @nodejs/release can you please chime in

Member

MylesBorins commented Oct 3, 2017

@jasnell would you be able to rebase?

@nodejs/tsc @nodejs/release can you please chime in

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Oct 3, 2017

Member

To me I think that http2 is experimental is not pertinent. This breaks end user code regardless of whether they were using the experimental support or not. As such I think the core question is whether breaking users of an external module is ok in this case, and if in general we consider adding APIs that obscure existing modules semver or not.

Member

mhdawson commented Oct 3, 2017

To me I think that http2 is experimental is not pertinent. This breaks end user code regardless of whether they were using the experimental support or not. As such I think the core question is whether breaking users of an external module is ok in this case, and if in general we consider adding APIs that obscure existing modules semver or not.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Oct 3, 2017

Member

Is it worth considering http/2 as the module name? Its still experimental, so we can change the name.

http/2 seems just as good a name as http2, and it completely sidesteps the name conflict problem and possibly semver-majorness. Would make it easier to backport, I'd think, into LTS.

Member

sam-github commented Oct 3, 2017

Is it worth considering http/2 as the module name? Its still experimental, so we can change the name.

http/2 seems just as good a name as http2, and it completely sidesteps the name conflict problem and possibly semver-majorness. Would make it easier to backport, I'd think, into LTS.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 3, 2017

Member

@sam-github ... renaming the module really is not necessary. The author of the original module on npm has already agreed to hand it over and has already published a deprecated version. Users are already writing code using http2, renaming it at this point is unnecessarily disruptive.

Member

jasnell commented Oct 3, 2017

@sam-github ... renaming the module really is not necessary. The author of the original module on npm has already agreed to hand it over and has already published a deprecated version. Users are already writing code using http2, renaming it at this point is unnecessarily disruptive.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Oct 5, 2017

Member

If http2 is exposed by default, then it masks the http2 installed from npmjs.org, so people using the package version won't be able to upgrade across this, not without changing the environment, right? Technically, this sounds semver-major to me.

The TSC might at their discretion decide this is worth doing in a release stream like 8.x that is not yet LTS.

I'm not sure @nodejs/lts will be keen to make that kind of breaking change in 6.x, though, so if http2 is proposed for backport, it will (EDIT: probably) have to be hidden behind a flag in 6.x, and at least some users find depending on features hidden behind CLI flags to be incredibly onerous.

And of course, existing (EDIT: http2) users are using an API that has absolutely no stability guarantee, removal of the API is explicitly allowed.

This is the last chance, I think, to look forward to the LTS proposal, and I think a number of issues about backwards compatibility become non-issues by renaming now.

/cc @ofrobots

Member

sam-github commented Oct 5, 2017

If http2 is exposed by default, then it masks the http2 installed from npmjs.org, so people using the package version won't be able to upgrade across this, not without changing the environment, right? Technically, this sounds semver-major to me.

The TSC might at their discretion decide this is worth doing in a release stream like 8.x that is not yet LTS.

I'm not sure @nodejs/lts will be keen to make that kind of breaking change in 6.x, though, so if http2 is proposed for backport, it will (EDIT: probably) have to be hidden behind a flag in 6.x, and at least some users find depending on features hidden behind CLI flags to be incredibly onerous.

And of course, existing (EDIT: http2) users are using an API that has absolutely no stability guarantee, removal of the API is explicitly allowed.

This is the last chance, I think, to look forward to the LTS proposal, and I think a number of issues about backwards compatibility become non-issues by renaming now.

/cc @ofrobots

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Oct 5, 2017

Member

@jasnell @sam-github @nwgh Just noticed that the engines field of the http2 module is ">=0.12.0 <9.0.0" ( https://github.com/molnarg/node-http2/blob/v3.3.7/package.json ) so it still can be installed in 8.x (and the <9.0.0 part was only added in v3.3.7), I think for this PR to land we should at least get that field changed to <8.0.0? (If @nwgh is OK with it at least)

Member

joyeecheung commented Oct 5, 2017

@jasnell @sam-github @nwgh Just noticed that the engines field of the http2 module is ">=0.12.0 <9.0.0" ( https://github.com/molnarg/node-http2/blob/v3.3.7/package.json ) so it still can be installed in 8.x (and the <9.0.0 part was only added in v3.3.7), I think for this PR to land we should at least get that field changed to <8.0.0? (If @nwgh is OK with it at least)

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Oct 5, 2017

Member

engines is advisory (https://docs.npmjs.com/files/package.json#engines), but would be good to have set. It shouldn't be <8.0.0 though, I don't think, but less than the specific release number where http2 is exposed by default.

Member

sam-github commented Oct 5, 2017

engines is advisory (https://docs.npmjs.com/files/package.json#engines), but would be good to have set. It shouldn't be <8.0.0 though, I don't think, but less than the specific release number where http2 is exposed by default.

@nwgh

This comment has been minimized.

Show comment
Hide comment
@nwgh

nwgh Oct 8, 2017

I'm more than happy to change the engine version in the http2 npm package to whatever the appropriate 8.x.y is for when this lands. Just need someone to inform me what that is/will be :)

nwgh commented Oct 8, 2017

I'm more than happy to change the engine version in the http2 npm package to whatever the appropriate 8.x.y is for when this lands. Just need someone to inform me what that is/will be :)

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Oct 18, 2017

Member

From the discussion in the TSC meeting (nodejs/TSC#384) this seems reasonable to me.

Member

Fishrock123 commented Oct 18, 2017

From the discussion in the TSC meeting (nodejs/TSC#384) this seems reasonable to me.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 18, 2017

Member

I will rebase this today and get it landed tomorrow

Member

jasnell commented Oct 18, 2017

I will rebase this today and get it landed tomorrow

jasnell added some commits Sep 29, 2017

http2: expose http2 by default, add NODE_NO_HTTP2
Make `--expose-http2` a non-op,
Expose http2 by default.
Add `NODE_NO_HTTP2=1` to suppress http2
http2: remove --expose-http2 flag from tests, etc
Remove the --expose-http2 flag from tests and benchmarks
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 18, 2017

Member

This is rebased.

Member

jasnell commented Oct 18, 2017

This is rebased.

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Oct 18, 2017

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

http2: expose http2 by default, add NODE_NO_HTTP2
Make `--expose-http2` a non-op,
Expose http2 by default.
Add `NODE_NO_HTTP2=1` to suppress http2

PR-URL: #15685
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

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

http2: remove --expose-http2 flag from tests, etc
Remove the --expose-http2 flag from tests and benchmarks

PR-URL: #15685
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Oct 23, 2017

Member

Landed in 57ab6e2 and e01daec. Thanks!

Member

evanlucas commented Oct 23, 2017

Landed in 57ab6e2 and e01daec. Thanks!

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 23, 2017

Member

@nodejs/tsc as this has landed in staging it is ready to go in the next release, which would be tomorrow.

Due to the community concerns should we back this out?

Member

MylesBorins commented Oct 23, 2017

@nodejs/tsc as this has landed in staging it is ready to go in the next release, which would be tomorrow.

Due to the community concerns should we back this out?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 23, 2017

Member

building on the last message. I currently have intent to ship http2 without a flag tomorrow. Opening a new issue

Member

MylesBorins commented Oct 23, 2017

building on the last message. I currently have intent to ship http2 without a flag tomorrow. Opening a new issue

MylesBorins added a commit that referenced this pull request Oct 24, 2017

2017-10-24, Node.js Version 8.8.0 (Current)
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.
    nodejs-private/node-private#95

PR-URL: nodejs-private/node-private#98

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

2017-10-24, Node.js Version 8.8.0 (Current)
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
@mgibson1187

This comment has been minimized.

Show comment
Hide comment
@mgibson1187

mgibson1187 Oct 26, 2017

I know this is kinda a side topic but I am still having issues importing http2 w/webpack.

Any advice or direction toward resources would be much appreciated.

Node Version: 8.8.1

mgibson1187 commented Oct 26, 2017

I know this is kinda a side topic but I am still having issues importing http2 w/webpack.

Any advice or direction toward resources would be much appreciated.

Node Version: 8.8.1

gibfahn added a commit to gibfahn/node that referenced this pull request Oct 31, 2017

test: remove empty comments in http2 tests
I assume they aren't meant to be there.

Refs: nodejs#15685

@gibfahn gibfahn referenced this pull request Oct 31, 2017

Closed

test: remove empty comments in http2 tests #16631

2 of 2 tasks complete

MylesBorins added a commit that referenced this pull request Oct 31, 2017

test: remove empty comments in http2 tests
I assume they aren't meant to be there.

PR-URL: #16631
Refs: #15685
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Nov 4, 2017

Member

@mgibson1187 was your issue resolved? Did you try the webpack repo (https://github.com/webpack/webpack/issues)?

Member

refack commented Nov 4, 2017

@mgibson1187 was your issue resolved? Did you try the webpack repo (https://github.com/webpack/webpack/issues)?

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

2017-10-24, Node.js Version 8.8.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment