http: https requestTimeout 0 #35264

ronag · 2020-09-18T23:38:59Z

requestTimeout should be 0 by default for https just like http. The current default may cause breaking regressions in user code.

Fixes: #35261

Checklist

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

Fixes: nodejs#35261

nodejs-github-bot · 2020-09-18T23:39:04Z

Review requested:

@nodejs/crypto
@nodejs/http
@nodejs/net

ronag · 2020-09-18T23:41:03Z

Fast track?

nodejs-github-bot · 2020-09-18T23:43:12Z

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

mcollina

lgtm

my bad! +1 to fast-track

mcollina · 2020-09-19T05:26:34Z

@nodejs/build this should hit v14 as soon as possible.

lpinca · 2020-09-19T18:29:33Z

test/parallel/test-https-server-request-timeout.js

+    client.write('12345678901234567890\r\n\r\n');
+  }, common.platformTimeout(2000)).unref();
+
+  const errOrEnd = common.mustCall(function(err) {


Suggested change

const errOrEnd = common.mustCall(function(err) {

const errOrEnd = common.mustCall(function() {

lpinca · 2020-09-19T18:54:08Z

test/parallel/test-https-server-request-timeout.js

+  });
+  let response = '';
+
+  client.on('data', common.mustCall((chunk) => {


How can you be sure that this is called only once?

lpinca · 2020-09-19T19:02:59Z

test/parallel/test-https-server-request-timeout.js

+  setTimeout(() => {
+    client.write('12345678901234567890\r\n\r\n');
+  }, common.platformTimeout(2000)).unref();


Suggested change

setTimeout(() => {

client.write('12345678901234567890\r\n\r\n');

}, common.platformTimeout(2000)).unref();

This does not seem to be very useful. The other peer already destroyed the socket when this write is attempted so this will result in a guaranteed error, no?

Also shouldn't all the writes be done after the 'connect' event?

lpinca · 2020-09-19T19:11:09Z

test/parallel/test-https-server-request-timeout.js

+  });
+
+  client.on('end', errOrEnd);
+  client.on('error', errOrEnd);


Suggested change

client.on('error', errOrEnd);

errOrEnd is returned by common.mustCall() so it can only be called once but is used a listener for two different events. This does not seem correct.

Under what circumstances is the 'error' event emitted here? I can only think of connection errors and a write after end error. I think the latter never occurs as the timer that would cause this is unrefed (the process exists before the write on line 60 occurs).

ronag · 2020-09-19T20:10:19Z

@lpina: The added test is just a copy paste from an existing test but with http replaced with https. This landed in 753f3b247a. Though we might want to fix those I think that's out of scope of this PR, i.e. I don't think we should stop this PR from fast tracking due to this.

@mcollina Please see @lpinca's comments.

lpinca · 2020-09-20T04:33:43Z

Though we might want to fix those I think that's out of scope of this PR.

I disagree. It's a copy paste of an existing test, yes but it's a new test so I see no reason to open a new PR to fix it. We can improve the test without a new PR and still fast track this.

ronag · 2020-09-20T07:35:58Z

I disagree. It's a copy paste of an existing test, yes but it's a new test so I see no reason to open a new PR to fix it. We can improve the test without a new PR and still fast track this.

I see. I don't have the answers to your comments. Sorting them out will probably take a while unless the original author has time. Should I remove most of the test and just add an assertion for the default value instead (which is what this PR is fixing)?

mcollina · 2020-09-20T09:27:51Z

Please do. I'll add a new test and fix the previous one after this land. We need to ship this asap.

nodejs-github-bot · 2020-09-20T09:42:11Z

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

mcollina

lgtm

mcollina · 2020-09-20T09:50:41Z

Is it ok now @lpinca?

mcollina · 2020-09-20T09:51:00Z

@nodejs/tsc this needs to be fast-tracked.

nodejs-github-bot · 2020-09-20T15:58:24Z

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

test/parallel/test-https-server-request-timeout.js

Co-authored-by: Luigi Pinca <[email protected]>

nodejs-github-bot · 2020-09-20T19:23:19Z

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

Trott · 2020-09-21T15:32:05Z

~~Landed in 3e06e646520533636e447d854c751668ad33fb37~~

Fixes: #35261 PR-URL: #35264 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Trott · 2020-09-21T15:33:16Z

Force-pushed 5461794

Fixes: #35261 PR-URL: #35264 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Fixes: nodejs#35261 PR-URL: nodejs#35264 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Rich Trott <[email protected]>

http: https requestTimeout 0

20d3ec5

Fixes: nodejs#35261

ronag requested a review from mcollina September 18, 2020 23:38

ronag requested a review from a team as a code owner September 18, 2020 23:38

nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Sep 18, 2020

fixup

eba16c1

ronag force-pushed the fix-35261 branch from f6bd76e to eba16c1 Compare September 18, 2020 23:40

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2020

mcollina approved these changes Sep 19, 2020

View reviewed changes

richardlau approved these changes Sep 19, 2020

View reviewed changes

lpinca reviewed Sep 19, 2020

View reviewed changes

fixup

865bdb3

ronag force-pushed the fix-35261 branch from 98d902c to 865bdb3 Compare September 20, 2020 09:37

ronag requested a review from lpinca September 20, 2020 09:37

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020

mcollina approved these changes Sep 20, 2020

View reviewed changes

fixup: remove unintended change

383a71a

ronag added v14.x request-ci Add this label to start a Jenkins CI on a PR. labels Sep 20, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020

lpinca approved these changes Sep 20, 2020

View reviewed changes

test/parallel/test-https-server-request-timeout.js Outdated Show resolved Hide resolved

Update test/parallel/test-https-server-request-timeout.js

f58c725

Co-authored-by: Luigi Pinca <[email protected]>

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020

BethGriggs approved these changes Sep 21, 2020

View reviewed changes

Trott approved these changes Sep 21, 2020

View reviewed changes

Trott closed this Sep 21, 2020

ruyadorno mentioned this pull request Sep 21, 2020

V14.12.0 proposal #35285

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: https requestTimeout 0 #35264

http: https requestTimeout 0 #35264

ronag commented Sep 18, 2020 •

edited

Loading

nodejs-github-bot commented Sep 18, 2020

ronag commented Sep 18, 2020

nodejs-github-bot commented Sep 18, 2020

mcollina left a comment

mcollina commented Sep 19, 2020

lpinca Sep 19, 2020

lpinca Sep 19, 2020

lpinca Sep 19, 2020

lpinca Sep 19, 2020

ronag commented Sep 19, 2020 •

edited

Loading

lpinca commented Sep 20, 2020

ronag commented Sep 20, 2020 •

edited

Loading

mcollina commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

mcollina left a comment

mcollina commented Sep 20, 2020

mcollina commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

Trott commented Sep 21, 2020 •

edited

Loading

Trott commented Sep 21, 2020

	const errOrEnd = common.mustCall(function(err) {
	const errOrEnd = common.mustCall(function() {

	setTimeout(() => {
	client.write('12345678901234567890\r\n\r\n');
	}, common.platformTimeout(2000)).unref();

http: https requestTimeout 0 #35264

http: https requestTimeout 0 #35264

Conversation

ronag commented Sep 18, 2020 • edited Loading

Checklist

nodejs-github-bot commented Sep 18, 2020

ronag commented Sep 18, 2020

nodejs-github-bot commented Sep 18, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 19, 2020

lpinca Sep 19, 2020

Choose a reason for hiding this comment

lpinca Sep 19, 2020

Choose a reason for hiding this comment

lpinca Sep 19, 2020

Choose a reason for hiding this comment

lpinca Sep 19, 2020

Choose a reason for hiding this comment

ronag commented Sep 19, 2020 • edited Loading

lpinca commented Sep 20, 2020

ronag commented Sep 20, 2020 • edited Loading

mcollina commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 20, 2020

mcollina commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

nodejs-github-bot commented Sep 20, 2020

Trott commented Sep 21, 2020 • edited Loading

Trott commented Sep 21, 2020

ronag commented Sep 18, 2020 •

edited

Loading

ronag commented Sep 19, 2020 •

edited

Loading

ronag commented Sep 20, 2020 •

edited

Loading

Trott commented Sep 21, 2020 •

edited

Loading