Open marten-seemann opened 3 years ago
Hi, I tried to debug it to identify the issue. As I don't have much knowledge about how all this works, I couldn't find the solution. One of the major cause of this issue seems to be caused by the similar code, where after closing the MuxedStream, the test suite calls ioutil.ReadAll, which causes error. I tried to comment out the following section of code to confirm it, and the tests were passing. Obviously, this isn't a legit way to do, but the purpose was only to identify the cause. If I get some proper guidance(directions) from you guys, then I would be able to resolve it
https://github.com/libp2p/go-libp2p-testing/blob/master/suites/transport/transport_suite.go#L144
err = s.Close()
if err != nil {
t.Fatal(err)
return
}
buf, err := ioutil.ReadAll(s)
if err != nil {
t.Fatal(err)
return
}
if !bytes.Equal(testData, buf) {
t.Errorf("expected %s, got %s", testData, buf)
}
That does seems strange to me. According to https://pkg.go.dev/github.com/libp2p/go-libp2p-core@v0.8.5/mux#MuxedStream:
Close closes the stream. ... Future reads will fail.
@raulk could you give us some clarity on this?
That's correct. Close
is equivalent to calling both CloseRead
and CloseWrite
.
By calling CloseRead
, you declare "I'm done reading from this stream". If you just want to close the write side of the stream, only call CloseWrite
, not Close
.
Ok, does that mean the go-libp2p-testing suite is violating this behavior or are we overlooking something? It seems to first close the steam and then try to read it using ioutil.ReadAll
.
Maybe this was meant to be a deferred close?
A deferred close won't work. We need to close the write side of the stream, so the peer's ReadAll
that's copying the data returns. So the right order would be: CloseWrite
, ReadAll
, CloseRead
(or Close
).
Yea, makes sense. This means:
Also please have a look at this DataChannel's Close() Implementation It indirectly calls sctp.Stream's Close() Thanks :)
Actually, point 2 in my previous comment may not apply. This transport is currently not meant to do stream muxing by itself. Is is supposed to use an external muxer for that: https://github.com/libp2p/go-libp2p-webrtc-direct/blob/master/webrtcdirect_test.go#L20. There is some stream muxing code in this repo. I originally built it because it made sense to me. However, I abandoned it because the JS counterpart also doesn't do stream muxing internally. It only implements the 'conn' interface on WebRTC level. This is open for discussion in #9. The internal muxing logic is disabled when an external muxer is provided: https://github.com/libp2p/go-libp2p-webrtc-direct/blob/master/conn.go#L199
So maybe fixing point 1 in my previous comment could be enough to fix the tests.
When updating go-libp2p-testing, the transport tests fail: https://github.com/libp2p/go-libp2p-webrtc-direct/blob/0a5a6f156e02ee34d3ead176542e72de5c541954/webrtcdirect_test.go#L14-L29