Opened 18 months ago

Last modified 17 months ago

#30511 closed defect

Remove OnIceComplete — at Version 6

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 (6)

comment:1 Changed 18 months ago by arlolra

Cc: arlolra dcf added

comment:2 Changed 18 months ago by arlolra

Status: newneeds_review

comment:3 Changed 18 months ago by dcf

I'm trying to understand the change around pc.CreateAnswer:

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 18 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 18 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:

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:

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 18 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.

Note: See TracTickets for help on using tickets.