Skip to content

fix: correct SCTPConn close logic and add Abort - #87

Open
gomaja wants to merge 2 commits into
ishidawataru:masterfrom
gomaja:refactor-sctpconn-close-logic
Open

fix: correct SCTPConn close logic and add Abort#87
gomaja wants to merge 2 commits into
ishidawataru:masterfrom
gomaja:refactor-sctpconn-close-logic

Conversation

@gomaja

@gomaja gomaja commented Jan 8, 2026

Copy link
Copy Markdown
Contributor

Close swapped _fd to -1 and then called SCTPWrite to send SCTP_EOF. SCTPWrite reads the descriptor back through c.fd(), so that write always went to -1 and silently failed — the EOF chunk was never sent.

Rather than reorder it, this removes the write and shuts the socket down directly. Close now performs a graceful shutdown and waits for the peer to acknowledge, falling back to ABORT if it does not respond within the grace period. Without that fallback a peer that has gone away left the association occupying its address, so a later bind failed with EADDRINUSE. The same path is used on the dial and listen error paths, where a plain close() left the socket bound and made the next dial fail the same way.

Also in here:

  • Abort() for callers wanting immediate termination with no handshake.
  • fd 0 leak. The descriptor guard was fd > 0. Zero is valid — a process that has closed stdin can be handed fd 0 for a socket, and such a connection was never closed at all, leaking it while Close returned EBADF as though nothing had been open. Now fd >= 0.
  • Discarded errors. closeSctpSocket ignored every syscall result. The timeout guarantee rests on SO_RCVTIMEO being programmed, so a failure there turned the wait into an unbounded blocking read; those cases now fall back to an immediate abort. The wait read itself is still ignored deliberately — every outcome ends the wait — and that is now stated rather than implied.
  • CloseWithTimeout exposes the grace period instead of hardcoding 3s, with both defaults as named constants. A non-positive timeout is equivalent to Abort.
  • The timeval is built with syscall.NsecToTimeval; the manual Sec/Usec arithmetic truncated a sub-microsecond timeout to zero, which the kernel reads as "no timeout".

Tests

The fd 0 leak is proven through the public API, not by asserting on internals: a child process closes stdin so the association lands on fd 0, then checks Close both reports success and actually released the descriptor. Reverting the guard to fd > 0 fails both the Close and Abort variants.

Scoped cases cover rebinding after close, an unresponsive peer not holding Close past its timeout, double close returning EBADF, a nil receiver, and concurrency — exactly one of eight racing Close/Abort callers wins the descriptor, and close racing a blocked read or active writer unblocks them. FuzzCloseWithTimeout varies the grace period across the negative, zero and sub-microsecond values (47,800 executions, no failures). TestCloseChurnUnderLoad runs 120 concurrent dial/teardown cycles asserting no descriptor growth.

testdata/ adds a Docker harness and a tshark script, since the difference between the teardown paths is only visible on the wire:

path observed
Close SHUTDOWN → SHUTDOWN_ACK → SHUTDOWN_COMPLETE, no ABORT
Abort one ABORT, no SHUTDOWN
Close, unresponsive peer ABORT fallback in ~100ms

Notes

The MSG_DONTWAIT change that was previously in this branch has been split out — it alters SCTPWrite's contract for every caller and deserves reviewing on its own.

TestDialUnderChurnReportsEISCONN records a separate dial-side limitation found while fuzzing: SCTPConnect can return EISCONN on a freshly created socket under rapid reconnects. It is documented rather than asserted so it does not become flaky.

CI shows TestStreams failing; it fails the same way on an unmodified master and is untouched here.

@gomaja
gomaja marked this pull request as draft January 8, 2026 23:20
@gomaja

gomaja commented Jan 8, 2026

Copy link
Copy Markdown
Contributor Author

@ishidawataru after the fix, the c.SCTPWrite(nil, info) is returning an error "invalid argument"

I think SCTPWrite can be removed completely from Close.

@gomaja
gomaja marked this pull request as ready for review January 11, 2026 12:21
@gomaja

gomaja commented Jan 18, 2026

Copy link
Copy Markdown
Contributor Author

This code part "defer" after an error with close is not performing well in dialSCTPExtConfig, specifically when SCTPConnect is returning one of the errors (syscall.EISCONN, syscall.EALREADY, syscall.EINPROGRESS). Mostly syscall.EINPROGRESS.
The close in defer is not completely closing the socket, and the next call of dialSCTPExtConfig is always giving syscall.EADDRINUSE. The issue can be fixed by adding also syscall.Shutdown(sock, syscall.SHUT_RDWR). However, I solved it with a more general close.

defer func() {
		if err != nil {
			syscall.Close(sock)
		}
	}()

@gomaja
gomaja marked this pull request as draft January 20, 2026 21:08
@gomaja
gomaja force-pushed the refactor-sctpconn-close-logic branch from b5ed2cc to b2960c6 Compare January 24, 2026 19:21
@gomaja
gomaja marked this pull request as ready for review February 6, 2026 16:22
@gomaja

