transport tests failing

[marten-seemann]

When updating go-libp2p-testing, the transport tests fail:

go-libp2p-webrtc-direct/webrtcdirect_test.go

Lines 14 to 29 in 0a5a6f1

	func TestTransport(t *testing.T) {
	logging.SetLogLevel("*", "warning")
	ta := NewTransport(
	webrtc.Configuration{},
	new(mplex.Transport),
	)
	tb := NewTransport(
	webrtc.Configuration{},
	new(mplex.Transport),
	)
	addr := "/ip4/127.0.0.1/tcp/0/http/p2p-webrtc-direct"
	utils.SubtestTransport(t, ta, tb, addr, "peerA")
	}

=== RUN   TestTransport/github.com/libp2p/go-libp2p-testing/suites/transport.SubtestStress1Conn1Stream100Msg
panic: Fail in goroutine after TestTransport/github.com/libp2p/go-libp2p-testing/suites/transport.SubtestStress1Conn1Stream1Msg has completed

goroutine 533 [running]:
testing.(*common).Fail(0xc000357680)
        /usr/local/Cellar/go/1.15.5/libexec/src/testing/testing.go:688 +0x125
testing.(*common).Error(0xc000357680, 0xc0004d0f98, 0x1, 0x1)
        /usr/local/Cellar/go/1.15.5/libexec/src/testing/testing.go:788 +0x78
github.com/libp2p/go-libp2p-testing/suites/transport.echoStream(0xc000357680, 0x18c05e0, 0xc00047a0d0)
        /Users/marten/src/go/pkg/mod/github.com/libp2p/go-libp2p-testing@v0.3.0/suites/transport/stream_suite.go:109 +0x26d
created by github.com/libp2p/go-libp2p-testing/suites/transport.goServe.func1.1
        /Users/marten/src/go/pkg/mod/github.com/libp2p/go-libp2p-testing@v0.3.0/suites/transport/stream_suite.go:145 +0x4b
exit status 2

[mearaj]

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

[backkem]

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?

[marten-seemann]

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.

[backkem]

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.

https://github.com/libp2p/go-libp2p-testing/blob/dccf408e2af2fe8f9fcabf71d9ba9498d3c3ec1f/suites/transport/transport_suite.go#L144-L154

Maybe this was meant to be a deferred close?

[marten-seemann]

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).

[backkem]

Yea, makes sense. This means:

1. We need a PR on go-libp2p-testing to correct the closing behavior to: CloseWrite, ReadAll, CloseRead (or Close).
2. We'll need to refine this transport implementation to support closing each side independently. I don't think that is supported just yet. It'll have to be determined if this can be done using WebRTC itself or if we need a thin extra protocol on top. Maybe we can look at what the JS WebRTC transport does here.

[mearaj]

Also please have a look at this
DataChannel's Close() Implementation
It indirectly calls sctp.Stream's Close()
Thanks :)

[backkem]

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.