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

fix(cluster): respect the backlog from workers #33827

Closed
wants to merge 2 commits into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Jun 10, 2020

Encounter the same issue of #4056.

Currently, the master process would ignore backlog passed from worker processes and use the default value instead. This commit will respect the first backlog passed to the master process for a specific handle.

It seems hard to test backlog in a Node.js script. I use the script below and ss to check backlog. It's correct on my Linux(Ubuntu):

const net = require('net');
const cluster = require('cluster');
const path = require('path');

if (cluster.isMaster) {
  const workers = [];

  for (let i = 0; i < 2; i += 1) {
    const worker = cluster.fork();
    workers.push(worker);
  }
} else {
  net.createServer().listen(9230, 7); // ss -l
  net.createServer().listen(path.resolve(__dirname, './test.sock'), 7); // ss -x -l
}
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. net Issues and PRs related to the net subsystem. labels Jun 10, 2020
@addaleax
Copy link
Member

@oyyd Could you add a test for this?

@oyyd
Copy link
Contributor Author

oyyd commented Jun 12, 2020

@addaleax Have added a test monkey-patching the net module to ensure it works as expected.

@oyyd oyyd requested a review from a team as a code owner August 10, 2020 16:07
@oyyd oyyd requested a review from a team August 10, 2020 16:07
@oyyd oyyd force-pushed the cluster-backlog branch 2 times, most recently from 9262918 to b322936 Compare August 13, 2020 13:33
@nodejs-github-bot

This comment has been minimized.

@oyyd oyyd force-pushed the cluster-backlog branch from b322936 to e23a64b Compare August 14, 2020 06:59
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@oyyd oyyd force-pushed the cluster-backlog branch from e23a64b to 999fe1c Compare August 14, 2020 12:43
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

Currently, the master process would ignore `backlog` from worker
processes and use the default value instead. This commit will respect
the first `backlog` passed to the master process for a specific handle.

Refs: nodejs#4056
@oyyd oyyd force-pushed the cluster-backlog branch from 8e3e8d0 to 5e339ec Compare August 18, 2020 10:01
@oyyd
Copy link
Contributor Author

oyyd commented Aug 18, 2020

The included test failed on a windows environment of CI as both net.Server.prototype.listen and net.Server.prototype._listen2 don't get called. Need more investigation.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Nov 9, 2020

@oyyd do you plan to keep working on this?

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Nov 11, 2020

@oyyd Test is consistently failing on Windows. Can you take a look?

@aduh95 aduh95 added stalled Issues and PRs that are stalled. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 11, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@oyyd
Copy link
Contributor Author

oyyd commented Nov 11, 2020

I can't fix the failure on windows.. so that I'm going to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. net Issues and PRs related to the net subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants