Fix coupling between tx.Receive() and 200OK write. Fix ACK missed wa…#289
Fix coupling between tx.Receive() and 200OK write. Fix ACK missed wa…#289csiwek wants to merge 3 commits intoemiago:mainfrom
Conversation
|
I see the other pr also fixes this. this branch also includes the missed ACK warning fix |
sip/transaction_server_tx.go
Outdated
| tx.acks = make(chan *Request) | ||
| // Buffer of 1 so that ACK delivery (e.g. after CANCEL→487) does not | ||
| // block when nobody is reading the channel. | ||
| tx.acks = make(chan *Request, 1) |
There was a problem hiding this comment.
Hi. It should block until transaction is terminated otherwise I think we could refactor this differently. For ex, no need even for goroutines.
Was it some other issue? I would avoid this change for now if not required
There was a problem hiding this comment.
you are right - this isn't an elegant fix. Let me think of something that will not affect other transactions.
There was a problem hiding this comment.
just pushed another way of dealing with those ACKs. This should not affect anything else but CANCEL related transactions
There was a problem hiding this comment.
Moved the ACK handling to another PR
sip/transaction_server_tx_fsm.go
Outdated
|
|
||
| tx.passAck() | ||
| // ACK for a CANCEL-triggered 487 is not relevant to the application. | ||
| if tx.fsmErr != ErrTransactionCanceled { |
There was a problem hiding this comment.
So how would unreliable transport detect CANCEL was successful.
I see here also issue for OnCancel callback, that could be better or more correct usage after receiving ACK.
On other hand could be issue for shadowing this for proxy.
Can we move this ACK in other PR, or it is anyhow relevant. I understand for reliable could mean nothing due to immediate transaction close.
There was a problem hiding this comment.
in the CANCEL flow (INVITE → CANCEL → 487 → ACK), the ACK is just the UAC acknowledging it received the 487.
the passAck() will send the ACK to the channel that nothing is reading from. This produces the warning. Happy to split the PR
There was a problem hiding this comment.
Yes split PR pls, to make this merge.
Reading ACKs I am not sure yet. You could be right in general that makes no sense to consume, just have to justify from proxy perspective.
There was a problem hiding this comment.
ACK for a 487 is hop-by-hop. It should be absorbed by the transaction layer
|
squashed, rebased and merged |
2 participants
Fix the tight coupling between tx.Receive() which fires the FSM and writes 487 synchronously and the subsequent 200 OK write means there's a window where the UA sees 487 first. Swapping the order is still the right fix
make acks buffered with size 1. The ACK gets queued without blocking