test: increase Http2ServerResponse test coverage #15074

apapirovski · 2017-08-29T13:48:55Z

A few bundled changes, all for Http2ServerResponse, that are too small for separate PRs:

Modified existing header tests for Http2ServerResponse to include sendDate (get and set) and headersSent
Expanded existing test for end to include a check for closed
New test for destroy

Refs: #14985

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

benjamingr · 2017-08-31T17:25:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9906/console

benjamingr · 2017-09-01T11:11:53Z

@nodejs/build

gibfahn · 2017-09-01T12:16:25Z

@benjamingr did you mean to cc/ @nodejs/http2

benjamingr · 2017-09-01T12:20:22Z

@gibfahn no, I wasn't sure if these are build related failures or code related failures.

gibfahn · 2017-09-01T12:25:58Z

@gibfahn no, I wasn't sure if these are build related failures or code related failures.

Oh I see, I didn't realise the ping was related to CI failures.

EDIT: The arm failure is nodejs/build#857, should be fixed now. The other two failures I'm not sure about.

apapirovski · 2017-09-01T15:48:57Z

It looks like OSX has been failing since 5am on 31st so it's probably not related to this.

mcollina · 2017-09-01T15:59:56Z

test/parallel/test-http2-compat-serverresponse-destroy.js

+
+  if (path === '/error') {
+    req.once('error', (err) => assert.strictEqual(err, error));
+    res.once('error', (err) => assert.strictEqual(err, error));


Is this behavior shared with HTTP1? Can we check?

Will confirm today.

So in h1 it doesn't emit any server-side error events (on req, res or server itself) which seems a bit weird. It causes a socket hang up which then throws an error on the client-side request (ECONNRESET).

This case is calling destroy(err) on the response, what does happen on H1?

In #14991, errors from the underlining socket are not forwarded to req and res anymore.

Actually, after testing this further, this code is clearly outdated. I was working on it at the same time as you were working on refactoring how the errors are emitted. Will revise.

mcollina · 2017-09-01T16:13:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9914/

jasnell · 2017-09-01T17:18:31Z

Looks like the osx build bot is consistently dead. ping @nodejs/build

refack · 2017-09-01T17:27:37Z

Looks like the osx build bot is consistently dead. ping @nodejs/build

Looking into it.

Modify existing header tests for Http2ServerResponse to include sendDate (get and set) and headersSent. Expand existing test for end to include a check for closed. Add a new test for destroy. Refs: nodejs#14985

apapirovski · 2017-09-01T19:22:20Z

@mcollina I had to rebase this locally to get the changes to error handling that you made. I've now rewritten the test to match the new behaviour. Should make more sense now. Thanks for the review!

(Also I really should've had common.mustCall on those error handlers then the build would've told us this wasn't working.)

refack · 2017-09-01T21:41:58Z

OSX fail ref: nodejs/build#861

mcollina · 2017-09-02T07:34:23Z

test/parallel/test-http2-compat-serverresponse-destroy.js

+
+const server = http2.createServer(common.mustCall((req, res) => {
+  req.once('error', common.mustNotCall());
+  req.once('error', common.mustNotCall());


this are both req. I think res should receive the error, considering the semantics of .destroy(err) throughout the codebase. Check with H1, but I think we have a bug in the compat layer because of this.

apapirovski · 2017-09-02T12:16:24Z

@mcollina I fixed the typo and there's no error emitted on res. Also, checked H1 and it doesn't emit an error on res either. Only the socket receives the error, as far as I can tell. It doesn't bubble up.

Looking at the documentation, calling destroy on the ServerResponse isn't actually documented in H1 but it is documented for the IncomingMessage:

Calls destroy() on the socket that received the IncomingMessage. If error is provided, an 'error' event is emitted and error is passed as an argument to any listeners on the event.

And the destroy code for both is equivalent in terms of error handling.

mcollina

LGTM

mcollina · 2017-09-03T10:25:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9928/

BridgeAR · 2017-09-03T23:19:44Z

Landed in 198fcb9

Modify existing header tests for Http2ServerResponse to include sendDate (get and set) and headersSent. Expand existing test for end to include a check for closed. Add a new test for destroy. PR-URL: #15074 Refs: #14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Modify existing header tests for Http2ServerResponse to include sendDate (get and set) and headersSent. Expand existing test for end to include a check for closed. Add a new test for destroy. PR-URL: nodejs/node#15074 Refs: nodejs/node#14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Modify existing header tests for Http2ServerResponse to include sendDate (get and set) and headersSent. Expand existing test for end to include a check for closed. Add a new test for destroy. PR-URL: #15074 Refs: #14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 29, 2017

jasnell approved these changes Aug 29, 2017

View reviewed changes

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Aug 29, 2017

benjamingr approved these changes Aug 30, 2017

View reviewed changes

mcollina reviewed Sep 1, 2017

View reviewed changes

test: increase Http2ServerResponse test coverage

ad35687

Modify existing header tests for Http2ServerResponse to include sendDate (get and set) and headersSent. Expand existing test for end to include a check for closed. Add a new test for destroy. Refs: nodejs#14985

apapirovski force-pushed the patch-test-http2-serverresponse-coverage branch from d242bb1 to ad35687 Compare September 1, 2017 19:20

mcollina reviewed Sep 2, 2017

View reviewed changes

Fix to test both req & res error listeners

12dfd8f

apapirovski force-pushed the patch-test-http2-serverresponse-coverage branch from 7605465 to 12dfd8f Compare September 2, 2017 12:18

mcollina approved these changes Sep 2, 2017

View reviewed changes

apapirovski force-pushed the patch-test-http2-serverresponse-coverage branch from a71def2 to 12dfd8f Compare September 2, 2017 21:31

BridgeAR closed this Sep 3, 2017

apapirovski deleted the patch-test-http2-serverresponse-coverage branch September 4, 2017 14:37

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

kanongil mentioned this pull request Sep 26, 2020

HTTP/2 ServerResponse.destroy() has the same effect as ServerResponse.end() #35302

Open

adamlaska mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities adamlaska/node#67

Open

snyk-bot mentioned this pull request Apr 27, 2023

[Snyk] Fix for 20 vulnerabilities aliscco/alisco-node#253

Open

This was referenced Apr 28, 2023

[Snyk] Fix for 16 vulnerabilities aliscco/alisco-node#304

Open

[Snyk] Fix for 18 vulnerabilities aliscco/alisco-node#310

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Fix for 20 vulnerabilities aliscco/alisco-node#314

Open

This was referenced Apr 28, 2023

[Snyk] Fix for 9 vulnerabilities aliscco/alisco-node#330

Open

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#334

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Fix for 114 vulnerabilities aliscco/alisco-node#338

Open

aliscco mentioned this pull request Apr 28, 2023

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#342

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: increase Http2ServerResponse test coverage #15074

test: increase Http2ServerResponse test coverage #15074

apapirovski commented Aug 29, 2017

benjamingr commented Aug 31, 2017

benjamingr commented Sep 1, 2017

gibfahn commented Sep 1, 2017

benjamingr commented Sep 1, 2017

gibfahn commented Sep 1, 2017 •

edited

Loading

apapirovski commented Sep 1, 2017

mcollina Sep 1, 2017

apapirovski Sep 1, 2017

apapirovski Sep 1, 2017 •

edited

Loading

mcollina Sep 1, 2017

apapirovski Sep 1, 2017

mcollina commented Sep 1, 2017

jasnell commented Sep 1, 2017

refack commented Sep 1, 2017

apapirovski commented Sep 1, 2017 •

edited

Loading

refack commented Sep 1, 2017

mcollina Sep 2, 2017

apapirovski commented Sep 2, 2017 •

edited

Loading

mcollina left a comment

mcollina commented Sep 3, 2017

BridgeAR commented Sep 3, 2017

test: increase Http2ServerResponse test coverage #15074

test: increase Http2ServerResponse test coverage #15074

Conversation

apapirovski commented Aug 29, 2017

Checklist

Affected core subsystem(s)

benjamingr commented Aug 31, 2017

benjamingr commented Sep 1, 2017

gibfahn commented Sep 1, 2017

benjamingr commented Sep 1, 2017

gibfahn commented Sep 1, 2017 • edited Loading

apapirovski commented Sep 1, 2017

mcollina Sep 1, 2017

Choose a reason for hiding this comment

apapirovski Sep 1, 2017

Choose a reason for hiding this comment

apapirovski Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

mcollina Sep 1, 2017

Choose a reason for hiding this comment

apapirovski Sep 1, 2017

Choose a reason for hiding this comment

mcollina commented Sep 1, 2017

jasnell commented Sep 1, 2017

refack commented Sep 1, 2017

apapirovski commented Sep 1, 2017 • edited Loading

refack commented Sep 1, 2017

mcollina Sep 2, 2017

Choose a reason for hiding this comment

apapirovski commented Sep 2, 2017 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 3, 2017

BridgeAR commented Sep 3, 2017

gibfahn commented Sep 1, 2017 •

edited

Loading

apapirovski Sep 1, 2017 •

edited

Loading

apapirovski commented Sep 1, 2017 •

edited

Loading

apapirovski commented Sep 2, 2017 •

edited

Loading