webrtc: wait for FIN_ACK before closing data channels

[sukunrt]

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")
		}
	}
}

[marten-seemann]

Partial review.

Discussed offline: If we move closing of the stream scope to the transports, we don't need to introduce the AsyncCloser interface. We need to decide if that structure makes sense first.

[marten-seemann]

Add a comment?

[sukunrt]

@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: pion/webrtc#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 pion/webrtc#2672

[Stebalien]

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.

[sukunrt]

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.

[sukunrt]

@Stebalien

Why not just call closeAndRemoveStream immediately?

I have addressed this and removed the AsyncClose logic. Is it fine to merge?

[Stebalien]

Some comments, mostly questions, otherwise LGTM.

[Stebalien]

nit: not related, but this locking is strange. We could write this as:

s.mx.Unlock()
var msg pb.Message
err := s.reader.ReadMsg(&msg)
s.mx.Lock()
if err != nil {
    // ...
}
// no second "lock" call needed.

[sukunrt]

This is much better. Thanks!

[Stebalien]

This seems like a strange way to do this. It'll also be racy (e.g., if the user calls the same function to change the read deadline).

Do we need to change the read deadline? If this is the only way to do this, let's add something to SetReadDeadline so it can't be changed again by the user.

[sukunrt]

This is the only way to unblock a waiting reader. Even closing the datachannel won't unblock it till 1RTT later when the peer replies with a close datachannel on its part.

I've changed this to make SetReadDeadline a noop after the read half is closed so the user can't change this again.

[Stebalien]

Future cleanup: make some form of notifyStateChanged function.

[sukunrt]

I've replaced the three channels with 1 writeStateChanged and using a method to notify on the channel removing all the select blocks everywhere.

[Stebalien]

same race comment as above.

[Stebalien]

Is this the rand you want to use?

[sukunrt]

I did mostly for this: golang/go#21835

But looks like math/rand.Read is deprecated and the recommendation is to always use crypto.Read. So using that now.

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.

[Stebalien]

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!