Skip to content

fix(net): close the connection when onOpen throws - #10

Merged
teidesu merged 1 commit into
teidesu:mainfrom
jumpogpo:fix/net-close-connection-on-error
Oct 14, 2026
Merged

teidesu merged 1 commit into
teidesu:mainfrom
jumpogpo:fix/net-close-connection-on-error

Conversation

@jumpogpo

Copy link
Copy Markdown
Contributor

Problem

When onOpen throws, PersistentConnection#loop() sets #connection to undefined but never closes the connection. That kind of error doesn't always mean the connection is already closed. A protocol-level error can be thrown while the socket is still open, for example when the server replies with an error but keeps the connection. After that nothing holds a reference to the connection, so it stays open until the peer closes it.

If onError returns 'reconnect', every such error leaks one more socket. In a real deployment this built up to ~10k sockets in ESTABLISHED state in a single process.

The default onError has the same problem. It returns 'close' for anything that isn't a ConnectionClosedError, the loop exits, and the connection is never closed.

Fix

Right after detaching the old connection, close it before calling onClose/onError. Any error from close() is ignored because the connection may already be closed. I also updated the onError doc comment to say it receives an already-closed connection in this case.

Testing

  • Added regression tests for the 'reconnect' and 'close' actions. Both fail without the fix ("expected close to be called once, but got 0 times").
  • vitest run packages/net: 149 passed. eslint and tsc report no errors.
  • With the TCP repro below running for 1 second, the server is left with 8,258 open sockets before the fix and 0 after.
Repro (real TCP sockets)
import net from 'node:net'
import { PersistentConnection } from '@fuman/net'
import { connectTcp } from '@fuman/node'

// a server that replies with an error and keeps the socket open
const open = new Set()
const server = net.createServer((sock) => {
  open.add(sock)
  sock.on('close', () => open.delete(sock))
  sock.on('error', () => {})
  sock.write('ERR\n')
})
await new Promise(resolve => server.listen(0, '127.0.0.1', resolve))

const pc = new PersistentConnection({
  connect: addr => connectTcp(addr),
  strategy: () => 0,
  onOpen: async (conn) => {
    await conn.read(new Uint8Array(4))
    throw new Error('server replied with an error') // the socket is still open at this point
  },
  onError: () => 'reconnect',
})

pc.connect({ address: '127.0.0.1', port: server.address().port })
await new Promise(resolve => setTimeout(resolve, 1000))
await pc.close()
await new Promise(resolve => setTimeout(resolve, 200))

console.log('sockets still open on the server:', open.size) // before: thousands, after: 0
for (const s of open) s.destroy()
server.close()

AI-assisted: I investigated this and wrote the fix with help from Claude Opus 5.5 in Claude Code. I have reviewed the change and verified it myself.

When onOpen throws, PersistentConnection dropped its reference to the connection without closing it. Such an error does not necessarily mean the connection is already closed: a protocol-level error (e.g. an error frame sent by the server) can be thrown while the underlying socket is still open. Nothing references the connection after that, so it stayed open until the peer closed it, and every reconnect leaked another one. The default onError is affected as well, since it returns 'close' for such errors without closing the connection.

Close the old connection before calling onClose/onError, and document that onError receives an already closed connection in this case.

Add regression tests for both the 'reconnect' and 'close' actions.
@pkg-pr-new

pkg-pr-new Bot commented Oct 14, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/teidesu/fuman/@fuman/build@10
npm i https://pkg.pr.new/teidesu/fuman/@fuman/bun@10
npm i https://pkg.pr.new/teidesu/fuman/@fuman/net@10
npm i https://pkg.pr.new/teidesu/fuman/@fuman/node@10

commit: e71a688

@teidesu
teidesu merged commit 1c5954e into teidesu:main Oct 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants