test: defer abort with setTimeout #15520

mcollina · 2017-09-21T06:59:45Z

Ref: #15404
Fixes: #15505

Checklist

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

Affected core subsystem(s)

test, http

mcollina · 2017-09-21T07:00:20Z

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

mcollina · 2017-09-21T07:44:18Z

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

mcollina · 2017-09-21T08:00:23Z

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

lpinca · 2017-09-21T08:47:15Z

test/parallel/test-http-writable-true-after-close.js

 get(`http://127.0.0.1:${internal.address().port}`, common.mustCall((inner) => {
- res.on('close', common.mustCall(() => {
- assert.strictEqual(res.writable, true);
- }));
 inner.pipe(res);


Is it safe to call external.abort() and internalRes.end('Hello World\n'); here or does this invalidate the test?
This would remove the setTimeout dependency.

mcollina · 2017-09-21T08:50:26Z

@lpinca the problem is that this test keeps failing only on Centos and I do not know why :(

lpinca · 2017-09-21T09:02:38Z

Yes that's weird, it looks like res.on('close', ...) is not emitted but I don't know why.

cjihrig · 2017-09-21T12:40:22Z

While you're here, could you move the stuff from // Proxy server until the end of the file into the internal server's listen() callback?

mcollina · 2017-09-21T15:50:59Z

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

mcollina · 2017-09-21T16:06:59Z

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

mcollina · 2017-09-22T13:35:49Z

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

mcollina · 2017-09-22T15:03:16Z

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

mcollina · 2017-09-22T15:22:39Z

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

mcollina · 2017-09-22T15:59:02Z

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

BridgeAR · 2017-09-25T04:33:15Z

@mcollina we could also mark the test as being flaky on centos?

mcollina · 2017-09-25T07:06:29Z

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

BridgeAR · 2017-09-25T14:00:18Z

test/parallel/test-http-writable-true-after-close.js

+ // everywhere else, 'close' is emitted
+ res.on('close', () => {
+ assert.strictEqual(res.writable, true);
+ });


This will test less than before, since the mustCall is not in place anymore. It might be worth to check that either finish or close will be called at least once? And I am somewhat surprised that our events are platform dependent. Would you be so kind and add a comment why that is the case?

I have no idea on why it's the case, that's the major problem of this PR.

I'll see if I can get another solution as well.

I'll add another check that verifies that at least one of them is called.

mcollina · 2017-09-25T16:16:29Z

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

mcollina · 2017-09-25T17:00:27Z

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

mcollina · 2017-09-26T09:58:15Z

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

This should be green, and it can land as it finshes.

@jasnell @lpinca @cjihrig can you review it again so we can land?

lpinca · 2017-09-26T10:24:36Z

test/parallel/test-http-writable-true-after-close.js

@@ -1,4 +1,4 @@
-'use strict';
+use strict';


Missing '

lpinca · 2017-09-26T10:27:05Z

test/parallel/test-http-writable-true-after-close.js

+ res.on('finish', console.log.bind(console, 'finish emitted'));
+ res.on('close', console.log.bind(console, 'close emitted'));
+
+ // on CentOS, 'finish' is emitted


I'd point out that this only happens on CentOS 5.

lpinca · 2017-09-26T10:32:59Z

test/parallel/test-http-writable-true-after-close.js

- }));
+ })
+
+ res.on('finish', console.log.bind(console, 'finish emitted'));


I guess these are only for debugging. Do we want to keep them?

mcollina · 2017-09-26T10:56:07Z

@lpinca thanks, updated!

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

lpinca · 2017-09-26T11:17:04Z

test/parallel/test-http-writable-true-after-close.js

 assert.strictEqual(res.writable, true);
- }));
+ })


Missing semicolon.

lpinca · 2017-09-26T11:17:13Z

test/parallel/test-http-writable-true-after-close.js

+ // on CentOS 5, 'finish' is emitted
+ res.on('finish', listener);
+ // everywhere else, 'close' is emitted
+ res.on('close', listener)


cjihrig · 2017-09-26T13:12:21Z

test/parallel/test-http-writable-true-after-close.js

