Fix concurrent pc.GracefulClose #2887
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2887 +/- ##
==========================================
+ Coverage 78.96% 79.00% +0.03%
==========================================
Files 89 89
Lines 8461 8477 +16
==========================================
+ Hits 6681 6697 +16
- Misses 1294 1295 +1
+ Partials 486 485 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
edaniels
force-pushed
the
graceful_close_self_deadlock_fix
branch
6 times, most recently
from
1ab3574
to
2e5e1bf
Compare
edaniels
changed the title
edaniels
force-pushed
the
graceful_close_self_deadlock_fix
branch
6 times, most recently
from
363bc7b
to
69d2e1a
Compare
edaniels
changed the title
edaniels
force-pushed
the
graceful_close_self_deadlock_fix
branch
from
69d2e1a
to
1215aab
Compare
| // want to make graceful and normal closure one time operations in order | ||
| // to avoid any double closure errors from cropping up. However, there are | ||
| // some overlapping close cases when both normal and graceful close are used | ||
| // that should be idempotent, but be cautioned when writing new close behavior |
There was a problem hiding this comment.
I came into this function thinking that it wouldn't make sense for applications to make concurrent graceful + non-graceful close calls. I think it would be fine to declare that usage undefined.
But allowing for both is obviously better if the idempotency requirement is not burdensome.
peerconnection.go
Outdated
| } | ||
|
|
||
| // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #3) | ||
| pc.signalingState.Set(SignalingStateClosed) | ||
| if shouldGracefullyClose && !isAlreadyGracefullyClosingOrClosed { |
There was a problem hiding this comment.
I suspect this is "belts and suspenders" logic -- but I expect isAlreadyGracefullyClosingOrClosed can only be false here.
Was the risk of double closing the reason this was in its own block rather than being put directly into the doGracefulClose lambda?
There was a problem hiding this comment.
Nope you're right. I think this was some left over boolean logic that can be folded into the lambda now. Nice catch.
Edit: I'll actually just keep the code here and remove the second condition to preserve the early defer logic to prevent against any panics in the many libraries that are subsequently called into here. If there were a lot less going on here in terms of stopping/closing, I would drop it into that lambda or if the function was smaller, use a named return value for err, defer the close logic, and update the error.
| } | ||
|
|
||
| // https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #3) | ||
| pc.signalingState.Set(SignalingStateClosed) |
There was a problem hiding this comment.
I've double checked that the place this line moved to was equivalent.
I suppose it must be the case that once someone calls close and moving the signaling state into SignalingStateClosed-- it's impossible for the signaling state to leave SignalingStateClosed?
Given the old + new code for Close do nothing on subsequent calls.
There was a problem hiding this comment.
So nothings prevents that state from moving out of Closed, but yeah, the logic in here doesn't have move to anything else other than Closed.
edaniels
force-pushed
the
graceful_close_self_deadlock_fix
branch
from
1215aab
to
3989fcf
Compare
|
Got Sean's approval offline |
@dgottlieb found an issue in the wild where if concurrent GracefulCloses happen, my wonky atomic bool logic could go awry and cause a leak of sorts and no actual graceful closure. I took some time to clean up the naming and comments in this code while working on the fix which is mainly to wrap a lock around the two bool operations, but also to have a more stringent series of operations when Graceful and normal close are overlapping.