Skip to content

fix: don't report a sent final response as failed - #365

Open
fridokus wants to merge 1 commit into
emiago:mainfrom
fridokus:respond-race
Open

fridokus wants to merge 1 commit into
emiago:mainfrom
fridokus:respond-race

Conversation

@fridokus

Copy link
Copy Markdown

ServerTx.Respond spins the FSM, releases fsmMu, then reads the error with tx.Err(). Timer J is zero on reliable transports, so the time.AfterFunc armed in actFinal is runnable at once — if it takes fsmMu in that gap it drives the transaction to Terminated, and Respond returns ErrTransactionTerminated for a response already written to the connection.

Reading the error under the same lock as the spin closes the gap. The timer goroutine then waits for that lock and can no longer change the result of a Respond that succeeded.

Found on a 10k-subscriber SIP soak over TCP: callers answering PRACK and UPDATE treated the error as a failed send and tore down established dialogs.

The test runs 150 rounds, with retransmissions keeping fsmMu contended so it hands off to the waiting timer goroutine rather than letting Respond reacquire. It fails reliably on main.

ServerTx.Respond spun the FSM, let fsmMu go, and then read the error with
tx.Err(). Timer J is zero on reliable transports, so the time.AfterFunc
armed in actFinal is runnable at once; when it took fsmMu in that gap it
drove the transaction to Terminated, and Respond returned
ErrTransactionTerminated for a response already written to the connection.

Read the error under the same lock as the spin. The timer goroutine then
waits for that lock, so it can no longer change the result of a Respond
that succeeded.

Seen on a 10k-subscriber SIP soak over TCP: callers answering PRACK and
UPDATE treated the error as a failed send and tore down established
dialogs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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