From 18eac8375d80da440c6df6976e0beec928d7aa15 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 15:41:30 +0000 Subject: [PATCH 01/13] chore: Update pion/ice fork to resolve goroutine leak --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 90afdb8faf365..e706eb3eb8fbb 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/coder/coder go 1.17 // Required until https://github.com/pion/ice/pull/413 is merged. -replace github.com/pion/ice/v2 => github.com/kylecarbs/ice/v2 v2.1.8-0.20220127013758-526c25708344 +replace github.com/pion/ice/v2 => github.com/kylecarbs/ice/v2 v2.1.8-0.20220127154011-583c185f6503 // Required until https://github.com/hashicorp/terraform-config-inspect/pull/74 is merged. replace github.com/hashicorp/terraform-config-inspect => github.com/kylecarbs/terraform-config-inspect v0.0.0-20211215004401-bbc517866b88 diff --git a/go.sum b/go.sum index add8a4cc62d3c..77d4ec3565ba0 100644 --- a/go.sum +++ b/go.sum @@ -835,8 +835,8 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/ktrysmt/go-bitbucket v0.6.4/go.mod h1:9u0v3hsd2rqCHRIpbir1oP7F58uo5dq19sBYvuMoyQ4= -github.com/kylecarbs/ice/v2 v2.1.8-0.20220127013758-526c25708344 h1:rXpDqMPlbnKASSBFwPrJbT2wEL5jZzIX/i0cvwISxlM= -github.com/kylecarbs/ice/v2 v2.1.8-0.20220127013758-526c25708344/go.mod h1:E5frMpIJ3zzcQiRo+XyT7z1IiAsGc1hDURcVJQUzGWA= +github.com/kylecarbs/ice/v2 v2.1.8-0.20220127154011-583c185f6503 h1:SBIFV+NYeUEIFKztoV6tEBxJKPbBEkXVIacf2bfGzFU= +github.com/kylecarbs/ice/v2 v2.1.8-0.20220127154011-583c185f6503/go.mod h1:E5frMpIJ3zzcQiRo+XyT7z1IiAsGc1hDURcVJQUzGWA= github.com/kylecarbs/terraform-config-inspect v0.0.0-20211215004401-bbc517866b88 h1:tvG/qs5c4worwGyGnbbb4i/dYYLjpFwDMqcIT3awAf8= github.com/kylecarbs/terraform-config-inspect v0.0.0-20211215004401-bbc517866b88/go.mod h1:Z0Nnk4+3Cy89smEbrq+sl1bxc9198gIP4I7wcQF6Kqs= github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348/go.mod h1:B69LEHPfb2qLo0BaaOLcbitczOKLWTsrBG9LczfCD4k= From ebb94b822348cc5abda4c46166787b5b2bb2397b Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 15:54:00 +0000 Subject: [PATCH 02/13] Flush remote too --- peer/conn.go | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index 6eddee070a187..56c791e1da3e2 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -72,7 +72,8 @@ func newWithClientOrServer(servers []webrtc.ICEServer, client bool, opts *ConnOp dcDisconnectChannel: make(chan struct{}), dcFailedChannel: make(chan struct{}), localCandidateChannel: make(chan webrtc.ICECandidateInit), - pendingCandidates: make([]webrtc.ICECandidateInit, 0), + pendingLocalCandidates: make([]webrtc.ICECandidateInit, 0), + pendingRemoteCandidates: make([]webrtc.ICECandidateInit, 0), localSessionDescriptionChannel: make(chan webrtc.SessionDescription), remoteSessionDescriptionChannel: make(chan webrtc.SessionDescription), } @@ -120,7 +121,8 @@ type Conn struct { localSessionDescriptionChannel chan webrtc.SessionDescription remoteSessionDescriptionChannel chan webrtc.SessionDescription - pendingCandidates []webrtc.ICECandidateInit + pendingLocalCandidates []webrtc.ICECandidateInit + pendingRemoteCandidates []webrtc.ICECandidateInit pendingCandidatesMutex sync.Mutex pendingCandidatesFlushed bool @@ -147,7 +149,7 @@ func (c *Conn) init() error { if !c.pendingCandidatesFlushed { c.opts.Logger.Debug(context.Background(), "adding local candidate to buffer") - c.pendingCandidates = append(c.pendingCandidates, iceCandidate.ToJSON()) + c.pendingLocalCandidates = append(c.pendingLocalCandidates, iceCandidate.ToJSON()) return } c.opts.Logger.Debug(context.Background(), "adding local candidate") @@ -291,7 +293,10 @@ func (c *Conn) negotiate() { if c.offerrer { // ICE candidates reset when an offer/answer is set for the first // time. If candidates flush before this point, a connection could fail. - c.flushPendingCandidates() + err = c.flushPendingCandidates() + if err != nil { + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + } } if !c.offerrer { @@ -315,26 +320,39 @@ func (c *Conn) negotiate() { } // Wait until the local description is set to flush candidates. - c.flushPendingCandidates() + err = c.flushPendingCandidates() + if err != nil { + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + } } } // flushPendingCandidates writes all local candidates to the candidate send channel. // The localCandidateChannel is expected to be serviced, otherwise this could block. -func (c *Conn) flushPendingCandidates() { +func (c *Conn) flushPendingCandidates() error { c.pendingCandidatesMutex.Lock() defer c.pendingCandidatesMutex.Unlock() - for _, pendingCandidate := range c.pendingCandidates { + for _, pendingCandidate := range c.pendingLocalCandidates { c.opts.Logger.Debug(context.Background(), "flushing local candidate") select { case <-c.closed: - return + return nil case c.localCandidateChannel <- pendingCandidate: } } - c.pendingCandidates = make([]webrtc.ICECandidateInit, 0) + + for _, pendingCandidate := range c.pendingRemoteCandidates { + c.opts.Logger.Debug(context.Background(), "flushing remote candidate") + err := c.rtc.AddICECandidate(pendingCandidate) + if err != nil { + return err + } + } + + c.pendingLocalCandidates = make([]webrtc.ICECandidateInit, 0) c.pendingCandidatesFlushed = true c.opts.Logger.Debug(context.Background(), "flushed candidates") + return nil } // LocalCandidate returns a channel that emits when a local candidate @@ -345,6 +363,13 @@ func (c *Conn) LocalCandidate() <-chan webrtc.ICECandidateInit { // AddRemoteCandidate adds a remote candidate to the RTC connection. func (c *Conn) AddRemoteCandidate(i webrtc.ICECandidateInit) error { + c.pendingCandidatesMutex.Lock() + defer c.pendingCandidatesMutex.Unlock() + if !c.pendingCandidatesFlushed { + c.opts.Logger.Debug(context.Background(), "adding remote candidate to buffer") + c.pendingRemoteCandidates = append(c.pendingRemoteCandidates, i) + return nil + } c.opts.Logger.Debug(context.Background(), "adding remote candidate") return c.rtc.AddICECandidate(i) } From de209c7e1a944c16aba15ce9119536ec383af603 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 15:59:12 +0000 Subject: [PATCH 03/13] Add logs for setting the description --- peer/conn.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/peer/conn.go b/peer/conn.go index 56c791e1da3e2..b1c043c460c82 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -264,6 +264,7 @@ func (c *Conn) negotiate() { _ = c.CloseWithError(xerrors.Errorf("create offer: %w", err)) return } + c.opts.Logger.Debug(context.Background(), "setting local description") err = c.rtc.SetLocalDescription(offer) if err != nil { _ = c.CloseWithError(xerrors.Errorf("set local description: %w", err)) @@ -283,6 +284,7 @@ func (c *Conn) negotiate() { case remoteDescription = <-c.remoteSessionDescriptionChannel: } + c.opts.Logger.Debug(context.Background(), "setting remote description") err := c.rtc.SetRemoteDescription(remoteDescription) if err != nil { c.pendingCandidatesMutex.Unlock() @@ -305,6 +307,7 @@ func (c *Conn) negotiate() { _ = c.CloseWithError(xerrors.Errorf("create answer: %w", err)) return } + c.opts.Logger.Debug(context.Background(), "setting local description") err = c.rtc.SetLocalDescription(answer) if err != nil { _ = c.CloseWithError(xerrors.Errorf("set local description: %w", err)) @@ -350,6 +353,7 @@ func (c *Conn) flushPendingCandidates() error { } c.pendingLocalCandidates = make([]webrtc.ICECandidateInit, 0) + c.pendingRemoteCandidates = make([]webrtc.ICECandidateInit, 0) c.pendingCandidatesFlushed = true c.opts.Logger.Debug(context.Background(), "flushed candidates") return nil From 6f7e3af78d6ac84d16b14c32699a2870a8c20d5f Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 16:59:03 +0000 Subject: [PATCH 04/13] Try locking only on remote --- peer/conn.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index b1c043c460c82..e7c01c9c50963 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -287,18 +287,14 @@ func (c *Conn) negotiate() { c.opts.Logger.Debug(context.Background(), "setting remote description") err := c.rtc.SetRemoteDescription(remoteDescription) if err != nil { - c.pendingCandidatesMutex.Unlock() _ = c.CloseWithError(xerrors.Errorf("set remote description (closed %v): %w", c.isClosed(), err)) return } - - if c.offerrer { - // ICE candidates reset when an offer/answer is set for the first - // time. If candidates flush before this point, a connection could fail. - err = c.flushPendingCandidates() - if err != nil { - _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) - } + // ICE candidates reset when an offer/answer is set for the first + // time. If candidates flush before this point, a connection could fail. + err = c.flushPendingCandidates() + if err != nil { + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) } if !c.offerrer { @@ -321,12 +317,6 @@ func (c *Conn) negotiate() { return case c.localSessionDescriptionChannel <- answer: } - - // Wait until the local description is set to flush candidates. - err = c.flushPendingCandidates() - if err != nil { - _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) - } } } From a918c9360ef7d150e597c42e97323fe1d1a306b3 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 17:07:14 +0000 Subject: [PATCH 05/13] Remove local bufferring in favor of remote --- peer/conn.go | 50 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 37 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index e7c01c9c50963..593582e084923 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -72,7 +72,6 @@ func newWithClientOrServer(servers []webrtc.ICEServer, client bool, opts *ConnOp dcDisconnectChannel: make(chan struct{}), dcFailedChannel: make(chan struct{}), localCandidateChannel: make(chan webrtc.ICECandidateInit), - pendingLocalCandidates: make([]webrtc.ICECandidateInit, 0), pendingRemoteCandidates: make([]webrtc.ICECandidateInit, 0), localSessionDescriptionChannel: make(chan webrtc.SessionDescription), remoteSessionDescriptionChannel: make(chan webrtc.SessionDescription), @@ -121,7 +120,6 @@ type Conn struct { localSessionDescriptionChannel chan webrtc.SessionDescription remoteSessionDescriptionChannel chan webrtc.SessionDescription - pendingLocalCandidates []webrtc.ICECandidateInit pendingRemoteCandidates []webrtc.ICECandidateInit pendingCandidatesMutex sync.Mutex pendingCandidatesFlushed bool @@ -144,14 +142,6 @@ func (c *Conn) init() error { if iceCandidate == nil { return } - c.pendingCandidatesMutex.Lock() - defer c.pendingCandidatesMutex.Unlock() - - if !c.pendingCandidatesFlushed { - c.opts.Logger.Debug(context.Background(), "adding local candidate to buffer") - c.pendingLocalCandidates = append(c.pendingLocalCandidates, iceCandidate.ToJSON()) - return - } c.opts.Logger.Debug(context.Background(), "adding local candidate") select { case <-c.closed: @@ -290,12 +280,20 @@ func (c *Conn) negotiate() { _ = c.CloseWithError(xerrors.Errorf("set remote description (closed %v): %w", c.isClosed(), err)) return } - // ICE candidates reset when an offer/answer is set for the first - // time. If candidates flush before this point, a connection could fail. - err = c.flushPendingCandidates() - if err != nil { - _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + // The ICE transport resets when the remote description is updated. + // Adding ICE candidates before this point causes a failed connection, + // because the candidate would be lost. + c.pendingCandidatesMutex.Lock() + for _, pendingCandidate := range c.pendingRemoteCandidates { + c.opts.Logger.Debug(context.Background(), "flushing remote candidate") + err := c.rtc.AddICECandidate(pendingCandidate) + if err != nil { + c.pendingCandidatesMutex.Unlock() + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + return + } } + c.pendingCandidatesMutex.Unlock() if !c.offerrer { answer, err := c.rtc.CreateAnswer(&webrtc.AnswerOptions{}) @@ -323,29 +321,7 @@ func (c *Conn) negotiate() { // flushPendingCandidates writes all local candidates to the candidate send channel. // The localCandidateChannel is expected to be serviced, otherwise this could block. func (c *Conn) flushPendingCandidates() error { - c.pendingCandidatesMutex.Lock() - defer c.pendingCandidatesMutex.Unlock() - for _, pendingCandidate := range c.pendingLocalCandidates { - c.opts.Logger.Debug(context.Background(), "flushing local candidate") - select { - case <-c.closed: - return nil - case c.localCandidateChannel <- pendingCandidate: - } - } - - for _, pendingCandidate := range c.pendingRemoteCandidates { - c.opts.Logger.Debug(context.Background(), "flushing remote candidate") - err := c.rtc.AddICECandidate(pendingCandidate) - if err != nil { - return err - } - } - c.pendingLocalCandidates = make([]webrtc.ICECandidateInit, 0) - c.pendingRemoteCandidates = make([]webrtc.ICECandidateInit, 0) - c.pendingCandidatesFlushed = true - c.opts.Logger.Debug(context.Background(), "flushed candidates") return nil } From 44cad06fc9b09c45556371c6b2b62edee3a64d72 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 17:08:25 +0000 Subject: [PATCH 06/13] Remove unused flush func --- peer/conn.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index 593582e084923..8b0b8af03b4d1 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -318,13 +318,6 @@ func (c *Conn) negotiate() { } } -// flushPendingCandidates writes all local candidates to the candidate send channel. -// The localCandidateChannel is expected to be serviced, otherwise this could block. -func (c *Conn) flushPendingCandidates() error { - - return nil -} - // LocalCandidate returns a channel that emits when a local candidate // needs to be exchanged with a remote connection. func (c *Conn) LocalCandidate() <-chan webrtc.ICECandidateInit { From d62797e02d8be67b90d5e3fb62bfbbe2934546e4 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 17:13:12 +0000 Subject: [PATCH 07/13] Set candidates flushed to true --- peer/conn.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/peer/conn.go b/peer/conn.go index 8b0b8af03b4d1..24a0ef12a60fd 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -293,6 +293,8 @@ func (c *Conn) negotiate() { return } } + c.pendingCandidatesFlushed = true + c.opts.Logger.Debug(context.Background(), "flushed remote candidates") c.pendingCandidatesMutex.Unlock() if !c.offerrer { From 982fde3ef787ca939ad0f6e42cb35053de494f31 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 19:10:21 +0000 Subject: [PATCH 08/13] Defer flush until the end of negotiation --- peer/conn.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index 24a0ef12a60fd..474f31f3fbd54 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -280,22 +280,6 @@ func (c *Conn) negotiate() { _ = c.CloseWithError(xerrors.Errorf("set remote description (closed %v): %w", c.isClosed(), err)) return } - // The ICE transport resets when the remote description is updated. - // Adding ICE candidates before this point causes a failed connection, - // because the candidate would be lost. - c.pendingCandidatesMutex.Lock() - for _, pendingCandidate := range c.pendingRemoteCandidates { - c.opts.Logger.Debug(context.Background(), "flushing remote candidate") - err := c.rtc.AddICECandidate(pendingCandidate) - if err != nil { - c.pendingCandidatesMutex.Unlock() - _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) - return - } - } - c.pendingCandidatesFlushed = true - c.opts.Logger.Debug(context.Background(), "flushed remote candidates") - c.pendingCandidatesMutex.Unlock() if !c.offerrer { answer, err := c.rtc.CreateAnswer(&webrtc.AnswerOptions{}) @@ -318,6 +302,22 @@ func (c *Conn) negotiate() { case c.localSessionDescriptionChannel <- answer: } } + + // The ICE transport resets when the remote description is updated. + // Adding ICE candidates before this point causes a failed connection, + // because the candidate would be lost. + c.pendingCandidatesMutex.Lock() + defer c.pendingCandidatesMutex.Unlock() + for _, pendingCandidate := range c.pendingRemoteCandidates { + c.opts.Logger.Debug(context.Background(), "flushing remote candidate") + err := c.rtc.AddICECandidate(pendingCandidate) + if err != nil { + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + return + } + } + c.pendingCandidatesFlushed = true + c.opts.Logger.Debug(context.Background(), "flushed remote candidates") } // LocalCandidate returns a channel that emits when a local candidate From e154d6eed8b07b3f0e35f644d4d7222000773237 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 19:17:16 +0000 Subject: [PATCH 09/13] Buffer ICE candidates --- peer/conn.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index 474f31f3fbd54..01e3026415619 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -71,7 +71,7 @@ func newWithClientOrServer(servers []webrtc.ICEServer, client bool, opts *ConnOp dcOpenChannel: make(chan *webrtc.DataChannel), dcDisconnectChannel: make(chan struct{}), dcFailedChannel: make(chan struct{}), - localCandidateChannel: make(chan webrtc.ICECandidateInit), + localCandidateChannel: make(chan webrtc.ICECandidateInit, 16), pendingRemoteCandidates: make([]webrtc.ICECandidateInit, 0), localSessionDescriptionChannel: make(chan webrtc.SessionDescription), remoteSessionDescriptionChannel: make(chan webrtc.SessionDescription), @@ -281,6 +281,22 @@ func (c *Conn) negotiate() { return } + // The ICE transport resets when the remote description is updated. + // Adding ICE candidates before this point causes a failed connection, + // because the candidate would be lost. + c.pendingCandidatesMutex.Lock() + defer c.pendingCandidatesMutex.Unlock() + for _, pendingCandidate := range c.pendingRemoteCandidates { + c.opts.Logger.Debug(context.Background(), "flushing remote candidate") + err := c.rtc.AddICECandidate(pendingCandidate) + if err != nil { + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + return + } + } + c.pendingCandidatesFlushed = true + c.opts.Logger.Debug(context.Background(), "flushed remote candidates") + if !c.offerrer { answer, err := c.rtc.CreateAnswer(&webrtc.AnswerOptions{}) if err != nil { @@ -302,22 +318,6 @@ func (c *Conn) negotiate() { case c.localSessionDescriptionChannel <- answer: } } - - // The ICE transport resets when the remote description is updated. - // Adding ICE candidates before this point causes a failed connection, - // because the candidate would be lost. - c.pendingCandidatesMutex.Lock() - defer c.pendingCandidatesMutex.Unlock() - for _, pendingCandidate := range c.pendingRemoteCandidates { - c.opts.Logger.Debug(context.Background(), "flushing remote candidate") - err := c.rtc.AddICECandidate(pendingCandidate) - if err != nil { - _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) - return - } - } - c.pendingCandidatesFlushed = true - c.opts.Logger.Debug(context.Background(), "flushed remote candidates") } // LocalCandidate returns a channel that emits when a local candidate From a2a4da4f70ffbedacc80f87c09da5380abc30af6 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 19:36:59 +0000 Subject: [PATCH 10/13] Add comment clarifying channel buffer --- peer/conn.go | 20 +++++++++++--------- peer/conn_test.go | 4 +++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index 01e3026415619..a9846358aea97 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -62,15 +62,17 @@ func newWithClientOrServer(servers []webrtc.ICEServer, client bool, opts *ConnOp return nil, xerrors.Errorf("create peer connection: %w", err) } conn := &Conn{ - pingChannelID: 1, - pingEchoChannelID: 2, - opts: opts, - rtc: rtc, - offerrer: client, - closed: make(chan struct{}), - dcOpenChannel: make(chan *webrtc.DataChannel), - dcDisconnectChannel: make(chan struct{}), - dcFailedChannel: make(chan struct{}), + pingChannelID: 1, + pingEchoChannelID: 2, + opts: opts, + rtc: rtc, + offerrer: client, + closed: make(chan struct{}), + dcOpenChannel: make(chan *webrtc.DataChannel), + dcDisconnectChannel: make(chan struct{}), + dcFailedChannel: make(chan struct{}), + // This channel needs to be bufferred otherwise slow consumers + // of this will cause a connection failure. localCandidateChannel: make(chan webrtc.ICECandidateInit, 16), pendingRemoteCandidates: make([]webrtc.ICECandidateInit, 0), localSessionDescriptionChannel: make(chan webrtc.SessionDescription), diff --git a/peer/conn_test.go b/peer/conn_test.go index f964a61bfe832..57bc130711823 100644 --- a/peer/conn_test.go +++ b/peer/conn_test.go @@ -48,7 +48,9 @@ var ( ) func TestMain(m *testing.M) { - goleak.VerifyTestMain(m) + // pion/ice doesn't properly close immediately. The solution for this isn't yet known. See: + // https://github.com/pion/ice/pull/413 + goleak.VerifyTestMain(m, goleak.IgnoreTopFunction("github.com/pion/ice/v2.(*Agent).startOnConnectionStateChangeRoutine.func1 ")) } func TestConn(t *testing.T) { From aa1e664e4ac6ead63dae02d5ffb5700e4f5ecba0 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 19:46:57 +0000 Subject: [PATCH 11/13] Flush after handshake --- peer/conn.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/peer/conn.go b/peer/conn.go index a9846358aea97..fa6081c1ab8c6 100644 --- a/peer/conn.go +++ b/peer/conn.go @@ -283,22 +283,6 @@ func (c *Conn) negotiate() { return } - // The ICE transport resets when the remote description is updated. - // Adding ICE candidates before this point causes a failed connection, - // because the candidate would be lost. - c.pendingCandidatesMutex.Lock() - defer c.pendingCandidatesMutex.Unlock() - for _, pendingCandidate := range c.pendingRemoteCandidates { - c.opts.Logger.Debug(context.Background(), "flushing remote candidate") - err := c.rtc.AddICECandidate(pendingCandidate) - if err != nil { - _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) - return - } - } - c.pendingCandidatesFlushed = true - c.opts.Logger.Debug(context.Background(), "flushed remote candidates") - if !c.offerrer { answer, err := c.rtc.CreateAnswer(&webrtc.AnswerOptions{}) if err != nil { @@ -320,6 +304,22 @@ func (c *Conn) negotiate() { case c.localSessionDescriptionChannel <- answer: } } + + // The ICE transport resets when the remote description is updated. + // Adding ICE candidates before this point causes a failed connection, + // because the candidate would be lost. + c.pendingCandidatesMutex.Lock() + defer c.pendingCandidatesMutex.Unlock() + for _, pendingCandidate := range c.pendingRemoteCandidates { + c.opts.Logger.Debug(context.Background(), "flushing remote candidate") + err := c.rtc.AddICECandidate(pendingCandidate) + if err != nil { + _ = c.CloseWithError(xerrors.Errorf("flush pending candidates: %w", err)) + return + } + } + c.pendingCandidatesFlushed = true + c.opts.Logger.Debug(context.Background(), "flushed remote candidates") } // LocalCandidate returns a channel that emits when a local candidate From 7b3e89d2814acc517c4dd8ebb7ec569f3ec1c32a Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 20:10:08 +0000 Subject: [PATCH 12/13] Move away from fork --- go.mod | 3 --- go.sum | 7 +++++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index e706eb3eb8fbb..7505a34a7167c 100644 --- a/go.mod +++ b/go.mod @@ -2,9 +2,6 @@ module github.com/coder/coder go 1.17 -// Required until https://github.com/pion/ice/pull/413 is merged. -replace github.com/pion/ice/v2 => github.com/kylecarbs/ice/v2 v2.1.8-0.20220127154011-583c185f6503 - // Required until https://github.com/hashicorp/terraform-config-inspect/pull/74 is merged. replace github.com/hashicorp/terraform-config-inspect => github.com/kylecarbs/terraform-config-inspect v0.0.0-20211215004401-bbc517866b88 diff --git a/go.sum b/go.sum index 77d4ec3565ba0..564c6aaae7be9 100644 --- a/go.sum +++ b/go.sum @@ -835,8 +835,6 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/ktrysmt/go-bitbucket v0.6.4/go.mod h1:9u0v3hsd2rqCHRIpbir1oP7F58uo5dq19sBYvuMoyQ4= -github.com/kylecarbs/ice/v2 v2.1.8-0.20220127154011-583c185f6503 h1:SBIFV+NYeUEIFKztoV6tEBxJKPbBEkXVIacf2bfGzFU= -github.com/kylecarbs/ice/v2 v2.1.8-0.20220127154011-583c185f6503/go.mod h1:E5frMpIJ3zzcQiRo+XyT7z1IiAsGc1hDURcVJQUzGWA= github.com/kylecarbs/terraform-config-inspect v0.0.0-20211215004401-bbc517866b88 h1:tvG/qs5c4worwGyGnbbb4i/dYYLjpFwDMqcIT3awAf8= github.com/kylecarbs/terraform-config-inspect v0.0.0-20211215004401-bbc517866b88/go.mod h1:Z0Nnk4+3Cy89smEbrq+sl1bxc9198gIP4I7wcQF6Kqs= github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348/go.mod h1:B69LEHPfb2qLo0BaaOLcbitczOKLWTsrBG9LczfCD4k= @@ -1014,8 +1012,12 @@ github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi github.com/pierrec/lz4/v4 v4.1.8/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4= github.com/pion/datachannel v1.5.2 h1:piB93s8LGmbECrpO84DnkIVWasRMk3IimbcXkTQLE6E= github.com/pion/datachannel v1.5.2/go.mod h1:FTGQWaHrdCwIJ1rw6xBIfZVkslikjShim5yr05XFuCQ= +github.com/pion/dtls/v2 v2.0.13/go.mod h1:OaE7eTM+ppaUhJ99OTO4aHl9uY6vPrT1gPY27uNTxRY= github.com/pion/dtls/v2 v2.1.0 h1:g6gtKVNLp6URDkv9OijFJl16kqGHzVzZG+Fa4A38GTY= github.com/pion/dtls/v2 v2.1.0/go.mod h1:qG3gA7ZPZemBqpEFqRKyURYdKEwFZQCGb7gv9T3ON3Y= +github.com/pion/ice/v2 v2.1.18/go.mod h1:9jDr0iIUg8P6+0Jq8QJ/eFSkX3JnsPd293TjCdkfpTs= +github.com/pion/ice/v2 v2.1.19 h1:z7iVx/fHlqvPILUbvcj1xjuz/6eVKgEFOM8h1AuLbF8= +github.com/pion/ice/v2 v2.1.19/go.mod h1:E5frMpIJ3zzcQiRo+XyT7z1IiAsGc1hDURcVJQUzGWA= github.com/pion/interceptor v0.1.6/go.mod h1:Lh3JSl/cbJ2wP8I3ccrjh1K/deRGRn3UlSPuOTiHb6U= github.com/pion/interceptor v0.1.7 h1:HThW0tIIKT9RRoDWGURe8rlZVOx0fJHxBHpA0ej0+bo= github.com/pion/interceptor v0.1.7/go.mod h1:Lh3JSl/cbJ2wP8I3ccrjh1K/deRGRn3UlSPuOTiHb6U= @@ -1311,6 +1313,7 @@ golang.org/x/crypto v0.0.0-20210616213533-5ff15b29337e/go.mod h1:GvvjBRRGRdwPK5y golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.0.0-20210817164053-32db794688a5/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/crypto v0.0.0-20211117183948-ae814b36b871/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.0.0-20220126234351-aa10faf2a1f8 h1:kACShD3qhmr/3rLmg1yXyt+N4HcwutKyPRB93s54TIU= golang.org/x/crypto v0.0.0-20220126234351-aa10faf2a1f8/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= From f3717ca263a2f4c1ad2e204be3c54406143390df Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 27 Jan 2022 20:14:21 +0000 Subject: [PATCH 13/13] Ignore pion/ice leaks --- peer/conn_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/peer/conn_test.go b/peer/conn_test.go index 57bc130711823..a1fd4d7a4c085 100644 --- a/peer/conn_test.go +++ b/peer/conn_test.go @@ -50,7 +50,12 @@ var ( func TestMain(m *testing.M) { // pion/ice doesn't properly close immediately. The solution for this isn't yet known. See: // https://github.com/pion/ice/pull/413 - goleak.VerifyTestMain(m, goleak.IgnoreTopFunction("github.com/pion/ice/v2.(*Agent).startOnConnectionStateChangeRoutine.func1 ")) + goleak.VerifyTestMain(m, + goleak.IgnoreTopFunction("github.com/pion/ice/v2.(*Agent).startOnConnectionStateChangeRoutine.func1"), + goleak.IgnoreTopFunction("github.com/pion/ice/v2.(*Agent).startOnConnectionStateChangeRoutine.func2"), + goleak.IgnoreTopFunction("github.com/pion/ice/v2.(*Agent).taskLoop"), + goleak.IgnoreTopFunction("internal/poll.runtime_pollWait"), + ) } func TestConn(t *testing.T) {