Opened 11 days ago

Last modified 4 days ago

#30511 needs_review defect

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

comment:1 Changed 11 days ago by arlolra

Cc: arlolra dcf added

comment:2 Changed 9 days ago by arlolra

Status: newneeds_review

comment:3 Changed 5 days 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 5 days ago by arlolra

Bear in mind that this is just a port of an already merged patch (see the inline comments there),
https://github.com/keroserene/snowflake/commit/c28c8ca489633aae2d9b9dbea0e781ca5e44cc66

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 4 days 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 4 days ago by arlolra

Description: modified (diff)

Replying to cohosh:

Otherwise the patches look good to me.

Thanks, I merged the snowflake changes in,
https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=d7676d2b9e45c957bf39ea2d5662996aeab5b4de
https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=5380aaca8c1c789fa4f692e93dfdc4e59d4992af

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.