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

http2: Introducing http/2 #14239

Closed
wants to merge 26 commits into from
Closed

http2: Introducing http/2 #14239

wants to merge 26 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 14, 2017

At long last: The initial experimental implementation of HTTP/2.

This is an accumulation of the work that has been done in the nodejs/http2
repository, squashed down to a couple of commits. The original commit
history has been preserved in the nodejs/http2 repository.

Contributors to this work include:

This PR introduces the nghttp2 C library as a new dependency. This library
provides the majority of the HTTP/2 protocol implementation, with the rest
of the code here providing the mapping of the library into a usable JS API.

Within src, a handful of new node_http2_.c and node_http2_.h files are
introduced. These provide the internal mechanisms that interface with nghttp
and define the process.binding('http2') interface.

The JS API is defined within internal/http2/*.js.

There are two APIs provided: Core and Compat.

The Core API is HTTP/2 specific and is designed to be as minimal and as
efficient as possible.

The Compat API is intended to be as close to the existing HTTP/1 API as
possible, with some exceptions.

Tests, documentation and initial benchmarks are included.

The http2 module is gated by a new --expose-http2 command line flag.
When used, require('http2') will be exposed to users. Note that there
is an existing http2 module on npm that would be impacted by the introduction
of this module, which is the main reason for gating this behind a flag.

When using require('http2') the first time, a process warning will be
emitted indicating that an experimental feature is being used.

To run the benchmarks, the h2load tool (part of the nghttp project) is
required: ./node benchmarks/http2/simple.js benchmarker=h2load. Only
two benchmarks are currently available.

Additional configuration options to enable verbose debugging are provided:

$ ./configure --debug-http2 --debug-nghttp2
$ NODE_DEBUG=http2 ./node

The --debug-http2 configuration option enables verbose debug statements
from the src/node_http2_* files. The --debug-nghttp2 enables the nghttp
library's own verbose debug output. The NODE_DEBUG=http2 enables JS-level
debug output.

The following illustrates as simple HTTP/2 server and client interaction:

(The HTTP/2 client and server support both plain text and TLS connections)

const req = client.request({ ':path': '/some/path' });
req.on('data', (chunk) => { /* do something with the data */ });
req.on('end', () => {
  client.destroy();
});

// Plain text (non-TLS server)
const server = http2.createServer();
server.on('stream', (stream, requestHeaders) => {
  stream.respond({ ':status': 200 });
  stream.write('hello ');
  stream.end('world');
});
server.listen(80);
const http2 = require('http2');
const client = http2.connect('http://localhost');
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

[Edit:ofrobots: fixed github handle for Kelvin Jin]

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 14, 2017
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 14, 2017
@jasnell jasnell added experimental Issues and PRs related to experimental features. http2 Issues or PRs related to the http2 subsystem. labels Jul 14, 2017
@jasnell jasnell changed the title Initial pr http2: Introducing http/2 Jul 14, 2017
@Fishrock123
Copy link
Member

Just a thought: separating the new dep checkin to a separate commit might make review easier?

@jasnell
Copy link
Member Author

jasnell commented Jul 14, 2017

This thing is going to be massive and difficult to review no matter how it is split up, unfortunately.

nghttp2_pq_entry **q;
/* Memory allocator */
nghttp2_mem *mem;
/* The number of items sotred */
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: typo on this comment

Copy link
Member Author

Choose a reason for hiding this comment

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

That's in the upstream dependency so it would need to be fixed there :-) https://github.com/nghttp2/nghttp2

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed upstream nghttp2/nghttp2#958


if (!common.hasCrypto) {
common.skip('missing crypto');
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: redundant after #14021

Copy link
Member Author

Choose a reason for hiding this comment

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

thinking about it, I think I need to go add the crypto skip in a couple of the other tests.

sebdeckers added a commit to sebdeckers/nghttp2 that referenced this pull request Jul 14, 2017
Came up in downstream code review by @lucaslago nodejs/node#14239 (comment)
@addaleax
Copy link
Member

@jasnell We also split out the deps commits for node-inspect and for the inspector iirc; that worked out pretty well and did help with reviewing. :)

@addaleax
Copy link
Member

Also, for N-API, async_hooks, and some other PRs, we used Author: name <email> metadata (example: 56e881d) for specifying multiple authors. Feel invited to adapt that format. :) (That has the nice side effect of not pinging everybody everytime you rebase/the commit is landed/etc.)


An object describing the current status of this `Http2Session`.

#### http2session.priority(stream, options)
Copy link
Member

Choose a reason for hiding this comment

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

This does the same thing as stream.priority(options), right? In that case I think I’d prefer not to expose it (same thing for rstStream)

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, yes, but eventually this will support the ability to pass in a numeric stream identifier rather than the Http2Stream object. The HTTP/2 spec allows priorities and rst_stream frames to be sent independently, even without a stream having been previously established.

@yoshuawuyts
Copy link
Member

The colon prefix in :status looks a bit odd to me. I'm sure there have been careful considerations made as to why that makes sense. Would you mind sharing the reasoning behind using colon prefixes? Thanks!

@mcollina
Copy link
Member

@yoshuawuyts it's part of the HTTP2 spec: https://http2.github.io/http2-spec/#HttpResponse

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 15, 2017 via email

@dougwilson
Copy link
Member

Awesome work! Above you said the following:

The Compat API is intended to be as close to the existing HTTP/1 API as possible, with some exceptions.

I tried looking in the documentation included in this PR, but couldn't figure out what these exceptions are. Can you ellaborate? Is the compatibility API just the same how the "spdy" module does the HTTP/2 -> 1 compatibility API or different somehow? I'm not really looking for a comparison to the "spdy" module, but thought if you knew that would help, otherwise would love to see the docs around what these differences are.

