test: http2 client operations after destroy #16094

trivikr · 2017-10-09T00:53:25Z

This code change tests client operations after calling client.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, http2

watilde · 2017-10-10T15:31:54Z

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

apapirovski · 2017-10-11T00:41:15Z

Looks like this failed on ubuntu1604-arm64, see https://ci.nodejs.org/job/node-test-commit-arm/nodes=ubuntu1604-arm64/12673/tapResults/

apapirovski · 2017-10-11T00:44:34Z

test/parallel/test-http2-client-destroy.js

+        common.expectsError(() => client.priority(req, {}), sessionError);
+        common.expectsError(() => client.shutdown(), sessionError);
+        common.expectsError(() => client.rstStream(req), sessionError);
+      }, 10);


This should be common.platformTimeout(10) — I would also strongly recommend trying to find a way to avoid the timeout. In general we try to avoid using timeout unless it's part of what's being tested.

(I haven't looked at the code so I don't know if it's possible. Perhaps there's an event emitted later on that can be piggy backed on? Or would setImmediate work here, since the other setImmediate is probably enqueued first?)

The destroy method calls finishSessionDestroy in a setImmediate at

node/lib/internal/http2/core.js

Line 974 in e399abd

setImmediate(finishSessionDestroy, this, socket);

While writing tests, I've to wait for finishSessionDestroy to complete and setTimeout is called after setImmediate (stackoverflow discussion)

I replaced setTimeout with setImmediate and the test was successful. I'l updated the commit.

joyeecheung · 2017-10-13T12:16:08Z

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

trivikr · 2017-10-13T17:32:36Z

CI build is successful, this change should be good to merge.

Trott · 2017-10-13T17:47:50Z

CI build is successful, this change should be good to merge.

I'm afraid that's incorrect. There's something wrong with CI and no tests were actually run. It will need to be re-run.

joyeecheung · 2017-10-14T04:33:12Z

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

trivikr · 2017-10-14T21:44:06Z

The CI is failing on slower platforms https://ci.nodejs.org/job/node-test-commit/13123//console
I've updated commit using setTimeout with common.platformTimeout() instead of setImmediate, as recommended in the documentation

apapirovski · 2017-10-14T21:49:29Z

While node-test-commit-arm-fanned did fail, I don't see any failures that are caused by this PR. Could someone else double-check please?

Trott · 2017-10-15T02:36:12Z

test/parallel/test-http2-client-destroy.js

+        common.expectsError(() => client.priority(req, {}), sessionError);
+        common.expectsError(() => client.shutdown(), sessionError);
+        common.expectsError(() => client.rstStream(req), sessionError);
+      }, common.platformTimeout(100));


Arbitrary timeouts like this are a sign of an unreliable test more often than not. Can we find another way to solve whatever problem it is supposed to solve? Listening for events or even an unfortunate setInterval() for polling would be preferable IMO. If the test absolutely must use common.platformTimeout(), chances are that it should be moved to sequential.

Here's this chunk of code rewritten with a polling setInterval():

const interval = setInterval(() => { if (client.destroyed) { clearInterval(interval); common.expectsError(() => client.request(), sessionError); common.expectsError(() => client.settings({}), sessionError); common.expectsError(() => client.priority(req, {}), sessionError); common.expectsError(() => client.shutdown(), sessionError); common.expectsError(() => client.rstStream(req), sessionError); } }, 1);

Using that instead makes it reliable under load:

$ tools/test.py -j 96 --repeat 192 test/parallel/test-http2-client-destroy.js [00:12|% 100|+ 192|- 0]: Done $

For comparison, here's what happens with the current version of this PR (using setTimeout() and common.platformTimeout():

