GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: https requestTimeout 0 by ronag · Pull Request #35264 · nodejs/node

Conversation

@ronag

@ronag ronag commented Sep 18, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ronag
http: https requestTimeout 0
20d3ec5 
Fixes: nodejs#35261
@ronag
ronag requested a review from mcollina September 18, 2020 23:38
@ronag
ronag requested a review from a team as a code owner September 18, 2020 23:38
@nodejs-github-bot nodejs-github-bot added http Issues and PRs related to the http subsystem. https Issues and PRs related to the https subsystem. labels Sep 18, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
eba16c1
@ronag
ronag force-pushed the fix-35261 branch from f6bd76e to eba16c1 Compare September 18, 2020 23:40
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 18, 2020
@ronag

ronag commented Sep 18, 2020

Copy link
Copy Markdown
Member Author

Fast track?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 18, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Sep 19, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

my bad! +1 to fast-track

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented Sep 19, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
richardlau
richardlau approved these changes Sep 19, 2020
View reviewed changes
lpinca
lpinca reviewed Sep 19, 2020
View reviewed changes
Comment thread test/parallel/test-https-server-request-timeout.js Outdated
client.write('12345678901234567890\r\n\r\n');
}, common.platformTimeout(2000)).unref();

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

@lpinca lpinca Sep 19, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread test/parallel/test-https-server-request-timeout.js Outdated
});
let response = '';

client.on('data', common.mustCall((chunk) => {

@lpinca lpinca Sep 19, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-https-server-request-timeout.js Outdated
Comment on lines +59 to +61
setTimeout(() => {
client.write('12345678901234567890\r\n\r\n');
}, common.platformTimeout(2000)).unref();

@lpinca lpinca Sep 19, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-https-server-request-timeout.js Outdated
});

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

@lpinca lpinca Sep 19, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ronag

ronag commented Sep 19, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca

lpinca commented Sep 20, 2020

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Sep 20, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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)?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 20, 2020

Copy link
Copy Markdown
Member

Please do. I'll add a new test and fix the previous one after this land. We need to ship this asap.
ronag reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
865bdb3
@ronag
ronag force-pushed the fix-35261 branch from 98d902c to 865bdb3 Compare September 20, 2020 09:37
@ronag
ronag requested a review from lpinca September 20, 2020 09:37
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 20, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 20, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Sep 20, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented Sep 20, 2020

Copy link
Copy Markdown
Member

Is it ok now @lpinca?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 20, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup: remove unintended change
383a71a
@ronag ronag added v14.x request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 20, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 20, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Sep 20, 2020
View reviewed changes
Comment thread test/parallel/test-https-server-request-timeout.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag @lpinca
Update test/parallel/test-https-server-request-timeout.js
f58c725 
Co-authored-by: Luigi Pinca <[email protected]>
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 20, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 20, 2020
@nodejs-github-bot

nodejs-github-bot commented Sep 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BethGriggs
BethGriggs approved these changes Sep 21, 2020
View reviewed changes
Trott
Trott approved these changes Sep 21, 2020
View reviewed changes
@Trott

Trott commented Sep 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Landed in 3e06e646520533636e447d854c751668ad33fb37
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott closed this Sep 21, 2020
Trott pushed a commit that referenced this pull request Sep 21, 2020
@ronag @Trott
https: set requestTimeout default to 0
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]>
@Trott

Trott commented Sep 21, 2020

Copy link
Copy Markdown
Member

Force-pushed 5461794
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ruyadorno pushed a commit that referenced this pull request Sep 21, 2020
@ronag @ruyadorno
https: set requestTimeout default to 0
d8b5714 
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]>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
V14.12.0 proposal #35285
Merged
4 tasks done
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@ronag @joesepi
https: set requestTimeout default to 0
dc55983 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Trott Trott Trott approved these changes
@lpinca lpinca lpinca approved these changes
@richardlau richardlau richardlau approved these changes
@BethGriggs BethGriggs BethGriggs approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem. https Issues and PRs related to the https subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Websocket connections incorrectly timing out after 2 minutes in node 14.11? Possible regression

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@ronag @nodejs-github-bot @mcollina @lpinca @Trott @richardlau @BethGriggs
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.