Repository navigation
dialog_client: preserve routing and isolate retransmitted ACKs - #369
Conversation
…nic)
WriteAck cloned the ACK once and re-sent that same *sip.Request on every 200 OK
retransmission. Each resend runs WriteRequest -> ClientRequestAddVia ->
Request.PrependHeader, which mutates the request's header slice in place. Each
retransmitted 2xx is delivered on its own transaction-layer goroutine (passUpRetransmission
releases the FSM lock before invoking the hook), so two close retransmissions race on
the one shared request and panic in PrependHeader:
panic: runtime error: index out of range [N] with length N
sip.(*headers).PrependHeader
ClientRequestAddVia
DialogClientSession.WriteAck.func1
ClientTx.passUpRetransmission
The panic is raised in the transaction-layer goroutine, so callers cannot recover it
and the process crashes. Reusing the clone also appends a duplicate Via on every resend.
Fix: snapshot the ACK once and send a fresh clone per retransmission. Concurrent
Clone() calls are concurrent reads of the immutable snapshot (safe); each resend
mutates its own copy.
Adds TestDialogClientACKRetransmissionOverlap, which overlaps the retransmission
callbacks and asserts one Via per ACK. It fails under -race on the shared request and
passes with the fix; the existing sequential test does not catch it.
|
@emiago Bumping this up. |
|
Updated in 9d5f4da: snapshot the fully routed ACK and replay fresh copies without rebuilding dialog headers. Added routing and real TCP/authentication regressions; focused race tests pass. This also fixes retransmitted ACKs bypassing a Record-Route proxy because Clone cached the Contact destination too early. |
ddas-rt
changed the title
|
Tests are very here complex, or testing maybe more like some integration. Can you remove them for now. I do not want too much AI slop ending up. |
| } | ||
|
|
||
| if err := s.WriteRequest(retransmissionAck); err != nil { | ||
| if err := s.UA.Client.WriteRequest(ackTemplate.Clone(), s.requestValidate); err != nil { |
There was a problem hiding this comment.
what is point of second Clone? After this request is serilaized
There was a problem hiding this comment.
The second clone gives each retransmission its own request. Before serialization, ClientRequestConnection writes req.raddr and can update the Via host/port.
Retransmission callbacks can overlap because passUpRetransmission releases fsmMu before calling the hook. Reusing ackTemplate would let those writes race with another callback’s transport setup or serialization. The first clone isolates the template from the initial send; the second keeps the template immutable across retransmissions.
This is a defensive strategy to handle the case where UA receives retransmitted ACKs in quick succession, that is addressed to a different endpoint and it needs to forward it to the next destination.
There was a problem hiding this comment.
then problem is overlapping, if I see, but I could only right now think more complex solution.
|
Thanks for detailed explanation. Was busy with other stuff. merging |
|
Awesome! Thanks @emiago . |
2 participants
Fixes #368.
Build ACK routing and headers before taking an immutable snapshot, then send a fresh copy per retransmission without rebuilding dialog headers. This prevents the shared-request race and retries bypassing the Record-Route proxy.
Removed the added tests at maintainer request. Existing comments and tests are preserved; existing dialog-client tests pass under -race (3 runs).