Fix bug where max call duration was not being respected for outbound calls. - #762
Fix bug where max call duration was not being respected for outbound calls.#762alexfish8 wants to merge 12 commits into
Conversation
| c.CloseWith(ctx, EndCall{ | ||
| Status: CallHangup, | ||
| Term: stats.Success("max-call-duration"), | ||
| Reason: livekit.DisconnectReason_CLIENT_INITIATED, |
There was a problem hiding this comment.
hmmm.. Would DisconnectReason_CONNECTION_TIMEOUT be more accurate here?
There was a problem hiding this comment.
When the inbound path times out due to max call duration being exceeded, the reason is given as CLIENT_INITIATED. Consistency aside, CLIENT_INITIATED feels more appropriate, as, ultimately, it was the client's request's configuration that caused the call to end early (but, I don't feel super strongly here. I can change both the inbound / outbound paths to give CONNECTION_TIMEOUT as the reason if you think that'd be more appropriate.)
alexfish8
left a comment
alexfish8
left a comment
There was a problem hiding this comment.
Thanks for the reviews!
| c.CloseWith(ctx, EndCall{ | ||
| Status: CallHangup, | ||
| Term: stats.Success("max-call-duration"), | ||
| Reason: livekit.DisconnectReason_CLIENT_INITIATED, |
There was a problem hiding this comment.
When the inbound path times out due to max call duration being exceeded, the reason is given as CLIENT_INITIATED. Consistency aside, CLIENT_INITIATED feels more appropriate, as, ultimately, it was the client's request's configuration that caused the call to end early (but, I don't feel super strongly here. I can change both the inbound / outbound paths to give CONNECTION_TIMEOUT as the reason if you think that'd be more appropriate.)
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Detach this context from the context of the RPC so that the RPC returning | ||
| // doesn't cause the call to end prematurely. | ||
| go func() { | ||
| detachedCtx, detatchedCancel := context.WithDeadline(context.WithoutCancel(ctx), deadline) |
There was a problem hiding this comment.
| detachedCtx, detatchedCancel := context.WithDeadline(context.WithoutCancel(ctx), deadline) | |
| detachedCtx, detachedCancel := context.WithDeadline(context.WithoutCancel(ctx), deadline) |
| // doesn't cause the call to end prematurely. | ||
| go func() { | ||
| detachedCtx, detatchedCancel := context.WithDeadline(context.WithoutCancel(ctx), deadline) | ||
| defer detatchedCancel() |
There was a problem hiding this comment.
| defer detatchedCancel() | |
| defer detachedCancel() |
| case <-ctx.Done(): | ||
| c.CloseWith(ctx, EndCall{ | ||
| Status: CallHangup, | ||
| Term: stats.Success("hangup"), |
There was a problem hiding this comment.
out of scope of this PR, but I think it's beneficial to differentiate max-duration reached vs normal hangup. Feel free to create a follow up ticket for this 👍
3 participants
Also in this PR:
*outboundCall.mediainoutboundCall.close(if the call toNewMediaPortinnewCallfails, then we'd hit this nil deref).tx.Responsesin `sipResponse.