node
node copied to clipboard
nodejs
server.maxConnections = 0 is treated as if its unset
server.maxConnections is an Integer, and so, I'd expect 0 to behave the same as -1; ie, drop all incoming conns.
(this is my first commit: I may have messed it up. I skimmed contributing.md but didn't pay much attention to it given this is a straight-forward fix... or, so I think)
Hello! First of all thanks for your PR. You haven't messed up anything, no worries. The only missing piece is a test to verify this change.
About the subject of the PR itself, I'm a bit conflicted. While you are right in saying that today setting server.maxConnections to 0 or -1 yields a completely different outcome, I'm unsure in what would be the right line of action.
Thinking about the original intent of the flag (and we're talking about Node.js 0.2.0 so it's pretty hard to verify this), I would say to treat 0 and -1 as "feature disabled" rather than "do not accept any connection", as for the latter you can just close the server. So in this case your change would just trim down to:
// ...
if (self.maxConnections > 0 && self._connections >= self.maxConnections) {
// ...
On the other hand, if we want a non strictly positive maxConnections to temporarily prevent the server to accept new connections without having to completely close the server, then your PR is absolutely correct. It will be a breaking change, but not a big deal.
@nodejs/tsc Any opinion on this?
Yeah, it'd be nice to have 0 and values less than 0 do the same thing (either disable the feature or drop all incoming conns). It is breaking change to do either so some might want to leave it as-is, which I think then needs to go up in the docs. I guess, we can look at how 0 or -ve Integer values in similar Node APIs are treated?
Will amend the code with tests once we reach consensus.
It does need docs and test updates tho.
I guess, the docs for behaviour changes are updated at the time of release? If not, let me know what needs to be added to the docs.
I've augmented 2 drop connection tests to account for server.maxConnections = 0.
(My PC isn't capable of building Node and running tests, and so I've enabled the auto-start-ci workflow on my github fork in the hope that it builds Node and runs these tests).
First of all, can you fix the lint problem outlined in the "Files changed tab"?
Second, you'll need to edit net.md and change the doc for maxConnections.
Before the option you'll find a little YAML changelog you need to edit as well. Use REPLACEME instead of a Node.js version.
Thanks.
I have no clue what how to proceed.
Second, you'll need to edit net.md and change the doc for maxConnections.
Today, the doc for net.Server.maxConnections is: Set this property to reject connections when the server's connection count gets high. What needs to be edited / changed?
Set this property to reject connections when the server's connection count gets high.
+ Set it to a non-positive number to reject all connections.
Before the option you'll find a little YAML changelog you need to edit as well. Use REPLACEME instead of a Node.js version.
Sorry, I don't get. Is there a previous commit I can look at?
To document it, you should expand the following comment https://github.com/nodejs/node/blob/52758435e1b057e5c1a775f75d2f2facbed57120/doc/api/net.md#L566-L568
with the following:
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/48276
description: Setting `maxConnections` to `0` drops all the incoming
connections. Previously it was interpreted as `Infinity`.
Also, can you please run git commit --fixup=reword:3a16de99e78777e98708a7911ea5fe4b9c1ab9d8 and set net: do not treat `server.maxConnections=0` as `Infinity` as the third line? Let me know if you need help with that, I can take care of that for you if you prefer. This is to comply with our commit message guidelines that says it should start with an imperative verb: https://github.com/nodejs/node/blob/260092eca316bae6036e52b2cdc73c4c4f8a9da6/doc/contributing/pull-requests.md#L168-L170
changes:
- version: REPLACEME pr-url: https://github.com/nodejs/node/pull/48276 description: Setting
maxConnectionsto0drops all the incoming connections. Previously it was interpreted asInfinity.
Thanks. Changed as mentioned above.
Also, can you please run git commit --fixup=reword:3a16de99e78777e98708a7911ea5fe4b9c1ab9d8 and set net: do not treat
server.maxConnections=0asInfinityas the third line?
I use github.dev for edits and not able to figure out how to run that command. Not enough disk space to checkout Node on my PC, unfortunately.
Let me know if you need help with that, I can take care of that for you if you prefer. This is to comply with our commit message guidelines that says it should start with an imperative verb
Please, feel free to do so.
Thanks a lot (everyone) for being so helpful and patient, despite the fact that I didn't even read contribution guidelines.
Hi all,
@nodejs-github-bot node-test-commit — tests failed @nodejs-github-bot node-test-commit-linuxone-rhel8-s390x — tests failed @nodejs-github-bot node-test-pull-request — tests failed
Are the tests I changed failing? Or, is this commit breaking other tests? I clicked on details but unsure if it I should grant it write permissions to my github account?
I think it's an unrelated flaky tests let me try to "resume ci" (run just the failing parts) and see.
Landed in https://github.com/nodejs/node/commit/72028594026528f7eb640ebb2807d1a1c7bae172
oh my.