#30511 closed defect (fixed)

Remove OnIceComplete

Reported by: arlolra Owned by:
Priority: Low Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: cohosh, arlolra, dcf Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Child Tickets

Change History (8)

comment:1 Changed 15 months ago by arlolra

Cc: arlolra dcf added

comment:2 Changed 15 months ago by arlolra

Status: newneeds_review

comment:3 Changed 15 months ago by dcf

I'm trying to understand the change around pc.CreateAnswer: https://github.com/keroserene/snowflake/compare/ice#diff-e32e5d12043e17d487f1252ee61dfd5fL167

Was it always unnecessary to run pc.CreateAnswer() in a goroutine? Is the removal of errChan and answerChan a separate cleanup, or is it necessitated by the removal of OnIceComplete?

comment:4 Changed 15 months ago by arlolra

Bear in mind that this is just a port of an already merged patch (see the inline comments there),

pc.CreateAnswer() is a blocking operation, which is probably why it was put in a goroutine. However, makePeerConnectionFromOffer falsely assumed that if OnIceComplete was called, it was ready to move on to sending the answer. That setup a race between getting the local description in sendAnswer and setting it in the goroutine, which was never great. Not to mention that there was plenty of time for ice to fail after the gathering stage completed, which resulted in the deadlock.

Since makePeerConnectionFromOffer always blocked waiting on the channels, it isn't a significant change to remove the goroutine since, as the comment in the commit states,

... we need
SetLocalDescription(answer) to be called before sendAnswer

It might be worth looking at whether runSession deserves to be called asynchronously, but that seems orthogonal to the change here.

comment:5 Changed 15 months ago by cohosh

Also noting that with the current implementation, pc.CreateAnswer won't block indefinitely. It will time out and return nil after 3 seconds: peerconnection.cc#L355.

It might be better practice to include our own timeout and not rely on the underlying implementation, at the very least we should document that this method times out since right now the documentation only mentions that it is blocking: https://godoc.org/github.com/keroserene/go-webrtc#PeerConnection.CreateAnswer

I left the comment to note that we might want to change this later.

Otherwise the patches look good to me.

comment:6 in reply to:  5 Changed 15 months ago by arlolra

Description: modified (diff)

Replying to cohosh:

Otherwise the patches look good to me.

Thanks, I merged the snowflake changes in,

For go-webrtc, I wasn't sure if you all looked at that yet and since it's a backwards incompatible change, I'll leave some more time.

comment:7 Changed 14 months ago by dcf

Status: needs_reviewmerge_ready

I think the go-webrtc change is fine as well.

comment:8 in reply to:  7 Changed 14 months ago by arlolra

Resolution: fixed
Status: merge_readyclosed

Replying to dcf:

I think the go-webrtc change is fine as well.

Merged as https://github.com/keroserene/go-webrtc/commit/68a6fb1b4353657c205bd7f6eba67630a5702bf2

Note: See TracTickets for help on using tickets.