refine negotiation process #807
Conversation
cnderrauber
commented
Jul 4, 2022
cnderrauber
commented
- Add a timeout for negotiate process, notify client to do full reconnect if timeout.
- recover from ice restart negotiate
| @@ -2055,3 +2057,16 @@ func (p *ParticipantImpl) GetCachedDownTrack(trackID livekit.TrackID) (*webrtc.R | |||
|
|
|||
| return nil, sfu.ForwarderState{} | |||
| } | |||
|
|
|||
| func (p *ParticipantImpl) handleNegotiationFailed() { | |||
| p.params.Logger.Infow("negotiation failed, notify client do full reconnect") | |||
There was a problem hiding this comment.
not close peerconnection here, since we have pc.state check, pc might still connect and negotiated media track is working, so not close it.
|
|
||
| func (p *ParticipantImpl) handleNegotiationFailed() { | ||
| p.params.Logger.Infow("negotiation failed, notify client do full reconnect") | ||
| _ = p.writeMessage(&livekit.SignalResponse{ |
There was a problem hiding this comment.
if negotiation failed caused by websocket failed, the Leave message might not arrive client
There was a problem hiding this comment.
I think that should be fine as client will detect that.
Question: should we close the signal connection below? Or should reconnect handle that?
There was a problem hiding this comment.
Have no idea which is better (close or left it open). Just concern if client received Leave request, then subsequent message will confuse it. @davidzhao any suggestion about this?
There was a problem hiding this comment.
as soon as clients receive the leave message, it'll ignore subsequent events. it's probably safer to close it right away.
|
|
||
| func (p *ParticipantImpl) handleNegotiationFailed() { | ||
| p.params.Logger.Infow("negotiation failed, notify client do full reconnect") | ||
| _ = p.writeMessage(&livekit.SignalResponse{ |
There was a problem hiding this comment.
I think that should be fine as client will detect that.
Question: should we close the signal connection below? Or should reconnect handle that?
| @@ -276,6 +286,10 @@ func (t *PCTransport) SetRemoteDescription(sd webrtc.SessionDescription) error { | |||
| lastState := t.negotiationState | |||
| t.negotiationState = negotiationStateNone | |||
|
|
|||
| if t.signalStateCheckTimer != nil { | |||
| t.signalStateCheckTimer.Stop() | |||
There was a problem hiding this comment.
Do we need to reset timer to nil here?
| if t.onNegotiationFailed != nil { | ||
| t.onNegotiationFailed() | ||
| } | ||
| } |
There was a problem hiding this comment.
Should timer be set to nil here? Maybe stop also (guess stop may not be necessary as it has already fired)?
Should this grab the lock as it is reading negotiationState?
There was a problem hiding this comment.
will add a lock. I think don't need to stop a fired timer
| @@ -80,6 +80,7 @@ const ( | |||
| ParticipantCloseReasonSimulateMigration | |||
| ParticipantCloseReasonSimulateNodeFailure | |||
| ParticipantCloseReasonSimulateServerLeave | |||
| ParticipantCloseReasonNegotiateFailed | |||
There was a problem hiding this comment.
Can you add this to the String() method below also?
There was a problem hiding this comment.
we also need to add this to ToDisconnectReason: it should be DisconnectReason_STATE_MISMATCH
