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

Document NODE_MANY_ACCEPTS environment variable. #18391

Closed
martinheidegger opened this issue Jan 26, 2018 · 8 comments
Closed

Document NODE_MANY_ACCEPTS environment variable. #18391

martinheidegger opened this issue Jan 26, 2018 · 8 comments
Labels
doc Issues and PRs related to the documentations.

Comments

@martinheidegger
Copy link

While investigating a problem with electron/dat I found the NODE_MANY_ACCEPTS environment variable exclusively used in the win32 context.

node/lib/net.js

Lines 1757 to 1760 in 02fef8a

if (simultaneousAccepts === undefined) {
simultaneousAccepts = (process.env.NODE_MANY_ACCEPTS &&
process.env.NODE_MANY_ACCEPTS !== '0');
}

However I was not able to find any documentation of why this is necessary or what it does in the environment variable doc.

@bnoordhuis
Copy link
Member

It's explained in the libuv documentation:

.. c:function:: int uv_tcp_simultaneous_accepts(uv_tcp_t* handle, int enable)

    Enable / disable simultaneous asynchronous accept requests that are
    queued by the operating system when listening for new TCP connections.

    This setting is used to tune a TCP server for the desired performance.
    Having simultaneous accepts can significantly improve the rate of accepting
    connections (which is why it is enabled by default) but may lead to uneven
    load distribution in multi-process setups.

NODE_MANY_ACCEPTS only has an effect on the child_process and cluster modules.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Jan 26, 2018
@Fishrock123
Copy link
Contributor

Sounds like this should be documented in https://github.com/nodejs/node/blob/master/doc/api/cli.md#environment-variables and https://github.com/nodejs/node/blob/master/doc/node.1

@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Jan 26, 2018
@bnoordhuis
Copy link
Member

IIRC, we added it as a debugging escape hatch but it's never been used and it's probably just dead code at this point. It's better to remove it than to document it.

@juggernaut451
Copy link
Contributor

juggernaut451 commented Jan 26, 2018

I would like to take this up. @Fishrock123 @bnoordhuis So it has to be documented or to be removed?

@Fishrock123 Fishrock123 removed the good first issue Issues that are suitable for first-time contributors. label Jan 26, 2018
@Fishrock123
Copy link
Contributor

I'm not sure, I guess we should investigate if it still works / is necessary or useful.

@martinheidegger
Copy link
Author

The only two places it seems to be used are:

// Update simultaneous accepts on Windows
if (process.platform === 'win32') {
handle._simultaneousAccepts = false;
net._setSimultaneousAccepts(handle);
}

and

// Update simultaneous accepts on Windows
if (obj.simultaneousAccepts) {
net._setSimultaneousAccepts(handle);
}

the c docs mention that:

... having simultaneous accepts can significantly improve the rate of accepting connections (which is why it is enabled by default)

It seems thought that the default in Node.js is to have it disabled by default.

@bnoordhuis
Copy link
Member

It seems thought that the default in Node.js is to have it disabled by default.

I have reply deja vu, I remember answering this... It's opt-out, i.e., on by default. The two calls you link to are the opt-outs.

We should probably just call handle.setSimultaneousAccepts(false) directly and get rid of the indirection.

@juggernaut451 Pull request welcome if you want to take this on. I'd make net._setSimultaneousAccepts print a deprecation warning but otherwise leave it unchanged for now.

jasnell added a commit to jasnell/node that referenced this issue Oct 25, 2018
This is an undocumented utility function that is of questionable
utility.

Fixes: nodejs#18391
@curtcurt871
Copy link

18391

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests

5 participants