Remove the pending buffer after the data channel call close() by either side.

[jerry-tao]

Summary

Remove the pending buffer after the channel is closed by either side.

Motivation

Please refer to #238

Describe alternatives you've considered

In the official WebRTC code, all pending data will be clear after the data channel call close() by either side.

But the SCTP RFC does not suggest so. The chrome will ACK the closing channel data too.

Based on @enobufs 's work, here is my thought:

• If we call close or receive reconfig, we should transform to closing state.
• When the close reconfig is finished, we transform to closed state.
• During the closing/closed state, the read and write should be rejected with error.
• During the closing/closed state, stop sending buffered data in pendingQueue.

Or we could emit the closing state to the caller ( data channel) to decide what to do.

dc.OnClosing(func(){
   dc.Close()
})

Additional context

During reading the official webrtc code, I found it has more assumptions like:

• Default buffer size for one data channel is 16MB, more data will be rejected.
• The max data channel count is 1024, rather not 65535.

References

• Fix Chrome datachannel stuck at closing #238
• Issue 559394: Closing WebRTC data channels is broken and only works chrome to chrome
  

[enobufs]

In the official WebRTC code, all pending data will be clear after the data channel call close() by either side.

Not true. Just to be clear...
W3C spec says in 6.2.4 Closing procedure:

1. Finish sending all currently pending messages of the channel.
2. Follow the closing procedure defined for the channel's underlying data transport :
    1. In the case of an SCTP-based transport, follow [RFC8831], section 6.7.

But Chrome does not seem to bother sending the pending data when the remote data channel has already called "close()".

[jerry-tao]

@enobufs You are right, I made a mistake in my test code.

So the only change should be:

• If received reconfigs with outgoingReset, clear sending pending buffer.

[enobufs]

I found myself still debating whether the pending data should be discarded when OutgoingResetRequest was already received. Remote sent OutgoingResetRequest ("I am not going to send any more") but not IncomingResetRequest ("please stop sending any more"). I can see RFC8831/6252 had a graceful shutdown (e.g. shutdown(SHUT_WR)) in mind when they were written.

shutdown(SHUT_WR) == OutgoingResetRequest

Interoperability with browsers is important but Pion does not need to follow everything what W3C spec says. Pion could take advantage of being a native library...

[jerry-tao]

Yes, I agree with you that OutgoingReset should only mean shutdown() rather than close.
But in the official lib, the usrsctp does not buffer anything, the SctpTransport only buffer one message, and the real buffer is in data channel.
It's datachannel.close(), not sctpTransport.close(), but in pion, this part of function is built in sctp.
As for me, there is a way I can cancel sending buffer data is OK.

[jerry-tao]

Here are my thoughts,

• The outgoingReset happened in sctp.Stream level, so they could and should keep sending the data until it has no data to send.
• The data channel, on the other hand, defines a closing procedure about what it should do when it receives an outgoingReset.

So my understanding is "clear data channel's buffer when data channel is closed remotely since no one will receive the data".

[enobufs]

Let us consider implementing stream.CloseWrite() in the future if a graceful shutdown is desired.
For now, stream.Close() should close both outgoing (send OutgoingResetRequest) and incoming (set readErr non-nil). If onInboundReset() has already been called, discard the stream's user data in the pending buffer.

Does this diagram make sense to you?

stateDiagram-v2
    Open --> Closing: onInboundStreamReset() called\nSet readErr to io.EOF
    Open --> Closed: Close() called\nSet readErr to io.EOF\nSend all pending user data\nSend OutgoingStreamResetRequest
    Closing --> Closed: Close() called\nSend OutgoingStreamResetRequest\nDiscard pending user data

Loading

NOTE

• onOutgoingStreamResetAcknowledged(): a callback made by association when a response to OutgoingStreamResetRequest has been received
• readErr is always non-nil in Closing state
• onInboundStreamReset() is called by Association when it received OutgoingStreamResetRequest (reconfig).
• Data channel (DCEP in the datachannel package) will call Close() when ReadSCTP() returns an error to fulfill RFC 8831 Sec 6.7.

[jerry-tao]

How about moving cache and close logic to datachannel?
In fact, when we talk about close we are talking about data channel close, the sctp rfc didn't define stream close either.

[jerry-tao]

@enobufs I have tried to implement the discard logic as you say, A, B.

• A: Discard the pending buffer when it popped.

• B: Move the buffer from association to stream.

Only for demonstration, please tell me which one is your preferred, and I will keep working on it.

[enobufs]

Hi Jerry,

Sorry for my late response. I was too busy over the last week...

Good call. I was thinking about the option "B" too. This buffering is for WebRTC API and I believe it does not even belong to SCTP stream. (Relates to #77) We'd need to carefully think about potential risk / drawbacks also.

What is your preference?

One little favor to ask you.. Could you please create two PRs (the options A and B), mark it as [WIP] in the title so that we can review these with comments?

One way to think about this is, if we agree that this buffering is WebRTC specific, we should move this buffer to datachannel (DCEP) package, likely in pion/sctp v2., and focus on "A" for now.

[jerry-tao]

I have created PR #245 and #246 .

I prefer Plan A too, small change, easy to implement.

After finishing this we could add buffer to datachannel first. I will try to finish #245 recently.

[enobufs]

I prefer Plan A too, small change, easy to implement.

Thanks for sharing your opinion. Let's focus on the plan A.

[stv0g]

@enobufs Just wondering: could it be that this PR (commit c0159aa) introduced a regression which makes the unit-test TestAssociation_Shutdown fail?

See https://github.com/pion/sctp/actions/runs/3495811764/jobs/5852996166