Non-blocking server transaction cleanup - #348
Open
ajrice6713 wants to merge 6 commits into
Open
Conversation
Author
|
Hey @emiago any chance to get a review on this? Thanks - its a really great project! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Server.handleRequestcallstx.TerminateGracefully()once the user handler returns. On UDP, if afinal response was sent, that parks the calling goroutine on
<-tx.Done()until the transactionterminates: 64×T1, so 32s at default timers.
That goroutine is the one sipgo spawned to handle the inbound message in the first place — one per
datagram, at
transaction_layer.go:140. The handler has already returned by the time it reaches thepark, so it spends those 32s doing nothing.
This PR adds
ServerTx.Cleanup(): same leak-safety checks, no wait.server.go's single call siteswitches to it.
TerminateGracefullystays as a deprecated wrapper (Cleanup()then<-tx.Done())so no external caller breaks.
The transaction is not touched. It stays in
txl.serverTransactionswith every timer armed andterminates at exactly the same moment it did before. Only the goroutine goes away.
Why the wait isn't needed
Retransmissions never ran on the parked goroutine anyway. Each inbound datagram gets its own
goroutine (
transaction_layer.go:140), which looks the transaction up inserverTransactionsandcalls
tx.Receive. SinceCleanup()doesn't remove the transaction from that map, retransmissionhandling is byte-for-byte what it was before.
<-tx.Done()is the last statement in the function. Nothing reads its value, and the only callerreturns immediately after. So no behavior can depend on it — the reception just decides when the
goroutine dies.
Retransmission and termination are the FSM's job, and every state you can be in after a final
response already has an
AfterFuncarmed for it:inviteStateCompleted(3xx-6xx, no ACK)timer_gfsm.go:188,timer_hfsm.go:204,actRespondCompleteinviteStateConfirmed(ACK in)timer_ifsm.go:286,actConfirminviteStateAccepted(2xx)timer_lfsm.go:219,actRespondAcceptstateCompleted(non-INVITE)timer_jfsm.go:243,actFinalThose all run on their own goroutines, never the parked one. And since
Cleanup()doesn't calldelete()on this path, the transaction stays in the map, so retransmissions still match it andstill reach the FSM.
Re #277: agreed that the transaction has to be held open past its final response. It still is. The
park was holding a goroutine open, not the transaction.
OnTerminate's own docstring already says it's the "alternative to tx.Done where you avoid creatingmore goroutines". This just applies that to the server's own call site.
What Cleanup Terminates
Both early exits from the original function, unchanged:
transaction for 64×T1. This is why
Cleanup()has to be called and not just dropped.Tests
TerminateGracefullyhad no test coverage. Added 7 insip/transaction_server_tx_cleanup_test.go,one per row above, each checking that
Cleanup()returns immediately and that the RFC behaviorstill happens after it does:
...NonInviteReplaysFinal...InviteCompletedRetransmitsFinal...InviteCompletedTerminatesOnTimerH...InviteAcceptedSurvivesUntilTimerL...TerminatesWithoutFinalResponse...TerminatesOnReliableTransport...TerminateGracefullyStillBlocksTimers are shortened via
SetTimersand restored after, so the 64×T1 windows run in milliseconds.Timer G writes come from a runtime timer goroutine, so the test writer is mutex-guarded.
Numbers
Setup: a stateless UDP proxy on 4 vCPU. Per call INVITE → 183 → 200 → BYE, so ~3 server transactions,
SDP rewritten on offer and answer through an external media relay, dialog state in Redis. Both runs
are the same commit on the same host, differing only in the sipgo version.
TerminateGracefullyThat's ~14,000 stacks at 2-8 KiB each, every one pinning its request, response and handler locals for
32s, and all of them walked by the GC on every cycle.
Latency was unchanged in our runs up to 500 CPS.
Not fixed here
ackSendAsync(transaction_server_tx.go:147) spawnsgo tx.ackSend(r)when nothing is readingtx.Acks(), and that goroutine parks ontx.done. Anything handling ACK throughOnRequest(ACK)instead of
tx.Acks(), which is normal for a proxy, leaks one per non-2xx ACK. In our test we measured 823 beforethis change and 774 after, so
Cleanup()doesn't help: the park is ontx.done, which still closesat Timer I/L.
Options look like dropping the ACK when nobody's listening, buffering the channel by one, or only
spawning when a reader is registered. Happy to do it as a separate PR if you have a preference.
Pre-existing test failures
Unmodified
main, not from this PR:TestServerTransactionRespondRejectsCRLFpanics on a nil derefThe
sippackage goes 117 → 124 passing with those two unchanged. Can file issues separately.