gomaja commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

PR ready for review,. Changes have made operation very smooth as previously it was very faulty specifically when the connection sends to a backhole (peer no more reachable) while having many messages to send (Full send-Q).

Later we can refactor syscall.SendmsgN(c.fd(), b, cbuf, nil, syscall.MSG_DONTWAIT) to a better one.

Close swapped _fd to -1 and then called SCTPWrite to send SCTP_EOF.
SCTPWrite reads the descriptor back through c.fd(), so that write always
went to -1 and silently failed; the EOF chunk was never sent. Remove it
and shut the socket down directly.

Close now initiates a graceful shutdown and waits for the peer to
acknowledge, falling back to an ABORT if the peer does not respond
within the grace period. Without that fallback a peer that has gone away
left the association occupying its address, so a subsequent bind failed
with EADDRINUSE.

Add Abort for callers that want immediate termination with no handshake,
and use the same path on the dial and listen error paths, where a plain
close left the socket bound and made the next dial fail with EADDRINUSE.

The guard on the descriptor was "fd > 0". Zero is a valid descriptor: a
process that has closed stdin can be handed fd 0 for a socket, and such
a connection was never closed at all, leaking the socket while Close
returned EBADF as though nothing had been open. Guard with "fd >= 0".

The grace period is exposed through CloseWithTimeout rather than
hardcoded, and both defaults are named constants. A non-positive timeout
skips the handshake and is equivalent to Abort.

Failures from Shutdown and from the two setsockopt calls are no longer
discarded. The timeout guarantee rests on SO_RCVTIMEO being programmed,
so a failure there would turn the wait into an unbounded blocking read;
those cases now fall back to an immediate abort. The result of the wait
read itself is still ignored deliberately, since every outcome ends the
wait, and that is now stated rather than implied.

The timeval is built with syscall.NsecToTimeval. Manual Sec/Usec
arithmetic truncated a sub-microsecond timeout to zero, which the kernel
reads as "no timeout" rather than "expire immediately".
@gomaja
gomaja force-pushed the refactor-sctpconn-close-logic branch 2 times, most recently from d828e35 to abbfb5d Compare July 28, 2026 22:48
The fd 0 leak is proven through the public API rather than by asserting
on internals: a child process closes stdin so the association lands on
fd 0, then checks Close both reports success and actually released the
descriptor. Reverting the guard to "fd > 0" fails both the Close and the
Abort variant, so these demonstrate the bug rather than merely passing
alongside the fix.

Scoped cases cover what the rework provides: the local address is
rebindable once an association is closed, an unresponsive peer does not
hold Close past its timeout, a second Close reports EBADF, and a nil
receiver is handled. Concurrency cases assert exactly one of eight
racing Close and Abort callers wins the descriptor, and that close
racing a blocked read or an active writer unblocks them.

FuzzCloseWithTimeout varies the grace period, covering the negative and
zero values that select the immediate-abort path and the sub-microsecond
values where the timeval conversion could round down to "block forever".
The invariant is that CloseWithTimeout always returns, always releases
the descriptor, and a second call always reports EBADF. 47800 executions
found no failures.

TestCloseChurnUnderLoad is the signalling-node case: 120 concurrent
dial/teardown cycles alternating graceful and immediate close, asserting
no descriptor growth.

testdata/ adds a Docker harness and a tshark script, since the
difference between the teardown paths is only observable on the wire.
Close produces a SHUTDOWN, SHUTDOWN_ACK and SHUTDOWN_COMPLETE exchange
with no ABORT; Abort produces a single ABORT and no SHUTDOWN; and Close
against an unresponsive peer falls back to ABORT in about 100ms rather
than waiting out the full grace period.

TestDialUnderChurnReportsEISCONN records a dial-side limitation found
while fuzzing: SCTPConnect can return EISCONN on a freshly created
socket under rapid reconnects, which DialSCTP surfaces unchanged. It is
separate from the close path and is documented rather than asserted, so
it does not become a flaky test.
@gomaja
gomaja force-pushed the refactor-sctpconn-close-logic branch from abbfb5d to 6f9fd88 Compare July 28, 2026 23:13
@gomaja gomaja changed the title ref: makes correction to SCTPConn close logic for SCTPWrite fix: correct SCTPConn close logic and add Abort Jul 28, 2026
@gomaja

gomaja commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased and restructured this to make it reviewable.

The branch had grown to seven commits covering four unrelated things. It is now two: the close fix, and its tests.

The MSG_DONTWAIT change moved out to #90 — it changes SCTPWrite's contract for every caller and should not ride along in a close-logic PR.

While adding tests I found the descriptor guard bug: fd > 0 skips fd 0, which is a valid descriptor, so a connection landing there leaked. Fixed here, with a test that fails if the guard is put back. Also handled the discarded syscall errors in closeSctpSocket and exposed the grace period through CloseWithTimeout instead of hardcoding it.

The original seven commits are preserved at backup/pr87-original on my fork if any of the intermediate history is wanted.

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.

1 participant