Opened 3 months ago

Closed 3 months ago

#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 3 months ago by arlolra

Cc: arlolra dcf added

comment:2 Changed 3 months ago by arlolra

Status: newneeds_review

comment:3 Changed 3 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 3 months 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 3 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 3 months 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.

comment:7 Changed 3 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 3 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.