From c28c8ca489633aae2d9b9dbea0e781ca5e44cc66 Mon Sep 17 00:00:00 2001 From: Cecylia Bocovich Date: Sat, 30 Mar 2019 12:19:29 -0400 Subject: [PATCH 1/3] Fix for proxy deadlock bug This is a fix for the proxy-go deadlock bug (ticket #25688). The assumption that OnIceComplete is always followed by a successful connection where OnDataChannel has been called turns out not to occur in practice. OnICEComplete looks like it is being deprecated in other libraries anyway, so it's safer to just remove it. --- proxy-go/snowflake.go | 44 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/proxy-go/snowflake.go b/proxy-go/snowflake.go index 2e8fe20..aeb9f51 100644 --- a/proxy-go/snowflake.go +++ b/proxy-go/snowflake.go @@ -259,8 +259,6 @@ func datachannelHandler(conn *webRTCConn, remoteAddr net.Addr) { // Installs an OnDataChannel callback that creates a webRTCConn and passes it to // datachannelHandler. func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc.Configuration) (*webrtc.PeerConnection, error) { - errChan := make(chan error) - answerChan := make(chan struct{}) pc, err := webrtc.NewPeerConnection(config) if err != nil { @@ -269,9 +267,6 @@ func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc. pc.OnNegotiationNeeded = func() { panic("OnNegotiationNeeded") } - pc.OnIceComplete = func() { - answerChan <- struct{}{} - } pc.OnDataChannel = func(dc *webrtc.DataChannel) { log.Println("OnDataChannel") @@ -310,30 +305,25 @@ func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc. } log.Println("sdp offer successfully received.") - go func() { - log.Println("Generating answer...") - answer, err := pc.CreateAnswer() // blocking - if err != nil { - errChan <- err - return - } - err = pc.SetLocalDescription(answer) - if err != nil { - errChan <- err - return - } - }() - - // Wait until answer is ready. - select { - case err = <-errChan: + log.Println("Generating answer...") + answer, err := pc.CreateAnswer() + // blocks on ICE gathering. we need to add a timeout if needed + // not putting this in a separate go routine, because we need + // SetLocalDescription(answer) to be called before sendAnswer + if err != nil { + pc.Destroy() + return nil, err + } + + if answer == nil { + pc.Destroy() + return nil, fmt.Errorf("Failed gathering ICE candidates.") + } + + err = pc.SetLocalDescription(answer) + if err != nil { pc.Destroy() return nil, err - case _, ok := <-answerChan: - if !ok { - pc.Destroy() - return nil, fmt.Errorf("Failed gathering ICE candidates.") - } } return pc, nil } From 08f5205461573bf8a6e8961540ac620865a3b45c Mon Sep 17 00:00:00 2001 From: Cecylia Bocovich Date: Wed, 3 Apr 2019 15:59:47 -0400 Subject: [PATCH 2/3] Added check to see if peer connection succeeded This is related to the proxy-go deadlock bug #25688. If a client doesn't do anything with the SDP answer, a token will get lost. Added a timeout after a minute that checks the PeerConnection state and destroys the peer connection and returns a token if did not yet succeed --- proxy-go/snowflake.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/proxy-go/snowflake.go b/proxy-go/snowflake.go index aeb9f51..1c24e47 100644 --- a/proxy-go/snowflake.go +++ b/proxy-go/snowflake.go @@ -307,9 +307,9 @@ func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc. log.Println("Generating answer...") answer, err := pc.CreateAnswer() - // blocks on ICE gathering. we need to add a timeout if needed - // not putting this in a separate go routine, because we need - // SetLocalDescription(answer) to be called before sendAnswer + // blocks on ICE gathering. we need to add a timeout if needed + // not putting this in a separate go routine, because we need + // SetLocalDescription(answer) to be called before sendAnswer if err != nil { pc.Destroy() return nil, err @@ -325,6 +325,20 @@ func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc. pc.Destroy() return nil, err } + + // Set a timeout on peerconnection. If the connection state has not + // advanced to PeerConnectionStateConnected in this time, + // destroy the peer connection and return the token. + go func() { + <-time.After(time.Minute) + if pc.ConnectionState() != webrtc.PeerConnectionStateConnected { + log.Println("Timed out waiting for client to open data cannel.") + pc.Destroy() + retToken() + } + + }() + return pc, nil } From 62fddab153019ac7e5d7efd1d327b20aede921c3 Mon Sep 17 00:00:00 2001 From: Cecylia Bocovich Date: Fri, 5 Apr 2019 10:40:11 -0400 Subject: [PATCH 3/3] Moved data channel timeout to constant --- proxy-go/snowflake.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/proxy-go/snowflake.go b/proxy-go/snowflake.go index 1c24e47..50c20a5 100644 --- a/proxy-go/snowflake.go +++ b/proxy-go/snowflake.go @@ -26,6 +26,9 @@ const defaultBrokerURL = "https://snowflake-broker.bamsoftware.com/" const defaultRelayURL = "wss://snowflake.bamsoftware.com/" const defaultSTUNURL = "stun:stun.l.google.com:19302" const pollInterval = 5 * time.Second +//amount of time after sending an SDP answer before the proxy assumes the +//client is not going to connect +const dataChannelTimeout = time.Minute var brokerURL *url.URL var relayURL string @@ -330,7 +333,7 @@ func makePeerConnectionFromOffer(sdp *webrtc.SessionDescription, config *webrtc. // advanced to PeerConnectionStateConnected in this time, // destroy the peer connection and return the token. go func() { - <-time.After(time.Minute) + <-time.After(dataChannelTimeout) if pc.ConnectionState() != webrtc.PeerConnectionStateConnected { log.Println("Timed out waiting for client to open data cannel.") pc.Destroy()