http2: add test-http2-date-header.js #94

robertkowalski · 2017-05-08T20:12:14Z

This is porting the http1 test https://github.com/nodejs/http2/blob/98e54b0bd4854bdb3e2949d1b6b20d6777fb7cde/test/parallel/test-http-date-header.js to http2.

I found several issues (see comments in test):

[ ] req.url is undefined, due to missing accessor for url
[ ] req.headers is undefined
[ ] req.getHeader('date') crashes with:

For the header issues, it seems stream[kHeaders] is returning undefined.

Stacktrace:

internal/http2/compat.js:180
    return headers.get(name);
                  ^

TypeError: Cannot read property 'get' of undefined
    at Http2ServerRequest.getHeader (internal/http2/compat.js:180:19)
    at Http2Server.http2.createServer (/Users/robert/projects/http2/test/parallel/test-http2-date-header.js:37:27)
    at emitTwo (events.js:125:13)
    at Http2Server.emit (events.js:213:7)
    at Http2Server.onServerStream (internal/http2/compat.js:497:10)
    at emitThree (events.js:135:13)
    at Http2Server.emit (events.js:216:7)
    at Http2Session.sessionOnStream (internal/http2/core.js:1042:10)
    at emitThree (events.js:135:13)
    at Http2Session.emit (events.js:216:7)

It seems stream[kHeaders] is returning undefined

Code for req.headers and req.getHeader, note the use of stream[kHeaders]:

  get headers() {
    var stream = this[kStream];
    return stream ? stream[kHeaders] : undefined;
  }

  getHeader(name) {
    var stream = this[kStream];
    if (!stream)
      return;
    var headers = stream[kHeaders];
    name = String(name).trim().toLowerCase();
    return headers.get(name);
  }

So far I haven't found out why exactly stream[kHeaders] does not return the right value and need help with that.

UPDATE: all issues fixed.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

http2

robertkowalski · 2017-05-10T21:26:26Z

thanks for your feedback.

i found another issue before i am ready to rebase and i am ready for the final review: #99

mcollina · 2017-05-15T09:36:35Z

@robertkowalski any update on this one?

robertkowalski · 2017-05-15T11:31:00Z

@mcollina i am blocked on #99, except i add a fake listener for data which is an ugly workaround imo

robertkowalski · 2017-05-15T21:29:55Z

thanks for your help and the friendly nudge @mcollina

all is ready now :)

PR-URL: nodejs#3 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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

Squashed rollup of all the progress to this point

PR-URL: nodejs#58 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#65 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Just reading through the code and noticed that the stream event can take headers and flags. Just adding them for readability and asserting the values in the headers and flags. PR-URL: nodejs#71 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs#78 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#64 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#83 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently when compiling the following error is displayed: In file included from /out/Release/obj/gen/node_javascript.cc:6: ../src/env-inl.h:201:7: warning: field 'fs_stats_field_array_' will be initialized after field 'http2_socket_buffer_' [-Wreorder] fs_stats_field_array_(nullptr), ^ In file included from /out/Debug/obj/gen/node_javascript.cc:6: ../src/env-inl.h:201:7: warning: field 'fs_stats_field_array_' will be initialized after field 'http2_socket_buffer_' [-Wreorder] fs_stats_field_array_(nullptr), ^ 1 warning generated. This commit changes the order so that the members appear in the same order in the initializer list. PR-URL: nodejs#85 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

fixes: nodejs#59 Also, add a testcase for http2/TLS secure connection. This verifies to send the server name and ALPN protocols by default. PR-URL: nodejs#61 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This might be an incorrect change but the naming looks to be a little different for these files and wanted to bring it up. Feel free to close if this is indeed correct. PR-URL: nodejs#74 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

These are forbidden by HTTP/2. PR-URL: nodejs#128 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#129 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#120 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Support the socket/connection getter like require('http') does. PR-URL: nodejs#130 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Support the upgrade path from https to http2. PR-URL: nodejs#125 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

* rename functions in docs * add warning against using socket directly * document paddingStrategy / selectPadding * fill in missing details and examples PR-URL: nodejs#132 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fixes: nodejs#135 PR-URL: nodejs#137 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2017-05-31T14:41:56Z

