net: close connection if no 'connection' listener #32516
Conversation
A `net.Server` without a `'connection'` event listener would accept incoming connections but then proceeded to drop them into the void: the connections stay around, using up resources, but can no longer be reached by the program - i.e., they become resource leaks. Avoid that by closing the connection when there is no listener. It would be even better to not accept connections at all but libuv does not currently support that because It's Complicated.
bnoordhuis
force-pushed
the
net-server-no-handler
branch
from
deeb645 to
41475dc
Compare
addaleax
added
the
author ready
addaleax
removed
the
author ready
|
@bnoordhuis Looks like the added test times out in CI. |
| net.createServer(common.mustCall()).listen(function() { | ||
| const server = this; | ||
| const { address: host, port } = server.address(); | ||
| net.connect(port, host).once('end', () => server.close()); |
There was a problem hiding this comment.
should probably be once('close', ...
There was a problem hiding this comment.
@bnoordhuis: I think if you change this to 'close' we can land this PR.
There was a problem hiding this comment.
@ronag At least locally for me, that doesn’t work 😕
There was a problem hiding this comment.
@addaleax This seems to be related to a bug on the tcp handle. _handle.readStart is called but _handle.onread is never invoked, in this case I guess it should be an error such as ECONNRESET.
There was a problem hiding this comment.
This is probably a bug that also affects the self.maxConnections guard.
There was a problem hiding this comment.
@ronag I feel like we had a conversation about this recently, assuming that we’re talking about the client handle here – namely that .close() doesn’t necessarily actually send a RST? I guess that means that we should shutdown the handle here (or .end() the created stream, which is a bit more expensive but avoids having to manually call .shutdown())?
There was a problem hiding this comment.
namely that .close() doesn’t necessarily actually send a RST
I thought this wasn't necessary. That's at least what I got from the conversation and that the handle should detect ungraceful disconnection?
BridgeAR
force-pushed
the
master
branch
2 times, most recently
from
8ae28ff to
2935f72
Compare
| @@ -1536,6 +1536,11 @@ function onconnection(err, clientHandle) { | |||
| return; | |||
| } | |||
|
|
|||
| if (self.listenerCount('connection') === 0) { | |||
There was a problem hiding this comment.
I have a feeling this might be expensive for all incoming connections. Instead, you can check the return value of emit: https://nodejs.org/api/events.html#events_emitter_emit_eventname_args.
|
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. |
|
Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions. |
lpinca
added
request-ci
github-actions
bot
removed
the
request-ci
|
This needs a rebase. |
A
net.Serverwithout a'connection'event listener would acceptincoming connections but then proceeded to drop them into the void:
the connections stay around, using up resources, but can no longer
be reached by the program - i.e., they become resource leaks.
Avoid that by closing the connection when there is no listener.
It would be even better to not accept connections at all but libuv
does not currently support that because It's Complicated.