Revert "Remove pending buffer when stream closed"

[edaniels]

This reverts commit c0159aa.

@jerry-tao @enobufs, the reverted commit made sense in theory but it's causing a regression in what https://github.com/pion/webrtc appears to expect in terms of data channel closing behavior with respect to pending writes on the sender side. More specifically, TestEOF/Detach started to fail where we expected that when the sender did a write followed by close, that the reader would see those writes and then see an EOF. That is to say, the enqueued writes are lost due to these changes. It doesn't seem like this change fixed any correctness issues with regards to the spec so it seems like we should revert it so as to unblock other changes in this v1.8.x. v1.8.4 is currently published but is now broken/flaky. #254 attempted to fix this but it introduced more issues due to how we lose a lot of context when we Close in that we don't know why we are closing (inbound reset or user requested).

[codecov]

Codecov Report

Base: 80.74% // Head: 80.87% // Increases project coverage by +0.13% 🎉

Coverage data is based on head (eeff761) compared to base (57a04ed).
Patch coverage: 85.71% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   80.74%   80.87%   +0.13%     
==========================================
  Files          48       48              
  Lines        4046     4032      -14     
==========================================
- Hits         3267     3261       -6     
+ Misses        636      629       -7     
+ Partials      143      142       -1     

Flag	Coverage Δ
go	80.87% <85.71%> (+0.13%)	⬆️
wasm	67.46% <85.71%> (+0.92%)	⬆️

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

Impacted Files	Coverage Δ
chunk_payload_data.go	72.83% <ø> (ø)
association.go	84.79% <25.00%> (+0.10%)	⬆️
stream.go	92.70% <100.00%> (+1.55%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[edaniels]

I reverted for one commit and back in master to unblock other feature work.

[edaniels]

If we're okay with how this is breaking the webrtc tests, then I think we should just go update the TestEOF/Detach test in datachannel_go_test.go to use channel synchronization to wait for the read of this is some test data to happen prior to the sender side closing; otherwise the test will be flaky. I do worry that this behavior though major version like breakage due to its subtle behavior in being a full close versus a half-close

[enobufs]

Hi @edaniels I wasn't aware v1.8.4 caused an error at pion/webrtc. I should have tested it earlier. I agree, we should revert it for now. I can see you have already done it with v1.8.5 - nice. I will revisit this and look into the affected test later.

[jerry-tao]

LGTM.

[jerry-tao]

As we discussed in #245, the commit does not fully resolve the origin issue either. As I said in #239 , the pion/sctp is more like webrtc/sctp_transport+webrtc/datachannel+libsrtcp.
Maybe in v2.0 we could split the sctp_transport and datachannel logic to pion/datachannel.
As for the correctness, this bothers me too, should we have the same logic with C++ webrtc/src or implements it according to RFC with personal understanding?

[edaniels]

Yeah I think splitting it up to more closely reflect the C++ code modulo idioms would be good. I think the really important bit is what @enobufs inquired about with the RFC coauthor regarding close in the W3C API