webrtc: run onDone callback immediately on close #2729
Conversation
sukunrt
force-pushed
the
webrtc-cleanup-quickly
branch
2 times, most recently
from
138f0a2
to
da66248
Compare
sukunrt
force-pushed
the
webrtc-cleanup-quickly
branch
from
da66248
to
c192570
Compare
p2p/transport/webrtc/stream_test.go
Outdated
| @@ -98,6 +100,50 @@ func getDetachedDataChannels(t *testing.T) (detachedChan, detachedChan) { | |||
| return <-answerChan, <-offerRWCChan | |||
| } | |||
|
|
|||
| // checkDataChannelClosed checks if the datachannel has been closed. | |||
There was a problem hiding this comment.
This comment is wrong, can you please update it to describe checkDataChannelOpen?
p2p/transport/webrtc/stream_test.go
Outdated
| // It sends empty messages on the data channel to check if the channel is still open. | ||
| // The control message reader goroutine depends on exclusive access to datachannel.Read | ||
| // so we have to depend on Write to determine whether the channel has been closed. | ||
| func checkDataChannelOpen(t *testing.T, dc *datachannel.DataChannel) { |
There was a problem hiding this comment.
nit rename these to assertDataChannel{Open,Closed}. They don't return a value they assert some state and fail the test otherwise.
|
We modified the specs to do a synchronous datachannel close after finding out that chrome implementation of datachannels drop enqueued messages if the datachannel is closed. see libp2p/specs#575 (comment) When implementing this spec change, I implemented this such that the stream resource manager scope is closed only after reading the FIN_ACK. This required a rather ugly hack in swarm_stream by introducing a asyncCloser interface. See this comment: #2615 (comment) After implementing this, I realized that the datachannel close procedure itself was a synchronous close where SCTP layer doesn't drop references to the stream till it gets a datachannel close message from the other end. So calling onDone in a separate go routine immediately after data channel close didn't help anything since the reference to the data channel would be kept for 1RTT. I removed the |
|
Does this mean that the resource manager is off from the true resource usage? For example, if a client closes a stream and the on close is called immediately, the resource manager frees that stream, but that memory is actually still reserved by pion until we finish the close. Is that correct? |
|
Yes, it'll be freed only on datachannel close which happens 1RTT after we call |
|
Is it significantly harder to call the onDone callback after we close the datachannel? (and thus free the resource token in the resource manager) That seems to me the most correct solution because it protects against a hypothetical memory exhaustion attack where a server can have a client think it has freed its resource usage to a stream but actually still have that memory reserved. Unless I'm misunderstanding something and we get this protection some other way? |
No description provided.