Skip to content

fix: Accept() must not close the listener socket on success#81

Open
cdgraff wants to merge 1 commit into
Eyevinn:masterfrom
cdgraff:fix/accept-must-not-close-listener
Open

fix: Accept() must not close the listener socket on success#81
cdgraff wants to merge 1 commit into
Eyevinn:masterfrom
cdgraff:fix/accept-must-not-close-listener

Conversation

@cdgraff

@cdgraff cdgraff commented Jul 14, 2026

Copy link
Copy Markdown

Summary

NodeSRT::Accept() (src/node-srt.cc) called srt_close() on the listener socket on the success path, so the listener was destroyed after every accepted connection.

In libsrt, srt_accept() returns the newly accepted socket and leaves the listener open so it can keep accepting - this diverged from that contract:

  • A server could accept only one connection per listener; after the first accept() the listener went BROKEN then NONEXIST, and any further caller timed out.
  • The usual workaround, re-creating (bind+listen) the listener after every accept, drops its backlog of pending handshakes. Under a burst of simultaneous callers this meant only one connection got through and the rest failed.

Fix

Removes the success-path srt_close(socketValue) (and the following no-op reassignment), so Accept() only returns the accepted fd and leaves the listener open, matching libsrt semantics. The caller is responsible for closing the listener when done. The failure path is unchanged.

Test

Adds spec/accept_keeps_listener_open_spec.js, a regression test covering both the sync (SRT) and async (AsyncSRT) accept paths: it accepts several connections in a row from the same listener and asserts the listener stays LISTENING throughout.

Verified locally:

  • Reverting just the src/node-srt.cc change and rebuilding the addon reproduces the bug: the listener's state goes BROKEN right after the first accept, and the next connection attempt errors out (invalid listener socket ID value).
  • With the fix applied, the full spec suite passes (23/23), including the new test.

Fixes #79

cc @birme

NodeSRT::Accept() called srt_close() on the listener socket on the success
path, so the listener was destroyed after every accepted connection. In
libsrt, srt_accept() returns the newly accepted socket and leaves the
listener open so it can keep accepting - this diverged from that contract.

A server could accept only one connection per listener; after the first
accept() the listener went BROKEN then NONEXIST, and any further caller
timed out. Working around it by re-creating the listener after every
accept dropped its backlog of pending handshakes, so a burst of
simultaneous callers lost all but one.

Removes the success-path srt_close()/reset so Accept() only returns the
accepted fd and leaves the listener open, matching libsrt semantics; the
caller is responsible for closing the listener when done. The failure
path is unchanged.

Adds a regression test covering both the sync (SRT) and async (AsyncSRT)
accept paths: it accepts several connections in a row from the same
listener and asserts the listener stays LISTENING throughout.

Fixes Eyevinn#79
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.

Accept() closes the listener socket on success — a listener can only accept one connection

1 participant