emiago / sipgo

SIP library for writing fast SIP services in GO
BSD 2-Clause "Simplified" License
550 stars 73 forks source link

WriteResponse fails when called from a goroutine #110

Closed andzsinszan closed 1 month ago

andzsinszan commented 3 months ago

This is a question, request for clarification.

// OK
dialogServerSession.WriteResponse(resp)

// FAILS silently
go dialogServerSession.WriteResponse(resp)

// FAILS silently
go func(){
  time.Sleep(5 * time.Second)
  dialogServerSession.WriteResponse(resp)
}()

Is there a restriction that dialogues/sessions can only be used from the same goroutine? I am familiar with phone.go, I know how to re-organize my code so that dialogues/sessions do not cross goroutine boundaries if absolutely necessary.

Cf.: phone.go around #1141

err = func() error {
  // ...
  if err := dialog.WriteResponse(res); err != nil {
  // ...
}()   // <-- sic!

That err = func() error {}() is a bit suspicious... almost looks like a goroutine.

emiago commented 3 months ago

If you are exiting Invite Server Transaction, then you are terminating Transaction. Normally you should do all responses in this goroutine. So after sending final response (>199) you are free to jump arround as much as you like.

andzsinszan commented 3 months ago

Thank you for the fast response. So it is a scope/lifetime issue - it isn't about goroutines. Indeed: if I keep OnInvite() alive, other go routines can also WriteResonse. Fine.

Could WriteResponse perhaps return an error upon trying to use a terminated transaction? Currently, tx.Err() WriteResponse just returns nil - as the previous/completed transaction(s) had no error.

// dialgog_server.go #297
select {
case req := <-tx.Cancels():
    tx.Respond(sip.NewResponseFromRequest(req, sip.StatusOK, "OK", nil))
    return ErrDialogCanceled
case <-tx.Done():
    // There must be some error         // [my comment] 'some error':  re-using a Done() transaction
    return tx.Err()
default:
}
emiago commented 3 months ago

@andzsinszan ok I see your point. Maybe it makes sense to return error, similar as context.Context returns on cancelation

andzsinszan commented 3 months ago

Hi @emiago , thanks again for the fast response.

Maybe it makes sense to return error, similar as context.Context returns on cancelation

Thanks, I would really be going for it. As a returning nil error implies that the task was competed.

For reference, it is common to prevent the restart of a completed thread (or session), e.g.,

emiago commented 1 month ago

Hi @andzsinszan now when responding on server transaction you should recieve error. This last one I do not get. Adding more runtime checks could be sign of bad written library as well. sipgo must stay in some low level area and built on to, that is why I suggest that you now switch to https://github.com/emiago/diago project and give feedback there for better call control