Skip to content

net: close connection if no 'connection' listener #32516

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.

Sign up for GitHub

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

Open
bnoordhuis wants to merge 3 commits into
base: main
Choose a base branch
from
Open

net: close connection if no 'connection' listener #32516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
156ea66
net: close connection if no 'connection' listener
bnoordhuis Apr 1, 2020
7316d74
fixup! update doc pr-url
bnoordhuis Apr 1, 2020
41475dc
fixup! use common.mustCall()
bnoordhuis Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/api/net.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ event is not emitted until all connections are ended.
### Event: `'connection'`
<!-- YAML
added: v0.1.90
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/32516
description: New connections are now closed instead of silently leaking them
when there are no `'connection'` listeners.
-->

* {net.Socket} The connection object
Expand Down
5 changes: 5 additions & 0 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,6 +1536,11 @@ function onconnection(err, clientHandle) {
return;
}

if (self.listenerCount('connection') === 0) {
Copy link
Member

@mcollina mcollina Aug 10, 2020

Choose a reason for hiding this comment

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

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.

clientHandle.close();
return;
}

if (self.maxConnections && self._connections >= self.maxConnections) {
clientHandle.close();
return;
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-net-server-no-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Ensure that a net.Server without 'connection' event listeners closes
// incoming connections rather than accepting, then forgetting about them.

'use strict';
const common = require('../common');
const net = require('net');

net.createServer(common.mustCall()).listen(function() {
const server = this;
const { address: host, port } = server.address();
net.connect(port, host).once('end', () => server.close());
Copy link
Member

@ronag ronag Apr 11, 2020

Choose a reason for hiding this comment

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

should probably be once('close', ...

Copy link
Member

@ronag ronag Apr 26, 2020

Choose a reason for hiding this comment

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

@bnoordhuis: I think if you change this to 'close' we can land this PR.

Copy link
Member

@addaleax addaleax Apr 27, 2020

Choose a reason for hiding this comment

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

@ronag At least locally for me, that doesn’t work 😕

Copy link
Member

@ronag ronag Apr 28, 2020

Choose a reason for hiding this comment

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

I'll take a look this week.

Copy link
Member

@ronag ronag May 1, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@ronag ronag May 1, 2020

Choose a reason for hiding this comment

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

This is probably a bug that also affects the self.maxConnections guard.

Copy link
Member

@addaleax addaleax May 5, 2020

Choose a reason for hiding this comment

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

@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())?

Copy link
Member

@ronag ronag May 5, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@ronag ronag May 5, 2020

Choose a reason for hiding this comment

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

#31916

});
2 changes: 1 addition & 1 deletion test/parallel/test-net-socket-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ for (let i = 0; i < invalidCallbacks.length; i++) {
);
}

const server = net.Server();
const server = net.Server((conn) => { /* do nothing */ });
server.listen(0, common.mustCall(() => {
const socket = net.createConnection(server.address().port);
socket.setTimeout(1, common.mustCall(() => {
Expand Down