http2: compatibility fixes, added docs by mcollina · Pull Request #12 · jasnell/node

mcollina · 2017-07-25T01:44:42Z

I added all the docs (to the best of my knowledge) for the Compatibility API.
I also made writeHead behave similarly between HTTP/1 and HTTP/2, and I removed flushHeaders because it was no longer needed after the change.

cc @sebdeckers

Allows TLS renegotiation to be disabled per `TLSSocket` instance. Per HTTP/2, TLS renegotiation is forbidden after the initial connection prefix is exchanged.

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>

Race condition in the closing of the stream causing failure on some platforms.

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

Fixes: nodejs/http2#179 Was fixing issue nodejs#179 and encountered a segault that was happening somewhat randomly on session destruction. Both should be fixed

* respondWithFD now supports optional statCheck * respondWithFD and respondWithFile both support offset/length for range requests * Fix linting nits following most recent update

sebdeckers · 2017-07-25T14:16:11Z

@mcollinares.flushHeaders is not documented but does exist as an HTTP/1 API. Various tests cover this feature. Wonder if this will break expected behaviour.

https://github.com/nodejs/node/blob/69fdb47e207f9201f36593ba72440e122d193743/lib/_http_outgoing.js#L879

Excellent work on the docs! 👏

mcollina · 2017-07-25T14:24:56Z

@sebdeckers I would prefer not to have it as it is undocumented. If people are using it, we should document it in HTTP/1 as well, and support it. My guess is that it is some leftover from the past

To the best of your knowledge, is it used in Express or similar?

mcollina · 2017-07-25T14:50:36Z

@sebdeckers it's documented for the client request object (https://nodejs.org/api/http.html#http_request_flushheaders), but not for the server response. test-http-flush-response-headers.js covers it for the server response.

@jasnell what do you think?

jasnell · 2017-07-25T15:09:39Z

Agree that it should be documented.
One nit... this should land against the initial-pr branch in the nodejs/http2 repo, not my jasnell/node repo. I'll keep this repo in sync for the PR, but let's direct the work over there.

jasnell · 2017-07-25T15:10:11Z

jasnell · 2017-07-25T15:11:13Z

If we are going to list these here, I think we need to be more explicit about these being part of the compatibility API and calling out the differences between this and the 'stream' event.

I think this is great progress, I would prefer landing this, and then we can check where things can be updated. Having some docs for the compat API would help.

jasnell · 2017-07-25T15:12:05Z

jasnell · 2017-07-25T15:14:53Z

long line... will stop pointing these out :-)

mcollina · 2017-07-25T18:50:56Z

One nit... this should land against the initial-pr branch in the nodejs/http2 repo, not my jasnell/node repo. I'll keep this repo in sync for the PR, but let's direct the work over there.

I would need to cherry pick the commits then, the two branches diverged too much.

mcollina · 2017-07-25T19:00:43Z

@sebdeckers I readded flushHeaders

mcollina · 2017-07-25T19:10:52Z

@jasnell I've fixed the line length.

This function call can fail with `Z_VERSION_ERROR` if the compiled library vs loaded library mismatched in version number or in stream structure size. In those cases, zlib doesn't initialize the `strm_.msg` field to null. Therefore, when a `CompressionError` object is created via `ErrorForMessage()`, it can read a stale or uninitialized `strm_.msg` pointer that will cause a crash. Example ASAN report: ``` AddressSanitizer: SEGV on unknown address #0 __strlen_avx2 string/../sysdeps/x86_64/multiarch/strlen-avx2.S:76 #1 strlen (/work/node/out/Debug/node+0x1a42ab7) #2 v8::(anonymous namespace)::StringLength(char const*) /work/node/out/../deps/v8/src/api/api.cc:7581:16 #3 v8::(anonymous namespace)::StringLength(unsigned char const*) /work/node/out/../deps/v8/src/api/api.cc:7587:10 #4 v8::String::NewFromOneByte(v8::Isolate*, unsigned char const*, v8::NewStringType, int) /work/node/out/../deps/v8/src/api/api.cc:7677:3 #5 node::OneByteString(v8::Isolate*, char const*, int, v8::NewStringType) /work/node/out/../src/util-inl.h:166:10 #6 node::(anonymous namespace)::CompressionStream< node::(anonymous namespace)::ZlibContext> ::EmitError(node::(anonymous namespace) ::CompressionError const&) /work/node/out/../src/node_zlib.cc:565:7 #7 node::(anonymous namespace)::CompressionStream< node::(anonymous namespace)::ZlibContext> ::CheckError() /work/node/out/../src/node_zlib.cc:519:5 #8 node::(anonymous namespace)::CompressionStream< node::(anonymous namespace)::ZlibContext> ::AfterThreadPoolWork(int) /work/node/out/../src/node_zlib.cc:543:10 #9 node::ThreadPoolWork::ScheduleWork() ::'lambda'(uv_work_s*, int) ::operator()(uv_work_s*, int) const /work/node/out/../src/threadpoolwork-inl.h:57:15 #10 node::ThreadPoolWork::ScheduleWork() ::'lambda'(uv_work_s*, int) ::__invoke(uv_work_s*, int) /work/node/out/../src/threadpoolwork-inl.h:48:7 #11 uv__work_done /work/libuv-1.51.0/src/threadpool.c:330:5 #12 uv__async_io.part.0 /work/libuv-1.51.0/src/unix/async.c:208:5 ``` Signed-off-by: ndossche <nora.dossche@ugent.be> PR-URL: nodejs#63476 Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added 14 commits July 21, 2017 17:20