This will need to be rebased :-)

* tighten up the Http2Stream/Http2Session lifecycle, destroy, and error handling. Some simplification, and more new tests * Eliminate queuing of internal callbacks. Queuing these to fire on the next event loop tick was throwing off timing. Now the js callbacks are called directly as they happen. This will require a bit more finesse on the javascript side (to ensure appropiate timing of destroy/shutdown actions) but that is handled from within the core api impl so users should not be affected. * fix debug output with nghttp2 PR-URL: nodejs#138 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Using `./configure --debug-http2` will enable verbose debug statements from node.js, Using `./configure --debug-nghttp2` will enable verbose debug statements from nghttp2. The two can be used together PR-URL: nodejs#138 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

robertkowalski · 2017-06-01T23:40:59Z

fixed, but it seems the tests of other http2 functionality are red right now?

=== release test-http2-client-rststream-before-connect ===
Path: parallel/test-http2-client-rststream-before-connect
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/robert/projects/http2/test/common/index.js:483:10)
    at Object.<anonymous> (/Users/robert/projects/http2/test/parallel/test-http2-client-rststream-before-connect.js:10:28)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
(node:8689) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-client-rststream-before-connect.js
=== release test-http2-client-stream-destroy-before-connect ===
Path: parallel/test-http2-client-stream-destroy-before-connect
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/robert/projects/http2/test/common/index.js:483:10)
    at Object.<anonymous> (/Users/robert/projects/http2/test/parallel/test-http2-client-stream-destroy-before-connect.js:11:28)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
(node:8694) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-client-stream-destroy-before-connect.js
=== release test-http2-create-client-connect ===
Path: parallel/test-http2-create-client-connect
Mismatched noop function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/robert/projects/http2/test/common/index.js:483:10)
    at Http2Server.<anonymous> (/Users/robert/projects/http2/test/parallel/test-http2-create-client-connect.js:46:29)
    at Http2Server.<anonymous> (/Users/robert/projects/http2/test/common/index.js:517:15)
    at emitNone (events.js:105:13)
    at Http2Server.emit (events.js:207:7)
    at emitListeningNT (net.js:1332:10)
    at _combinedTickCallback (internal/process/next_tick.js:99:11)
    at process._tickCallback (internal/process/next_tick.js:161:9)
    at Function.Module.runMain (module.js:607:11)
(node:8707) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-create-client-connect.js
[03:14|% 100|+ 1621|-   3]: Done
make: *** [test] Error 1

jasnell · 2017-06-01T23:57:24Z

Yeah, will need my current PR to land.

* Follow the model of the existing http code and pass the headers in to js as an array. Building the array on the native side then converting it to an object on the js side is generally faster than building the object on the native side. This also allows us to eliminate some duplicated checks. * Fill in additional doc details * Work on timeouts PR-URL: nodejs#148 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-06-06T00:28:11Z

It should work now after a rebase.

Emit `unknownProtocol` event or silently destroy the socket PR-URL: nodejs#144 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

robertkowalski · 2017-06-06T22:40:24Z

sorry, still doesn't work:

/usr/bin/python tools/test.py --mode=release -J \
		doctool inspector known_issues message pseudo-tty parallel sequential \
		async-hooks addons addons-napi
=== release test-http2-client-rststream-before-connect ===
Path: parallel/test-http2-client-rststream-before-connect
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/robert/projects/http2/test/common/index.js:483:10)
    at Object.<anonymous> (/Users/robert/projects/http2/test/parallel/test-http2-client-rststream-before-connect.js:10:28)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
(node:31476) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-client-rststream-before-connect.js
=== release test-http2-client-stream-destroy-before-connect ===
Path: parallel/test-http2-client-stream-destroy-before-connect
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/robert/projects/http2/test/common/index.js:483:10)
    at Object.<anonymous> (/Users/robert/projects/http2/test/parallel/test-http2-client-stream-destroy-before-connect.js:11:28)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
