Run ops.GracefulClose earlier in pc.Close

[edaniels]

Without this, it increases the chance of races between resources a PeerConnection owns and operations on the ops queue that affect those resources. For example, a SetRemoteDescription that opens an SCTP stream for a data channel could be running while we close down the peer connection. If there were a bug in the SCTP library that incorrectly performs operations after close, this would be an issue (like in pion/sctp#348).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.82%. Comparing base (9ca52b4) to head (6b0a656).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
- Coverage   78.90%   78.82%   -0.09%     
==========================================
  Files          89       89              
  Lines        8452     8452              
==========================================
- Hits         6669     6662       -7     
- Misses       1296     1302       +6     
- Partials      487      488       +1     

Flag	Coverage Δ
go	80.37% <100.00%> (-0.10%)	⬇️
wasm	65.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sukunrt]

I am not too sure of this change. It looks like a stopgap measure where ideally we should be moving all the code in peerconnection.go that does

pc.sctpTransport.lock.Lock()
...
pc.sctpTransport.lock.Unlock()

into sctpTransport and let that object handle the correctness of operations after close.

However, I'm sure it doesn't make anything worse, so it's okay from my side to merge.

Note: we cannot gracefully close datachannels once sctpTransport.Stop has been called.

[edaniels]

I am not too sure of this change. It looks like a stopgap measure where ideally we should be moving all the code in peerconnection.go that does

pc.sctpTransport.lock.Lock()
...
pc.sctpTransport.lock.Unlock()

into sctpTransport and let that object handle the correctness of operations after close.

However, I'm sure it doesn't make anything worse, so it's okay from my side to merge.

Note: we cannot gracefully close datachannels once sctpTransport.Stop has been called.

Why can't they be gracefully closed at that point?

[edaniels]

I am not too sure of this change. It looks like a stopgap measure where ideally we should be moving all the code in peerconnection.go that does

pc.sctpTransport.lock.Lock()
...
pc.sctpTransport.lock.Unlock()

into sctpTransport and let that object handle the correctness of operations after close.

Also I agree but it's a larger change and a little bit more invasive to existing behavior. I'll file an issue! #2867

[sukunrt]

Why can't they be gracefully closed at that point?

I think I was wrong here. Gracefully closing the datachannel will wait for the readloop to complete, which I guess is the point of graceful close.

[edaniels]

Reverted this. There wasn't an accompanying test that would show this fails easily in a race to start and stop ops. Op queue has no way of canceling in progress ops.