search

libp2p / go-libp2p-webrtc-direct

A libp2p transport that enables browser-to-server, and server-to-server, direct communication over WebRTC without requiring signalling servers
MIT License
82 stars 18 forks source link

Update pion/webrtc to 2.0.27 #30

Closed albrow closed 5 years ago

albrow commented 5 years ago

The latest release might fix the performance problems we were experiencing. Waiting to see what happens in CI in order to confirm.

albrow commented 5 years ago

Just updated dependencies to include the following unreleased changes:

All tests pass locally now. Just waiting to see what happens on CI. 🤞

albrow commented 5 years ago

@backkem all the tests pass if we disable the race detector. I think the sheer amount of goroutines we are spinning up is just too much for the race detector to handle, especially on the TravisCI machines, which are not particularly powerful.

I vote that we disable the race detector for the stress tests and only use it for the other tests. What do you think?

https://github.com/libp2p/go-libp2p-transport/blob/045097a6a962aeccac5e06d672a9b2489cf4b68c/test/stream.go#L158 suggests the race detector only supports ~8k funcs (I think they mean goroutines) and the stress tests themselves spin up 5k. We probably run much much more than that with everything going on under the hood.

backkem commented 5 years ago

Ok, it would be nice if we could do this while also using the test suite directly again: https://github.com/libp2p/go-libp2p-webrtc-direct/blob/b213ddc5d17985af4a77a60757cdc5956d71e6b7/webrtcdirect_test.go#L28

raulk commented 5 years ago

Yes, indeed. The race detector gives up as soon as you spin up more than 8192 concurrent goroutines. It's easy to detect when we're running with the race checker via build tags, we tend to use https://github.com/ipfs/go-detect-race to avoid reinventing the wheel; the upcoming libp2p/go-libp2p-testing module will absorb this util.

The pattern is:

  1. skip stress tests if we're running with the race detector.
  2. run a CI build matrix with and without the race detector.
backkem commented 5 years ago

The update to webrtc v2.0.16 and go-libp2p-core seems to cause a deadlock of some sort. I don't have the time to look into this ATM :(

backkem commented 5 years ago

Figured out the problem currently being faced: a pion/datachannel.DataChannel is packet-based and go-mplex expects a stream based net.Conn which has slightly different characteristics (while having the same interface since they are both connection-oriented).

Overview of different types of connections as I see them:

Connection oriented
(net.Conn)		 Connection-less
(net.PacketConn)
Stream based
(Read/write byte streams)		 IP, all of libp2p for now ?
Packet based
(Read/write bounded packets)		 ICE, DataChannel UDP

Side-note: Properties a connection may or may not have:

It feels like libp2p should model for that.

backkem commented 5 years ago

@albrow Last commit makes the entire test suite green. I opted to use a fixed buffer to overcome the stream vs packet based issue. This point definitely needs further investigation and/or discussion with the libp2p guys. However, I think it's mergable in the mean time. Do you mind reviewing?

albrow commented 5 years ago

@backkem LGTM I think we should go ahead and merge this PR since it passes all the tests. We can separately start a broader discussion about how to handle stream vs. packet based connections.

(I can't leave a review in GitHub because I'm the original author of the PR. If you hit the approve button I think GitHub will let us merge).