Conversation
|
Just a thought: separating the new dep checkin to a separate commit might make review easier? |
|
This thing is going to be massive and difficult to review no matter how it is split up, unfortunately. |
deps/nghttp2/lib/nghttp2_pq.h
Outdated
There was a problem hiding this comment.
Nitpick: typo on this comment
There was a problem hiding this comment.
That's in the upstream dependency so it would need to be fixed there :-) https://github.com/nghttp2/nghttp2
There was a problem hiding this comment.
thinking about it, I think I need to go add the crypto skip in a couple of the other tests.
Came up in downstream code review by @lucaslago nodejs/node#14239 (comment)
|
@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. :) |
|
Also, for N-API, async_hooks, and some other PRs, we used |
doc/api/http2.md
Outdated
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
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! |
|
@yoshuawuyts it's part of the HTTP2 spec: https://http2.github.io/http2-spec/#HttpResponse |
|
@mcollina cool, thanks for answering!
…On Sat, Jul 15, 2017, 14:47 Matteo Collina ***@***.***> wrote:
@yoshuawuyts <https://github.com/yoshuawuyts> it's part of the HTTP2
spec: https://http2.github.io/http2-spec/#HttpResponse
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14239 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACWleuhN8XM3Dk7G-OD3ZsFjZciMOVUsks5sOLThgaJpZM4OYrK1>
.
|
|
Awesome work! Above you said the following:
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. |
|
@dougwilson .... I'm hoping to get that compat api documented this next week... Including those caveats. |
|
|
doc/api/errors.md
Outdated
|
Thanks @jasnell ! Please feel free to ping me when you do, I would love to take a look. |
doc/api/http2.md
Outdated
doc/api/http2.md
Outdated
There was a problem hiding this comment.
According to using in text,
[ServerHttp2Stream]: [ClientHttp2Stream]: [Http2Stream]:
should be:
[`ServerHttp2Stream`]: [`ClientHttp2Stream`]: [`Http2Stream`]:
doc/api/http2.md
Outdated
There was a problem hiding this comment.
Missing reference for [Buffer][]
doc/api/http2.md
Outdated
|
Issue with the first examples from the main doc: #14304 |
|
@dougwilson Some notable differences, off the top of my head:
|
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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.
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)
```jt client = http2.connect('http://localhost:80');
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);
```
```js
const http2 = require('http2');
const client = http2.connect('http://localhost');
```
Author: Anna Henningsen <anna@addaleax.net>
Author: Colin Ihrig <cjihrig@gmail.com>
Author: Daniel Bevenius <daniel.bevenius@gmail.com>
Author: James M Snell <jasnell@gmail.com>
Author: Jun Mukai
Author: Kelvin Jin
Author: Matteo Collina <matteo.collina@gmail.com>
Author: Robert Kowalski <rok@kowalski.gd>
Author: Santiago Gimeno <santiago.gimeno@gmail.com>
Author: Sebastiaan Deckers <sebdeckers83@gmail.com>
Author: Yosuke Furukawa <yosuke.furukawa@gmail.com>
PR-URL: nodejs#14239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Race condition in the closing of the stream causing failure on some platforms. PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Fixes: nodejs/http2#184 Refines the `'socketError'` event a bit and adds a test for the emission of the `'socketError'` event on the server. Client side is tested separately PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
* respondWithFD now supports optional statCheck * respondWithFD and respondWithFile both support offset/length for range requests * Fix linting nits following most recent update PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than using the `'fetchTrailers'` event to collect trailers, a new `getTrailers` callback option is supported. If not set, the internals will skip calling out for trailers at all. Expands the test to make sure trailers work from the client side also. PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#14239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
@jasnell https://nodejs.org/api/http2.html has many not rendered links now (just search |
|
AFAICT the md has has broken links (or rather links with no definition in the footer) in certain areas. Some were fixed in 85d7d97 |
|
Cross ref: #14857 |
|
Setting don't land on v6.x. Assuming we are not going to backport this |
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
http2module is gated by a new--expose-http2command line flag.When used,
require('http2')will be exposed to users. Note that thereis an existing
http2module on npm that would be impacted by the introductionof this module, which is the main reason for gating this behind a flag.
When using
require('http2')the first time, a process warning will beemitted indicating that an experimental feature is being used.
To run the benchmarks, the
h2loadtool (part of the nghttp project) isrequired:
./node benchmarks/http2/simple.js benchmarker=h2load. Onlytwo benchmarks are currently available.
Additional configuration options to enable verbose debugging are provided:
The
--debug-http2configuration option enables verbose debug statementsfrom the
src/node_http2_*files. The--debug-nghttp2enables the nghttplibrary's own verbose debug output. The
NODE_DEBUG=http2enables JS-leveldebug 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)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http2
[Edit:ofrobots: fixed github handle for Kelvin Jin]