Skip to content

net: fix net.Server keepalive and noDelay - #43497

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:fix_net_keepalive
Jun 22, 2022
Merged

net: fix net.Server keepalive and noDelay#43497
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:fix_net_keepalive

Conversation

@theanarkh

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

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented Jun 19, 2022

Copy link
Copy Markdown
Collaborator

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

mscdex commented Jun 19, 2022

Copy link
Copy Markdown
Contributor

This needs a test.

@theanarkh

theanarkh commented Jun 19, 2022

Copy link
Copy Markdown
ContributorAuthor

This needs a test.

Thanks, done !

violethaze74
violethaze74 reviewed Jun 19, 2022
View reviewed changes

@violethaze74violethaze74 left a comment

Copy link
Copy Markdown

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

@ShogunPandaShogunPanda left a comment

Copy link
Copy Markdown
Contributor

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

theanarkh commented Jun 22, 2022

Copy link
Copy Markdown
ContributorAuthor

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

mcollina
mcollina approved these changes Jun 22, 2022
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

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-bot
nodejs-github-bot merged commit dbe5874 into nodejs:mainJun 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

Landed in dbe5874

This was referenced Jun 23, 2022
Open
Open
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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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