@jasnell
Copy link
Member Author

jasnell commented Jul 15, 2017

@dougwilson .... I'm hoping to get that compat api documented this next week... Including those caveats.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 15, 2017

http2 should also be added to all.md.

<a id="ERR_HTTP2_STREAM_ERROR"></a>
### ERR_HTTP2_STREAM_ERROR

Used when a non-zero error code has been specified in an RST_STREAM frame.
Copy link
Contributor

Choose a reason for hiding this comment

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

`RST_STREAM`?


To run the `http2` benchmarks, the `h2load` benchmarker must be used. The
`h2load` tool is a component of the `nghttp2` project and may be installed
from [nghttp.org][] or built from source.
Copy link
Contributor

Choose a reason for hiding this comment

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

nghttp2.org

@dougwilson
Copy link
Member

Thanks @jasnell ! Please feel free to ping me when you do, I would love to take a look.

doc/api/http2.md Outdated
[`tls.TLSSocket`]: tls.html
[`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
[ClientHttp2Stream]: #http2_class_clienthttp2stream
[Compatibility API: #http2_compatibility_api
Copy link
Contributor

Choose a reason for hiding this comment

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

missing ]

doc/api/http2.md Outdated
[`net.Socket`]: net.html
[`tls.TLSSocket`]: tls.html
[`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
[ClientHttp2Stream]: #http2_class_clienthttp2stream
Copy link
Contributor

Choose a reason for hiding this comment

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

According to using in text,

[ServerHttp2Stream]: [ClientHttp2Stream]: [Http2Stream]:

should be:

[`ServerHttp2Stream`]: [`ClientHttp2Stream`]: [`Http2Stream`]:

doc/api/http2.md Outdated
* `settings` {[Settings Object][]}
* Returns: {Buffer}

Returns a [Buffer][] instance containing serialized representation of the given
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing reference for [Buffer][]

doc/api/http2.md Outdated
* A new HTTP/2 `HEADERS` frame with a previously unused stream ID is received;
* The `http2stream.pushStream()` method is called.

On the client side, instances of [`ClientHttp2Stream`[] are created when the
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing ]

@vsemozhetbyt
Copy link
Contributor

Issue with the first examples from the main doc: #14304

@sebdeckers
Copy link
Contributor

sebdeckers commented Jul 16, 2017

@dougwilson Some notable differences, off the top of my head:

  • Most private variables are not supported. Only the documented methods, properties, and events are supported. Code that relies on undocumented behaviour is not supported by the compat api. E.g. no more _header checking; use headersSent instead.

  • The request and response objects do not inherit from http.IncomingMessage and http.ServerResponse respectively. This poses a challenge for express@4, in my experience.

  • No status message in sendHead because HTTP/2 dropped the concept of text status messages. Status code is now the :status header. A process warning is emitted if the (optional) status message is passed. The message itself is ignored. Status code and rest of the headers are sent.

  • The socket property should not be written to. HTTP/2 has a framing layer and does not directly expose the socket. Only the low-level nghttp2 library deals with the socket. However meta properties on the socket can be useful, like detecting http vs https, address/port, TLS info, etc.

  • Only status codes 1xx-5xx are supported. This follows the HTTP spec. Previously in the HTTP/1 API status codes 1xx-9xx were allowed.

  • Extensions:

    • The [req|res].stream property exposes the HTTP/2 stream, which in turn exposes the HTTP/2 session.
    • res.createPushResponse(headers, cb) can use used to send a PUSH_PROMISE frame. The callback receives the corresponding Http2ServerResponse instance via which headers and a body can be sent.

addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 14, 2017
Backport-PR-URL: #14813
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Timothy Gu <timothygu99@gmail.com>

PR-URL: #14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax added a commit that referenced this pull request Aug 14, 2017
As discussed in the review for
#14239, these buffers should
be per-Environment rather than static.

PR-URL: #14744
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Aug 14, 2017
4 tasks
addaleax added a commit that referenced this pull request Aug 14, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
@addaleax addaleax mentioned this pull request Aug 14, 2017
evanlucas pushed a commit that referenced this pull request Aug 15, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811
@vsemozhetbyt
Copy link
Contributor

@jasnell https://nodejs.org/api/http2.html has many not rendered links now (just search ][ in a browser, skipping headings).

@refack
Copy link
Member

refack commented Aug 16, 2017

AFAICT the md has has broken links (or rather links with no definition in the footer) in certain areas. Some were fixed in 85d7d97

@refack
Copy link
Member

refack commented Aug 16, 2017

Cross ref: #14857

MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs/node#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs/node#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs/node#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs/node#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs/node#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs/node#14558)

PR-URL: nodejs/node#14811
@MylesBorins
Copy link
Member

Setting don't land on v6.x. Assuming we are not going to backport this

@Hackzzila Hackzzila mentioned this pull request May 2, 2018
4 tasks
sam-github added a commit to sam-github/node that referenced this pull request Feb 7, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: nodejs#14239
See: nodejs@b1909d3a70f9
danbev pushed a commit that referenced this pull request Feb 13, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

PR-URL: #25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 13, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

PR-URL: #25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request May 28, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: nodejs#14239
See: nodejs@b1909d3a70f9

PR-URL: nodejs#25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this pull request Jun 7, 2019
In the initial version of this test there were two zero-length writes to
force tls state to cycle. The second is not necessary, at least not now,
but the first was. The renegotiate() API should ensure that packet
exchange takes place, not its users, so move the zero-length write into
tls.

See: #14239
See: b1909d3a70f9

Backport-PR-URL: #27938
PR-URL: #25997
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. experimental Issues and PRs related to experimental features. http2 Issues or PRs related to the http2 subsystem. notable-change PRs with changes that should be highlighted in changelogs. 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