(node:31481) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-client-stream-destroy-before-connect.js
=== release test-http2-create-client-connect ===
Path: parallel/test-http2-create-client-connect
Mismatched noop function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/robert/projects/http2/test/common/index.js:483:10)
    at Http2Server.<anonymous> (/Users/robert/projects/http2/test/parallel/test-http2-create-client-connect.js:46:29)
    at Http2Server.<anonymous> (/Users/robert/projects/http2/test/common/index.js:517:15)
    at emitNone (events.js:105:13)
    at Http2Server.emit (events.js:207:7)
    at emitListeningNT (net.js:1332:10)
    at _combinedTickCallback (internal/process/next_tick.js:99:11)
    at process._tickCallback (internal/process/next_tick.js:161:9)
    at Function.Module.runMain (module.js:607:11)
(node:31494) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-create-client-connect.js
=== release test-http2-server-startup ===
Path: parallel/test-http2-server-startup
(node:31514) ExperimentalWarning: The http2 module is an experimental API.
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at TLSSocket.onHangUp (_tls_wrap.js:1140:19)
    at Object.onceWrapper (events.js:312:19)
    at emitNone (events.js:110:20)
    at TLSSocket.emit (events.js:207:7)
    at endReadableNT (_stream_readable.js:1045:12)
    at _combinedTickCallback (internal/process/next_tick.js:102:11)
    at process._tickCallback (internal/process/next_tick.js:161:9)
Command: out/Release/node /Users/robert/projects/http2/test/parallel/test-http2-server-startup.js
[03:20|% 100|+ 1621|-   4]: Done
make: *** [test] Error 1

(00:40:04) [robert@tequila-new] ~/projects/http2 (test-http2-date-header) $ git log --format=short
commit 8faa0d155c8d1d522fa12478149247cd30f58de9
Author: Robert Kowalski <rok@kowalski.gd>

    [squash] review: add common must-call

commit 24c3779b25287ea030de00f7559688057fca4ee8
Author: Robert Kowalski <rok@kowalski.gd>

    http2: add test-http2-date-header.js

commit 9b6b0676221903592ba5a8145dfd10b87cc60ce6
Author: Sebastiaan Deckers <sebdeckers83@gmail.com>

    http2: reject incompatible TLS ALPN handshakes

commit 897d5b1e66ff72a6bb97334c53a76d01fea25606
Author: James M Snell <jasnell@gmail.com>

    http2: additional cleanups, fixes and docs

jasnell · 2017-06-07T02:38:38Z

Interesting... the only failure I'm getting is the socketHangUp which is fixed by my other PR. What platform are you on?

jasnell · 2017-06-07T14:15:06Z

@mcollina ... can you test to see if you see the same issues @robertkowalski is getting?

mcollina · 2017-06-07T20:58:29Z

I'm getting:

Path: parallel/test-http2-client-rststream-before-connect
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/matteo/Repositories/http2/test/common/index.js:483:10)
    at Object.<anonymous> (/Users/matteo/Repositories/http2/test/parallel/test-http2-client-rststream-before-connect.js:10:28)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
(node:63993) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/matteo/Repositories/http2/test/parallel/test-http2-client-rststream-before-connect.js
[03:06|% 100|+ 1623|-   1]: Done
make: *** [test] Error 1

EDIT: on master, and without this applied.

robertkowalski mentioned this pull request May 8, 2017

http2: get path throws #76

Closed

This comment was marked as off-topic.

Sign in to view

robertkowalski force-pushed the test-http2-date-header branch 2 times, most recently from 8a63c8e to c6f37ae Compare May 15, 2017 21:20

This comment was marked as off-topic.

Sign in to view

jasnell force-pushed the master branch from cf8b5da to 5f17ac4 Compare May 19, 2017 19:47

jasnell and others added 17 commits May 30, 2017 17:24

deps: initial checkin of nghttp2 lib 1.15.0

be065e3

PR-URL: nodejs#3 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http: move utcDate to internal/http.js

e6dc786

tls: add tlsSocket.disableRenegotiation()

bf1d846

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

http2: initial partial implementation

6199295

Squashed rollup of all the progress to this point

http2: fixes to make things work with grpc

31608ba

http2: turn on auto window_size updates

61e253f

http2: fix linting errors in documentation

2ebb716

http2: update nghttp2 to 1.22.0

6404b0d

PR-URL: nodejs#58 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http2: fix test-http2-binding after round 11 (Squash)

7431f0a

PR-URL: nodejs#65 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

