Skip to content

net: fix net.Server keepalive and noDelay#43497

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
Merged
nodejs-github-bot merged 1 commit into from
Jun 22, 2022
Merged

net: fix net.Server keepalive and noDelay #43497

nodejs-github-bot merged 1 commit into from
Jun 22, 2022

Conversation

@theanarkh
Copy link
Contributor

@theanarkhtheanarkh commented Jun 19, 2022•
edited by VoltrexKeyva
Loading

The setXXX function should be called on clientHandle instead of server handle.

The code below can trigger the bug.

server.js

constnet=require('net');net.createServer({keepAlive: true,keepAliveInitialDelay: 1000},()=>{}).listen(8080);

client.js

constnet=require('net');net.createConnection({port:8080,});

the code above do not send keepalive packet after 1s.

  • 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

Affected subsystem: net

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 19, 2022

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Jun 19, 2022
@mscdex
Copy link
Contributor

mscdex commented Jun 19, 2022

This needs a test.

@theanarkh
Copy link
ContributorAuthor

theanarkh commented Jun 19, 2022

This needs a test.

Thanks, done !

violethaze74
violethaze74 reviewed Jun 19, 2022
View reviewed changes
Copy link

@violethaze74violethaze74 left a comment

Choose a reason for hiding this comment

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

theanarkh:fix_net_keepalive

ShogunPanda
ShogunPanda approved these changes Jun 19, 2022
View reviewed changes
Copy link
Contributor

@ShogunPandaShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for noticing it!

@theanarkh
Copy link
ContributorAuthor

theanarkh commented Jun 22, 2022

@mcollina Hi. Can you help review this PR ? Thanks!

mcollina
mcollina approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollinamcollina added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jun 22, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2022

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2022

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

@mcollinamcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 22, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-botnodejs-github-bot merged commit dbe5874 into nodejs:mainJun 22, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 22, 2022

Landed in dbe5874

This was referenced Jun 23, 2022
Open
Open
F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
@theanarkh@F3n67u
net: fix net.Server keepalive and noDelay
f1cd06c
PR-URL: nodejs#43497 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This was referenced Jun 25, 2022
Open
Open
Open
targos pushed a commit that referenced this pull request Jul 12, 2022
@theanarkh@targos
net: fix net.Server keepalive and noDelay
47a2522
PR-URL: #43497 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@targostargos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@theanarkh@targos
net: fix net.Server keepalive and noDelay
63d05b5
PR-URL: #43497 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@theanarkh@targos
net: fix net.Server keepalive and noDelay
e4a5bd1
PR-URL: #43497 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@targostargos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@theanarkh@guangwong
net: fix net.Server keepalive and noDelay
4e373b2
PR-URL: nodejs/node#43497 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@ShogunPandaShogunPandaShogunPanda approved these changes

+1 more reviewer

@violethaze74violethaze74violethaze74 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.netIssues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@theanarkh@nodejs-github-bot@mscdex@mcollina@ShogunPanda@violethaze74