Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: only set keep-alive when not exists #35138

Closed
wants to merge 1 commit into from
Closed

fix: only set keep-alive when not exists #35138

wants to merge 1 commit into from

Conversation

atian25
Copy link
Contributor

@atian25 atian25 commented Sep 10, 2020

  • only set Keep-Alive once
  • set Keep-Alive with correct ms

fix: #34561 (comment)

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

@atian25 atian25 requested a review from a team as a code owner September 10, 2020 00:39
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 10, 2020
@atian25
Copy link
Contributor Author

atian25 commented Sep 10, 2020

sorry, but it seems there's a bug of probot pull auto delete my branch for #35096

so had to raise another PR.

cc @ronag @lpinca @rickyes

@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 10, 2020 •
edited by rickyes
Loading

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

@bnoordhuis bnoordhuis mentioned this pull request Sep 10, 2020
Closed
@rickyes rickyes added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 11, 2020
rickyes pushed a commit that referenced this pull request Sep 12, 2020
@atian25 @rickyes
http: only set keep-alive when not exists
a63e121 
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@rickyes
Copy link
Contributor

rickyes commented Sep 12, 2020

Landed in a63e121

@atian25 congratulations on your first commit to Node.js! 🎉

@rickyes rickyes closed this Sep 12, 2020
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
@atian25 @ruyadorno
http: only set keep-alive when not exists
8ea2853 
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
Merged
4 tasks
@rickyes rickyes mentioned this pull request Oct 10, 2020
Closed
4 tasks
atian25 added a commit to atian25/node that referenced this pull request Oct 13, 2020
@atian25
http: only set keep-alive when not exists
5732f67 
Backport-PR-URL: nodejs#35138
@atian25 atian25 mentioned this pull request Oct 13, 2020
Closed
4 tasks
fengmk2
fengmk2 reviewed Oct 19, 2020
View reviewed changes
lib/_http_outgoing.js
if (this._keepAliveTimeout) {
const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;
if (this._keepAliveTimeout && this._defaultKeepAlive) {
const timeoutSeconds = MathFloor(this._keepAliveTimeout / 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@atian25 @MylesBorins
http: only set keep-alive when not exists
ff3875d 
Backport-PR-URL: #35623
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@atian25 @MylesBorins
http: only set keep-alive when not exists
d477e2e 
Backport-PR-URL: #35623
PR-URL: #35138
Fixes: #34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@rickyes rickyes mentioned this pull request Nov 27, 2020
Closed
2 tasks
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@atian25 @joesepi
http: only set keep-alive when not exists
1efbf64 
PR-URL: nodejs#35138
Fixes: nodejs#34561
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengmk2 fengmk2 fengmk2 left review comments

@rickyes rickyes rickyes approved these changes

@trivikr trivikr trivikr approved these changes

@himself65 himself65 himself65 approved these changes

@ronag ronag ronag approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@atian25 @nodejs-github-bot @rickyes @fengmk2 @lpinca @ronag @himself65 @trivikr