src: minor comment correction (Squash)

47d5b4f

PR-URL: nodejs#78 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

http2: client session destroy also destroys associated socket

28fda31

PR-URL: nodejs#64 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

http2: add test for client.destroy behavior

f0f6481

PR-URL: nodejs#64 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

repl: add http2 to built-in libs in REPL

e6f3d48

PR-URL: nodejs#83 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell and others added 8 commits May 30, 2017 17:27

http2: add more connection specific headers

79417fe

These are forbidden by HTTP/2. PR-URL: nodejs#128 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http2: improve header single value checking

e91a1a3

PR-URL: nodejs#129 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http2: statusCode should be a Number

aabdf4a

PR-URL: nodejs#120 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

http2: request.socket|connection

e057863

Support the socket/connection getter like require('http') does. PR-URL: nodejs#130 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

http2: ALPN fallback to HTTP/1.1

f6ea0b7

Support the upgrade path from https to http2. PR-URL: nodejs#125 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http2: update docs

afb7327

* rename functions in docs * add warning against using socket directly * document paddingStrategy / selectPadding * fill in missing details and examples PR-URL: nodejs#132 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http2: improve error handling, reliability and tests

6d2ba82

PR-URL: nodejs#134 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http2: Do not imply a callback to compat res.write

3c6b987

Fixes: nodejs#135 PR-URL: nodejs#137 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell force-pushed the master branch from 5f17ac4 to ee243e4 Compare May 31, 2017 14:26

jasnell added 2 commits May 31, 2017 08:59

jasnell force-pushed the master branch from ee243e4 to f43597b Compare May 31, 2017 15:59

robertkowalski force-pushed the test-http2-date-header branch from c6f37ae to 2d62725 Compare June 1, 2017 23:40

This comment was marked as off-topic.

Sign in to view

sebdeckers and others added 3 commits June 6, 2017 09:50

http2: reject incompatible TLS ALPN handshakes

9b6b067

Emit `unknownProtocol` event or silently destroy the socket PR-URL: nodejs#144 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

http2: add test-http2-date-header.js

24c3779

[squash] review: add common must-call

8faa0d1

robertkowalski force-pushed the test-http2-date-header branch from 2d62725 to 8faa0d1 Compare June 7, 2017 08:47

jasnell force-pushed the master branch from 19fab1b to 547c15e Compare June 22, 2017 20:49

jasnell force-pushed the master branch 2 times, most recently from b271202 to 734ad72 Compare July 14, 2017 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: add test-http2-date-header.js #94

http2: add test-http2-date-header.js #94

robertkowalski commented May 8, 2017 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

robertkowalski commented May 10, 2017

mcollina commented May 15, 2017

robertkowalski commented May 15, 2017

robertkowalski commented May 15, 2017

This comment was marked as off-topic.

jasnell commented May 31, 2017

robertkowalski commented Jun 1, 2017 •

edited

Loading

jasnell commented Jun 1, 2017

This comment was marked as off-topic.

jasnell commented Jun 6, 2017

robertkowalski commented Jun 6, 2017

jasnell commented Jun 7, 2017

jasnell commented Jun 7, 2017

mcollina commented Jun 7, 2017 •

edited

Loading

http2: add test-http2-date-header.js #94

Are you sure you want to change the base?

http2: add test-http2-date-header.js #94

Conversation

robertkowalski commented May 8, 2017 • edited Loading

Checklist

Affected core subsystem(s)

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

robertkowalski commented May 10, 2017

mcollina commented May 15, 2017

robertkowalski commented May 15, 2017

robertkowalski commented May 15, 2017

This comment was marked as off-topic.

jasnell commented May 31, 2017

robertkowalski commented Jun 1, 2017 • edited Loading

jasnell commented Jun 1, 2017

This comment was marked as off-topic.

jasnell commented Jun 6, 2017

robertkowalski commented Jun 6, 2017

jasnell commented Jun 7, 2017

jasnell commented Jun 7, 2017

mcollina commented Jun 7, 2017 • edited Loading

robertkowalski commented May 8, 2017 •

edited

Loading

robertkowalski commented Jun 1, 2017 •

edited

Loading

mcollina commented Jun 7, 2017 •

edited

Loading