libp2p
webrtc: wait for FIN_ACK before closing data channels
part of #2656
This removes the control message queue because that only ever adds 500 bytes to the queue. This will not cause too much HOL Blocking for other streams.
There is no problem with backpressure since sctp writes never actually block. There is no blocking on sctp Write.
To verify:
func TestStreamBackpressure(t *testing.T) {
client, _ := getDetachedDataChannels(t)
clientStr := newStream(client.dc, client.rwc, nil)
b := make([]byte, 1024)
for i := 0; i < 1e7; i++ {
n, err := clientStr.dataChannel.Write(b)
if n != 1024 {
t.Fatalf("failed to write to datachannel")
}
if err != nil {
t.Fatalf("failed to write to datachannel")
}
}
}
I don't have a good idea on how to ensure that we collect the stream handler goroutine and then close the stream rcmgr scope if the transport stream object is responsible for closing the scope and not the swarm stream object.
However a public interface that exposes AsyncClose doesn't seem required either. I've made the assertion private to swarm package. This is subtle action at a distance but at least it makes it clear that we don't want this AsyncClose system to be copied in other transports.
go-libp2p open maintainers call: We have @guillaumemichel 's approval and pending other review comments we will merge this PR. There are other Pion fixes that are necessary to take WebRTC out of experimental as well but merging this will help get the next v0.33 release out the door
@Stebalien I have removed AsyncClose.
The difference between webrtc and yamux/quic is that you're relying on the transport stack to discard all the buffers when you do a Close. This happens with yamux. With quic, the receive side of the stream is not immediately discarded but the peer doesn't get credits to open a new stream till it cleanly finishes the stream after we have sent STOP_SENDING on.
No such mechanism exists in webrtc. There is no limit to streams you can open(At least not in pion) or unilateral stream closing since streams are reusable. In webrtc datachannel.Close is a 1RTT sync procedure over the FIN / FIN_ACK closing that we do. So an effective AsyncClose procedure needs to do it after the datachannel is Closed and not after a FIN_ACK is received. Moreover pion never drops a reference to the datachannel(including all the transport layer allocated buffers) till the connection is open. So a system like this only makes sense if we fix: https://github.com/pion/webrtc/issues/2672
There is one way to handle this other than doing an explicit async close without waiting for 1RTT which would require us to check a misbehaving peer which is forcing us to keep all the datachannels open and just close the connection to such a peer. We can discuss that strategy in a separate issue. That'd also require https://github.com/pion/webrtc/issues/2672
With quic, the receive side of the stream is not immediately discarded but the peer doesn't get credits to open a new stream till it cleanly finishes the stream after we have sent STOP_SENDING on.
Can't we do the same thing here? My objection is adding this hack to libp2p itself as this is a transport specific detail.
My objection is adding this hack to libp2p itself as this is a transport specific detail.
That's a fair point. I'll open an issue to discuss.
@Stebalien
Why not just call closeAndRemoveStream immediately?
I have addressed this and removed the AsyncClose logic. Is it fine to merge?
I have addressed your concerns here. I want to do a release today. Happy to take any comments in a follow up PR and if something major is missed we can do a patch release.
Nope, LGTM!