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.

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

cluster: respect server.listen() backlog parameter set by workers (credit: @oyyd) #41623

Closed
wants to merge 3 commits into from
Closed

cluster: respect server.listen() backlog parameter set by workers (credit: @oyyd) #41623

wants to merge 3 commits into from

Conversation

@eladnava
Copy link
Contributor

@eladnava eladnava commented Jan 20, 2022

Re-creating the closed pull request #33827 to fix #4056, a bug where the backlog parameter passed in to server.listen(handle[, backlog][, callback]) from within a Node.js worker process isn't respected. Instead, the default value of 511 is always set and unable to be overriden.

More about the server.listen() backlog parameter:

All listen() methods can take a backlog parameter to specify the maximum length of the queue of pending connections. The actual length will be determined by the OS through sysctl settings such as tcp_max_syn_backlog and somaxconn on Linux. The default value of this parameter is 511 (not 512).

This PR and all commits were authored by @oyyd. I've updated from nodejs/master so it won't be behind any commits, and there were no conflicts.

The original PR was closed because a test was failing on Windows. I'd like the opportunity to fix that test. If you could please run Node.js Jenkins on the PR so I can look at the error log in case the test still fails, that would be greatly appreciated.

All thanks go to @oyyd for the original PR.

Checklist:

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 20, 2022

Review requested:

@eladnava eladnava mentioned this pull request Jan 20, 2022
Closed
@tniessen
Copy link
Member

@tniessen tniessen commented Jan 21, 2022

If you could please run Node.js Jenkins on the PR so I can look at the error log in case the test still fails, that would be greatly appreciated.

Thanks for the contribution, I'll start CI.

This PR and all commits were authored by @oyyd.

FYI, if these changes were taken from @oyyd's PR as they are, @oyyd should remain the commit author. Once you start modifying their changes, add a second author using Co-authored-by.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 21, 2022

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

@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Jan 21, 2022 •
edited

Hi @tniessen,
Thanks so much for your help and guidance.

I tried keeping @oyyd's commit, but unfortunately the naming convention in that commit no longer adheres to the node commit naming conventions/guidelines (as it starts with "fix" which is no longer allowed), causing the relevant test to fail.

I've gone forth and modified the commit to adhere to the naming conventions, and to include Co-authored-by as recommended.

I'll review the CI results and resolve any issues in the upcoming days. Thanks again for your help!

@oyyd
Copy link
Member

@oyyd oyyd commented Jan 21, 2022 •
edited

@eladnava Thanks for working on this.

I've gone forth and modified the commit to adhere to the naming conventions, and to include Co-authored-by as recommended.

That's ok to me.

The test on windows still fails.

@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Jan 21, 2022 •
edited

Thanks @oyyd for the support!

I believe this commit will fix the failing test on the Windows build by forcing round-robin cluster scheduling on Windows, where the default is cluster.SCHED_NONE.

@tniessen, can I please request CI again?

@github-actions github-actions bot mentioned this pull request Jan 22, 2022
Open
17 tasks
@github-actions github-actions bot mentioned this pull request Jan 23, 2022
Open
13 tasks
@github-actions github-actions bot mentioned this pull request Jan 24, 2022
Open
22 tasks
@github-actions github-actions bot mentioned this pull request Jan 25, 2022
Open
28 tasks
@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Jan 25, 2022

Hi @tniessen,
Sorry to bug, may I please request updated CI test results so I can confirm the test on the Windows build now passes with the change made?

Thanks for your help!

@tniessen
Copy link
Member

@tniessen tniessen commented Jan 25, 2022

For sure, sorry that I'm only getting to this now.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 26, 2022

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

@github-actions github-actions bot mentioned this pull request Jan 26, 2022
Open
26 tasks
@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Jan 26, 2022

Hi @tniessen and @oyyd,
Thanks so much! All CI tests pass now. Anything else needed to merge? :)

tniessen
tniessen reviewed Jan 26, 2022
View changes
doc/api/net.md Outdated Show resolved Hide resolved
@eladnava @tniessen
cluster: respect server.listen() backlog parameter set by workers (fi…
7e083a3 
…x typo)

Co-authored-by: Tobias Nießen <tniessen@tnie.de>
@tniessen
Copy link
Member

@tniessen tniessen commented Jan 26, 2022

cc'ing those who reviewed the original PR: @mcollina @jasnell @addaleax

@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Jan 26, 2022

Thanks @tniessen.

Please note that an unrelated test is now timing out, most likely running that specific GitHub check again (test-asan) will resolve the issue.

@tniessen
Copy link
Member

@tniessen tniessen commented Jan 26, 2022

That unfortunately happens a lot. Please ping me again if it fails again.

oyyd
oyyd approved these changes Jan 29, 2022
View changes
@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Feb 3, 2022

@oyyd thanks for approving.
@tniessen any other reviewers needed to merge?

mcollina
mcollina approved these changes Feb 5, 2022
View changes
Copy link
Member

@mcollina mcollina left a comment

lgtm

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 5, 2022

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

@eladnava
Copy link
Contributor Author

@eladnava eladnava commented Feb 10, 2022

Thanks @mcollina! 😄
Anything else needed to merge, @tniessen?

@tniessen
Copy link
Member

@tniessen tniessen commented Feb 10, 2022

Merging. Thanks for your contribution!

@nodejs-github-bot

This comment has been minimized.

Sign in to view
tniessen added a commit that referenced this issue Feb 10, 2022
@eladnava @oyyd @tniessen
cluster: respect listen backlog set by workers
217366e 
PR-URL: #41623
Co-authored-by: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@tniessen
Copy link
Member

@tniessen tniessen commented Feb 10, 2022

Landed in 217366e.

@tniessen tniessen closed this Feb 10, 2022
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
@eladnava @oyyd @bengl
cluster: respect listen backlog set by workers
c41e402 
PR-URL: nodejs#41623
Co-authored-by: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
@eladnava @oyyd @bengl
cluster: respect listen backlog set by workers
a337c6e 
PR-URL: nodejs#41623
Co-authored-by: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@bengl bengl mentioned this pull request Feb 21, 2022
Merged
@eladnava eladnava mentioned this pull request Feb 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen

@mcollina mcollina

@oyyd oyyd

Assignees
No one assigned
Labels
author ready cluster commit-queue-failed commit-queue-squash needs-ci net
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
6 participants
@eladnava @nodejs-github-bot @tniessen @oyyd @mcollina @Ayase-252