$ tools/test.py -j 96 --repeat 192 test/parallel/test-http2-client-destroy.js === release test-http2-client-destroy === Path: parallel/test-http2-client-destroy (node:22883) ExperimentalWarning: The http2 module is an experimental API. assert.js:42 throw new errors.AssertionError({ ^ AssertionError [ERR_ASSERTION]: Missing expected exception. at innerThrows (assert.js:186:7) at Function.throws (assert.js:205:3) at Object.expectsError (/Users/trott/io.js/test/common/index.js:746:12) at Timeout.setTimeout [as _onTimeout] (/Users/trott/io.js/test/parallel/test-http2-client-destroy.js:105:16) at ontimeout (timers.js:474:11) at tryOnTimeout (timers.js:298:5) at Timer.listOnTimeout (timers.js:258:5) Command: out/Release/node /Users/trott/io.js/test/parallel/test-http2-client-destroy.js === release test-http2-client-destroy === Path: parallel/test-http2-client-destroy (node:22846) ExperimentalWarning: The http2 module is an experimental API. assert.js:42 throw new errors.AssertionError({ ^ AssertionError [ERR_ASSERTION]: Missing expected exception. at innerThrows (assert.js:186:7) at Function.throws (assert.js:205:3) at Object.expectsError (/Users/trott/io.js/test/common/index.js:746:12) at Timeout.setTimeout [as _onTimeout] (/Users/trott/io.js/test/parallel/test-http2-client-destroy.js:105:16) at ontimeout (timers.js:474:11) at tryOnTimeout (timers.js:298:5) at Timer.listOnTimeout (timers.js:258:5) Command: out/Release/node /Users/trott/io.js/test/parallel/test-http2-client-destroy.js [00:11|% 100|+ 190|- 2]: Done $

As far as I can tell, this never fails with setImmediate. (If setImmediate fails then there's a bug in http2 so it's strictly the correct choice here.)

Machine:nodejs apapirovski$ tools/test.py -j 96 --repeat 960 test/parallel/test-http2-client-destroy.js [00:30|% 100|+ 960|- 0]: Done

I don't really understand why it was changed from that to setTimeout when the previous CI ran successfully.

I too am also seeing 100% success with setImmediate(). Putting it back to that is probably the most elegant solution. CI failures seem unrelated to this PR.

(Speaking of Ci challenges: Anybody good with Jenkins and/or ansible and/or systems administration would probably make a great addition to the Build WG. If that sounds like you and you are a known quantity around these parts, maybe nominate yourself with an issue in https://github.com/nodejs/build? There are some talented folks on that team, but we probably could use many more active members.)

Switched to setImmediate() in the new commit
@Trott Can you create a new issue for CI challenges and reference this PR?

Refs: nodejs#14985

setImmediate() has been restored, thanks

Trott · 2017-10-15T03:44:16Z

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

PR-URL: #16094 Ref: #14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

jasnell · 2017-10-15T22:25:37Z

Landed in 68df587

PR-URL: #16094 Ref: #14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

PR-URL: nodejs/node#16094 Ref: nodejs/node#14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

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

hiroppy added the http2 Issues or PRs related to the http2 subsystem. label Oct 9, 2017

jasnell approved these changes Oct 10, 2017

View reviewed changes

watilde approved these changes Oct 10, 2017

View reviewed changes

apapirovski reviewed Oct 11, 2017

View reviewed changes

trivikr force-pushed the test-http2-client-destroy branch from 5149496 to 25b5f86 Compare October 11, 2017 04:27

trivikr force-pushed the test-http2-client-destroy branch from 25b5f86 to 3fd6b13 Compare October 14, 2017 21:41

Trott previously requested changes Oct 15, 2017

View reviewed changes

test: http2 client operations after destroy

d7e0ae4

Refs: nodejs#14985

trivikr force-pushed the test-http2-client-destroy branch from 3fd6b13 to d7e0ae4 Compare October 15, 2017 03:21

jasnell pushed a commit that referenced this pull request Oct 15, 2017

test: http2 client operations after destroy

68df587

PR-URL: #16094 Ref: #14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

jasnell closed this Oct 15, 2017

trivikr deleted the test-http2-client-destroy branch October 15, 2017 22:34

targos pushed a commit that referenced this pull request Oct 18, 2017

test: http2 client operations after destroy

8a172cb

PR-URL: #16094 Ref: #14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017

test: http2 client operations after destroy

3abaa5d

PR-URL: nodejs/node#16094 Ref: nodejs/node#14985 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

MarcelRaschke mentioned this pull request Aug 19, 2022

[Snyk] Upgrade eslint from 7.32.0 to 8.20.0 MarcelRaschke/node#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: http2 client operations after destroy #16094

test: http2 client operations after destroy #16094

trivikr commented Oct 9, 2017

watilde commented Oct 10, 2017

apapirovski commented Oct 11, 2017

apapirovski Oct 11, 2017 •

edited

Loading

trivikr Oct 11, 2017

joyeecheung commented Oct 13, 2017

trivikr commented Oct 13, 2017

Trott commented Oct 13, 2017

joyeecheung commented Oct 14, 2017

trivikr commented Oct 14, 2017

apapirovski commented Oct 14, 2017

Trott Oct 15, 2017 •

edited

Loading

Trott Oct 15, 2017 •

edited

Loading

apapirovski Oct 15, 2017 •

edited

Loading

Trott Oct 15, 2017

trivikr Oct 15, 2017

Trott commented Oct 15, 2017

jasnell commented Oct 15, 2017

test: http2 client operations after destroy #16094

test: http2 client operations after destroy #16094

Conversation

trivikr commented Oct 9, 2017

Checklist

Affected core subsystem(s)

watilde commented Oct 10, 2017

apapirovski commented Oct 11, 2017

apapirovski Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

trivikr Oct 11, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 13, 2017

trivikr commented Oct 13, 2017

Trott commented Oct 13, 2017

joyeecheung commented Oct 14, 2017

trivikr commented Oct 14, 2017

apapirovski commented Oct 14, 2017

Trott Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

apapirovski Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Oct 15, 2017

Choose a reason for hiding this comment

trivikr Oct 15, 2017

Choose a reason for hiding this comment

Trott commented Oct 15, 2017

jasnell commented Oct 15, 2017

apapirovski Oct 11, 2017 •

edited

Loading

Trott Oct 15, 2017 •

edited

Loading

Trott Oct 15, 2017 •

edited

Loading

apapirovski Oct 15, 2017 •

edited

Loading