+ res.writeHead(200);
+ setImmediate(common.mustCall(() => {
+ external.abort();
+ external.socket.on('close', () => {


I guess this isn't needed after all.

cjihrig · 2017-09-26T13:13:16Z

test/parallel/test-http-writable-true-after-close.js

 // Proxy server
 const server = createServer(common.mustCall((req, res) => {
 get(`http://127.0.0.1:${internal.address().port}`, common.mustCall((inner) => {
- res.on('close', common.mustCall(() => {
+ const listener = common.mustCall(() => {


Is this called more than once on any platforms? Removing the listeners kind of defeats the purpose of wrapping it in common.mustCall().

I guess the point is to ensure that the listener is actually called.

Yes, but common.mustCall() also enforces the number of times the function is called. Removing the listeners means we lose that checking.

I think either 'close' or 'finish' should be emitted so it doesn't matter as long as at least one is emitted.

That's why I asked:

Is this called more than once on any platforms?

We shouldn't remove the listeners unless we really need to.

mcollina · 2017-09-27T11:26:35Z

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

Ref: nodejs#15404 Fixes: nodejs#15505

mcollina · 2017-09-27T11:28:20Z

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

@cjihrig @lpinca updated

lpinca · 2017-09-27T12:37:03Z

Still LGTM.

mcollina · 2017-09-27T13:09:21Z

Landed as 1fe9b53

Ref: #15404 Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: nodejs#15404 Fixes: nodejs#15505 PR-URL: nodejs#15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: #15404 Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: nodejs/node#15404 Fixes: nodejs/node#15505 PR-URL: nodejs/node#15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ref: #15404 Fixes: #15505 PR-URL: #15520 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

mscdex added the http Issues or PRs related to the http subsystem. label Sep 21, 2017

mcollina force-pushed the fix-flaky-http-test branch from ec53a15 to 71f0b14 Compare September 21, 2017 07:43

mcollina force-pushed the fix-flaky-http-test branch from 71f0b14 to cfaf6bf Compare September 21, 2017 08:00

lpinca reviewed Sep 21, 2017

View reviewed changes

jasnell approved these changes Sep 21, 2017

View reviewed changes

mcollina force-pushed the fix-flaky-http-test branch from cfaf6bf to 8772804 Compare September 21, 2017 15:49

mcollina force-pushed the fix-flaky-http-test branch from 8772804 to a2ba6b8 Compare September 21, 2017 16:06

mcollina force-pushed the fix-flaky-http-test branch from a2ba6b8 to a4c7be8 Compare September 22, 2017 13:34

mcollina force-pushed the fix-flaky-http-test branch from a4c7be8 to 9b36dfa Compare September 22, 2017 15:02

mcollina force-pushed the fix-flaky-http-test branch from 9b36dfa to 643f8a8 Compare September 22, 2017 15:22

mcollina force-pushed the fix-flaky-http-test branch from 643f8a8 to 830dda8 Compare September 22, 2017 15:50

mcollina force-pushed the fix-flaky-http-test branch from 830dda8 to d349345 Compare September 25, 2017 07:04

BridgeAR reviewed Sep 25, 2017

View reviewed changes

mcollina force-pushed the fix-flaky-http-test branch from d349345 to 717a67a Compare September 25, 2017 16:12

mcollina force-pushed the fix-flaky-http-test branch from 061f7a9 to f2061d8 Compare September 26, 2017 09:54

lpinca reviewed Sep 26, 2017

View reviewed changes

test/parallel/test-http-writable-true-after-close.js Outdated

@@ -1,4 +1,4 @@

'use strict';

use strict';

Copy link

Member

lpinca Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing '

lpinca reviewed Sep 26, 2017

View reviewed changes

mcollina force-pushed the fix-flaky-http-test branch from f2061d8 to 7d6a0aa Compare September 26, 2017 10:54

lpinca approved these changes Sep 26, 2017

View reviewed changes

cjihrig reviewed Sep 26, 2017

View reviewed changes

mcollina force-pushed the fix-flaky-http-test branch from 7d6a0aa to adf649a Compare September 27, 2017 11:25

test: fix http-writable-true-after-close flakyness

dfd4c63

Ref: nodejs#15404 Fixes: nodejs#15505

mcollina force-pushed the fix-flaky-http-test branch from adf649a to dfd4c63 Compare September 27, 2017 11:27

cjihrig approved these changes Sep 27, 2017

View reviewed changes

mcollina closed this Sep 27, 2017

mcollina deleted the fix-flaky-http-test branch September 27, 2017 13:09

Trott mentioned this pull request Sep 30, 2017

CentOS 5 nodejs/build#898

Closed

Trott mentioned this pull request Sep 30, 2017

investigate flaky parallel/test-http-writable-true-after-close on macOS #15700

Closed

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Upgrade eslint from 7.32.0 to 8.7.0 Iceymann18777/node#4

Open

test: defer abort with setTimeout #15520

test: defer abort with setTimeout #15520

Conversation

mcollina commented Sep 21, 2017

Checklist

Affected core subsystem(s)

mcollina commented Sep 21, 2017

mcollina commented Sep 21, 2017

mcollina commented Sep 21, 2017

Choose a reason for hiding this comment

mcollina commented Sep 21, 2017

lpinca commented Sep 21, 2017

cjihrig commented Sep 21, 2017

mcollina commented Sep 21, 2017

mcollina commented Sep 21, 2017

mcollina commented Sep 22, 2017

mcollina commented Sep 22, 2017

mcollina commented Sep 22, 2017

mcollina commented Sep 22, 2017

BridgeAR commented Sep 25, 2017

mcollina commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Sep 25, 2017

mcollina commented Sep 25, 2017

mcollina commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Sep 27, 2017

mcollina commented Sep 27, 2017

lpinca commented Sep 27, 2017

mcollina commented Sep 27, 2017

lpinca Sep 26, 2017 •

edited

Loading