Make pc.Close wait on spawned goroutines to close #2798
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2798 +/- ##
==========================================
+ Coverage 79.13% 79.18% +0.04%
==========================================
Files 89 89
Lines 8249 8268 +19
==========================================
+ Hits 6528 6547 +19
Misses 1255 1255
Partials 466 466
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
What’s the downside of making this the default behavior? Close takes longer? I would say having two Close methods is worse then blocking in Go. It’s a extra cognitive load on the user |
I'm cool with it being just one 8). Yes it'll just take longer. If it ever locks up for too long, we have a bug, which I prefer to be caught this way. |
|
@Sean-Der updated Close instead. |
edaniels
changed the title
datachannel.go
Outdated
| @@ -457,6 +460,12 @@ func (d *DataChannel) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| if d.readLoopActive != nil { | |||
There was a problem hiding this comment.
What do you think of dropping the nil check or logging a error message? I would rather have a crash/error message over inconsistent behavior.
Is closing a nil chan safe?
There was a problem hiding this comment.
I'm okay doing a check for !d.api.settingEngine.detach.DataChannels instead, which is when we have a read loop. What would you prefer?
| @@ -2268,8 +2291,11 @@ func (pc *PeerConnection) startTransports(iceRole ICERole, dtlsRole DTLSRole, re | |||
| } | |||
|
|
|||
| pc.dtlsTransport.internalOnCloseHandler = func() { | |||
| pc.log.Info("Closing PeerConnection from DTLS CloseNotify") | |||
| if pc.isClosed.get() { | |||
There was a problem hiding this comment.
Is this getting fired multiple times right now?
There was a problem hiding this comment.
It doesn't, but it can get fired from a DTLS closenotify while Close is happening, if the timing is right.
| @@ -2097,6 +2114,19 @@ func (pc *PeerConnection) Close() error { | |||
| // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #11) | |||
| pc.updateConnectionState(pc.ICEConnectionState(), pc.dtlsTransport.State()) | |||
|
|
|||
| // non-canon steps | |||
| pc.sctpTransport.lock.Lock() | |||
There was a problem hiding this comment.
@Sean-Der I moved these steps lower. I also made Close not block since it's not clear to me if that's allowed per the spec. AFAICT, a normal Close waits for both sides to see the reset stream and let reads come through in the meantime.
edaniels
force-pushed
the
clean_close
branch
2 times, most recently
from
d7d555d to
49b7f46
Compare
|
@Sean-Der cleaned up Close and made a blocking/non-blocking version to behave with when/where it's called (PeerConnection.Close (block) or directly (cannot block due to spec)) |
|
Thanks for doing this @edaniels! Hopefully this means one less annoyed user :) Were you doing tests that looked for leaked goroutines in a code base that used Pion? |
|
Yep! It came up for Viam and I saw this years ago and we just ignored it. It's become more important as more sophisticated testing has ramped up. |
|
I'll put up a backport later! |
This ensures no goroutines are running when a PeerConnection is closed. I'd also like to backport this into v3 so that existing users can benefit. Relies on pion/ice#706 (tests will fail before that's in).