Eliminate separate WebRTCPeer.Connect method.

Do it as a side effect of NewWebRTCPeer.

Remove WebRTCPeer tests as they currently require invasively modifying
internal fields at different stages of construction.

client/lib/interfaces.go

@@ -5,10 +5,6 @@ import (
 	"net"
 )
 
-type Connector interface {
-	Connect() error
-}
-
 // Interface for catching Snowflakes. (aka the remote dialer)
 type Tongue interface {
 	Catch() (*WebRTCPeer, error)

client/lib/lib_test.go

@@ -6,35 +6,12 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
-	"sync"
 	"testing"
 
 	"git.torproject.org/pluggable-transports/snowflake.git/common/util"
-	"github.com/pion/webrtc/v2"
 	. "github.com/smartystreets/goconvey/convey"
 )
 
-type MockDataChannel struct {
-	destination bytes.Buffer
-	done        chan bool
-}
-
-func (m *MockDataChannel) Send(data []byte) error {
-	m.destination.Write(data)
-	m.done <- true
-	return nil
-}
-
-func (*MockDataChannel) Close() error { return nil }
-
-type MockResponse struct{}
-
-func (m *MockResponse) Read(p []byte) (int, error) {
-	p = []byte(`{"type":"answer","sdp":"fake"}`)
-	return 0, nil
-}
-func (m *MockResponse) Close() error { return nil }
-
 type MockTransport struct {
 	statusOverride int
 	body           []byte
@@ -74,10 +51,6 @@ func (f FakePeers) Collect() (*WebRTCPeer, error) { return &WebRTCPeer{}, nil }
 func (f FakePeers) Pop() *WebRTCPeer              { return nil }
 func (f FakePeers) Melted() <-chan struct{}       { return nil }
 
-const sampleSDP = `"v=0\r\no=- 4358805017720277108 2 IN IP4 8.8.8.8\r\ns=-\r\nt=0 0\r\na=group:BUNDLE data\r\na=msid-semantic: WMS\r\nm=application 56688 DTLS/SCTP 5000\r\nc=IN IP4 8.8.8.8\r\na=candidate:3769337065 1 udp 2122260223 8.8.8.8 56688 typ host generation 0 network-id 1 network-cost 50\r\na=candidate:2921887769 1 tcp 1518280447 8.8.8.8 35441 typ host tcptype passive generation 0 network-id 1 network-cost 50\r\na=ice-ufrag:aMAZ\r\na=ice-pwd:jcHb08Jjgrazp2dzjdrvPPvV\r\na=ice-options:trickle\r\na=fingerprint:sha-256 C8:88:EE:B9:E7:02:2E:21:37:ED:7A:D1:EB:2B:A3:15:A2:3B:5B:1C:3D:D4:D5:1F:06:CF:52:40:03:F8:DD:66\r\na=setup:actpass\r\na=mid:data\r\na=sctpmap:5000 webrtc-datachannel 1024\r\n"`
-
-const sampleAnswer = `{"type":"answer","sdp":` + sampleSDP + `}`
-
 func TestSnowflakeClient(t *testing.T) {
 
 	Convey("Peers", t, func() {
@@ -191,79 +164,6 @@ func TestSnowflakeClient(t *testing.T) {
 			Handler(socks, snowflakes)
 			So(socks.rejected, ShouldEqual, true)
 		})
-
-		Convey("WebRTC Connection", func() {
-			c := NewWebRTCPeer(nil, nil)
-			So(c.buffer.Bytes(), ShouldEqual, nil)
-
-			Convey("Can construct a WebRTCConn", func() {
-				s := NewWebRTCPeer(nil, nil)
-				So(s, ShouldNotBeNil)
-				So(s.offerChannel, ShouldNotBeNil)
-				So(s.answerChannel, ShouldNotBeNil)
-				s.Close()
-			})
-
-			Convey("Write buffers when datachannel is nil", func() {
-				c.Write([]byte("test"))
-				c.transport = nil
-				So(c.buffer.Bytes(), ShouldResemble, []byte("test"))
-			})
-
-			Convey("Write sends to datachannel when not nil", func() {
-				mock := new(MockDataChannel)
-				c.transport = mock
-				mock.done = make(chan bool, 1)
-				c.Write([]byte("test"))
-				<-mock.done
-				So(c.buffer.Bytes(), ShouldEqual, nil)
-				So(mock.destination.Bytes(), ShouldResemble, []byte("test"))
-			})
-
-			Convey("Exchange SDP sets remote description", func() {
-				c.offerChannel = make(chan *webrtc.SessionDescription, 1)
-				c.answerChannel = make(chan *webrtc.SessionDescription, 1)
-
-				c.config = &webrtc.Configuration{}
-				c.pc, _ = webrtc.NewPeerConnection(*c.config)
-				offer, _ := c.pc.CreateOffer(nil)
-				err := c.pc.SetLocalDescription(offer)
-				So(err, ShouldBeNil)
-
-				c.offerChannel <- nil
-				answer, err := util.DeserializeSessionDescription(sampleAnswer)
-				So(err, ShouldBeNil)
-				So(answer, ShouldNotBeNil)
-				c.answerChannel <- answer
-				err = c.exchangeSDP()
-				So(err, ShouldBeNil)
-			})
-
-			Convey("Exchange SDP keeps trying on nil answer", func(ctx C) {
-				var wg sync.WaitGroup
-				wg.Add(1)
-
-				c.offerChannel = make(chan *webrtc.SessionDescription, 1)
-				c.answerChannel = make(chan *webrtc.SessionDescription, 1)
-				c.config = &webrtc.Configuration{}
-				c.pc, _ = webrtc.NewPeerConnection(*c.config)
-				offer, _ := c.pc.CreateOffer(nil)
-				c.pc.SetLocalDescription(offer)
-
-				c.offerChannel <- nil
-				c.answerChannel <- nil
-				go func() {
-					err := c.exchangeSDP()
-					ctx.So(err, ShouldBeNil)
-					wg.Done()
-				}()
-				answer, err := util.DeserializeSessionDescription(sampleAnswer)
-				So(err, ShouldBeNil)
-				c.answerChannel <- answer
-				wg.Wait()
-			})
-
-		})
 	})
 
 	Convey("Dialers", t, func() {

client/lib/rendezvous.go

@@ -153,7 +153,5 @@ func NewWebRTCDialer(broker *BrokerChannel, iceServers []webrtc.ICEServer) *WebR
 func (w WebRTCDialer) Catch() (*WebRTCPeer, error) {
 	// TODO: [#25591] Fetch ICE server information from Broker.
 	// TODO: [#25596] Consider TURN servers here too.
-	connection := NewWebRTCPeer(w.webrtcConfig, w.BrokerChannel)
+	return NewWebRTCPeer(w.webrtcConfig, w.BrokerChannel)
-	err := connection.Connect()
-	return connection, err
 }

client/lib/webrtc.go

@@ -41,7 +41,7 @@ type WebRTCPeer struct {
 
 // Construct a WebRTC PeerConnection.
 func NewWebRTCPeer(config *webrtc.Configuration,
-	broker *BrokerChannel) *WebRTCPeer {
+	broker *BrokerChannel) (*WebRTCPeer, error) {
 	connection := new(WebRTCPeer)
 	{
 		var buf [8]byte
@@ -60,7 +60,13 @@ func NewWebRTCPeer(config *webrtc.Configuration,
 
 	// Pipes remain the same even when DataChannel gets switched.
 	connection.recvPipe, connection.writePipe = io.Pipe()
-	return connection
+
+	err := connection.connect()
+	if err != nil {
+		connection.Close()
+		return nil, err
+	}
+	return connection, nil
 }
 
 // Read bytes from local SOCKS.
@@ -113,8 +119,7 @@ func (c *WebRTCPeer) checkForStaleness() {
 	}
 }
 
-// As part of |Connector| interface.
+func (c *WebRTCPeer) connect() error {
-func (c *WebRTCPeer) Connect() error {
 	log.Println(c.id, " connecting...")
 	// TODO: When go-webrtc is more stable, it's possible that a new
 	// PeerConnection won't need to be re-prepared each time.