http: move utcDate to internal/http.js
403e33d

tls: add tlsSocket.disableRenegotiation()
873ea66
Allows TLS renegotiation to be disabled per `TLSSocket` instance. Per HTTP/2, TLS renegotiation is forbidden after the initial connection prefix is exchanged.

deps: add nghttp2 dependency
515c6aa

http2: add tests and benchmarks
2acc5ae

http2: remove redundant return in test
823fd83

http2: fix documentation nits
a91b463

doc: include http2.md in all.md
aeffae8

test: fix flakiness in test-http2-client-upload
f79e2d1
Race condition in the closing of the stream causing failure on some platforms.

test: fix flaky test-http2-client-unescaped-path on osx
27fd5dc

http2: fix abort when client.destroy inside end event
78be06c

http2: refinement and test for socketError
0281788
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

http2: fix socketOnTimeout and a segfault
477f84b
Fixes: nodejs/http2#179 Was fixing issue nodejs#179 and encountered a segault that was happening somewhat randomly on session destruction. Both should be fixed

http2: add range support for respondWith{File|FD}
c9eb404
* respondWithFD now supports optional statCheck * respondWithFD and respondWithFile both support offset/length for range requests * Fix linting nits following most recent update

jasnell reviewed Jul 25, 2017
View reviewed changes

Comment threaddoc/api/http2.md Outdated
jasnellJul 25, 2017
Copy link
Copy Markdown
Owner
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long line?

jasnell reviewed Jul 25, 2017
View reviewed changes

Comment threaddoc/api/http2.md Outdated
jasnellJul 25, 2017
Copy link
Copy Markdown
Owner
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long line

jasnell reviewed Jul 25, 2017
View reviewed changes

Comment threaddoc/api/http2.md Outdated
jasnellJul 25, 2017
Copy link
Copy Markdown
Owner
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long line... will stop pointing these out :-)

mcollina force-pushed the jasnell-initial-pr-fixes branch from 6a36561 to 3311227Compare July 25, 2017 18:49

mcollina added 2 commits July 25, 2017 12:08

http2: doc and fixes to the Compatibility API
a93f39e

http2: make writeHead behave like HTTP/1.
2c560bb

mcollina force-pushed the jasnell-initial-pr-fixes branch from 3311227 to 2c560bbCompare July 25, 2017 19:08

jasnell force-pushed the initial-pr branch from b806aa1 to 358b3dfCompare August 3, 2017 18:59

jasnell force-pushed the initial-pr branch from e286d61 to 873c1c5Compare August 4, 2017 17:21

jasnell added fast-track help wanted bug duplicate and removed fast-track help wanted bug duplicate labels Apr 27, 2021

jasnell closed this Apr 28, 2021

Navigation Menu

http2: compatibility fixes, added docs - #12

http2: compatibility fixes, added docs#12
mcollina wants to merge 16 commits into
jasnell:initial-prfrom
mcollina:jasnell-initial-pr-fixes

mcollina commented Jul 25, 2017

Uh oh!

sebdeckers commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

jasnell commented Jul 25, 2017

Uh oh!

jasnellJul 25, 2017

Uh oh!

jasnellJul 25, 2017

Uh oh!

mcollinaJul 25, 2017

Uh oh!

jasnellJul 25, 2017

Uh oh!

jasnellJul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mcollina commented Jul 25, 2017

Uh oh!

sebdeckers commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

jasnell commented Jul 25, 2017

Uh oh!

jasnellJul 25, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellJul 25, 2017

Choose a reason for hiding this comment

Uh oh!

mcollinaJul 25, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellJul 25, 2017

Choose a reason for hiding this comment

Uh oh!

jasnellJul 25, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

mcollina commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants