DataChannel multiplexing

[t-mullen]

Here's my implementation of DataChannel multiplexing. Opening channels and sending data works well, while preserving all of the old API.

It allows you to use the same Peer duplex stream as before by having the peers inherit the DataChannel object. But it also allows you to do multiplexing easily like this:

var dc = peer1.createDataChannel('secondChannel')
dc // Another Duplex stream!

peer.on('datachannel', (dc) => {
  dc // Duplex stream!
})

peer.write('abc') // still a Duplex, data goes through a "default" Datachannel

Open Problems:

• Buggy onclose behaviour in Chromium. Chrome intermittently doesn't fire the event when the datachannel creator closes it but instead gets stuck on "closing".
• Buggy behaviour when re-using channel names (labels). Safari throws and InvalidStateError when you attempt to do this.
• DataChannel instances require a reference to the original peer in order to determine if a connection is established. I didn't have much luck simply using the onopen event, so any help here would be appreciated.
• Safari refuses to send data through a datachannel that is using the same label as a previously closed datachannel.
• Electron WebRTC has no support for multiple datachannels and needs to be fixed before this can be merged. Error when creating second DataChannel. mappum/electron-webrtc#127 (electron-webrtc support has been dropped).
• More tests are needed.

Seeing as this is another significant change, I'd appreciate feedback on the code and API from the community.

[t-mullen]

@nazar-pc As per our previous discussion about reducing bundle size, the DataChannel module can be changed to any lightweight module that has a similar constructor and exposes the 'open' event. Everything else it implements (streaming API or otherwise) will be inherited by the peer object. So a light version of the DataChannel will also make the Peer object light.

[nazar-pc]

On the surface looks good, API is simple to use, will look into details later.

Are there upstream bug reports about onopen/onclose? Links would be useful if there are any.

Concerning size reduction, I think it would be a good idea to decouple basic functionality from DataChannel even further, so that potential DataChannelLight would not need to reimplement common methods. Not sure if it worth to decouple that far now though.

[t-mullen]

I'm still working on reproducing the upstream bugs with onclose simply. The onopen problem is probably an implementation flaw on my side. Definitely still some work to be done here.

I'll attempt to make a DataChannelLight implementation once these issues are worked out and we can explore further decoupling if that turns out to be difficult.

[nazar-pc]

Didn't run the code yet, will try to find time soon. Eager to implement signaling for re-negotiation on top of this using separate named data channel instead of forcing user to handle signal event even when there is a working connection already.

[nazar-pc]

PRs 1-5 in https://github.com/RationalCoding/simple-peer/pulls should hopefully solve points 1-3 and some of my review comments. There is also a test case for pre-negotiated default data channel.

I think after merging those together it should work more reliable, though I have no access to Safari to check behavior there.

We should probably mark Electron WebRTC support as deprecated now and note that it will not support multiple data channels, but should continue working otherwise until next major release.

[nazar-pc]

Looks good to me now. How can I reproduce the first issue if it is still present?

[t-mullen]

There are already tests in this PR for those issues that were disabled for Chrome and Safari. I've turned them back on, so just running the browser tests in Chrome should be enough to reproduce.

Specifically: 'closing channels from creator side' and 'reusing channelNames of closed channels'

[nazar-pc]

So everything works fine in Chromium Nightly from about a week ago (a bit slow on some tests) and works perfectly smooth on latest Chromium Nightly.
Chromium stable however will often stuck on some of the tests ("data sends on seperate channels", "closing channels from creator side" and "closing channels from non-creator side" in my case).
Looks like Chromium developers have already fixed this and it is just a matter of time when this will be released. If you have Chrome Beta, would be nice to check it as well.

Shouldn't be a blocker if it is already fixed upstream.

"reusing channelNames of closed channels" will stuck in Firefox Nightly and stable, but works perfectly in both once Dev Tools are open 😕. Not sure how to debug this behavior. Dev Tools typically slows down Firefox quite a bit so probably some kind of race too. Will try to play with various timeouts.

[nazar-pc]

Looks good to me now, awesome work!

[t-mullen]

So this has a few more unsolved browser-related issues. I've enforced unique channel names, which solved some issues, but there are more.

• Safari doesn't work correctly when data channels are created asynchronously. It announces the channel correctly, and closes correctly, but drops messages sent by the creator. The message simply never arrives on the receiving end. It's almost as if the socket never flushes, regardless of how long we wait. The creator can receive messages fine.
  https://bugs.webkit.org/show_bug.cgi?id=192566

• Firefox won't emit datachannel events created after a channel is closed...
  https://bugzilla.mozilla.org/show_bug.cgi?id=1513107

Going to need to reproduce with native WebRTC and open bugs.

Synchronous creation works fine though.

[t-mullen]

I've added the multiplex suite of tests (minus a couple that didn't make sense for simple-peer, like chunking the multiplexed stream).

All remaining issues are reported browser bugs.

[tinchoz49]

Hi everyone!! this is an awesome feature 👍 . Do you know when is going to be merged?

[t-mullen]

Can't be merged until the browser bugs breaking some of the functionality are fixed. You're welcome to try it out if you don't mind a bit of instability.

[t-mullen]

I've completed cross-browser testing for this.

The only bug that's left is Firefox's inability to create new channels after any are destroyed. We could support only creating DataChannels for now (no calling dc.destroy() before the peer is destroyed). I think this would still be very useful.

[samuelmaddock]

I think this would still be very useful.

I agree with @t-mullen here. This change moves simple-peer forward in supporting complex applications while maintaining the original public API. Would it be possible to add an experimentalDataChannels flag to the Peer options? That way it would discourage users from using it until all browser issues are resolved, but still allow for testing and more contributions to the new API.

[HR]

I agree with @samuelmaddock @t-mullen @nazar-pc this is a much-needed feature and should be added with a harmony flag so others can test it, is it possible to do that now?

Also @t-mullen, do these browser bugs persist in Node as well? If not, the more reason to land this now.

[Khivar]

Hello, I took a couple of hours this morning to read your code, fork the current master branch (which is now written in ES6) and compared them. Then I adapted a little bit of code, rewrote everything in ES6 and updated the documentation. It was not really easy comparing the files manually since they didn't share a common base / were written in different ES versions so I hope I didn't forget anything. I only did some basic testing like creating another data channel, checking its label and real channel name, sending / receiving data and destroying the data channel but so far it worked. If you want to have a look you can see the code here :

https://github.com/Khivar/simple-peer/tree/multi-datachannels

[HR]

@Khivar

I'm getting an infinite loop when I call peer.destroy()

  simple-peer [bbed62c] destroy (error: Connection failed.) +0ms
  simple-peer [bbed62c] destroy (error: Connection failed.) +0ms
  simple-peer [bbed62c] destroy (error: Connection failed.) +0ms
  simple-peer [bbed62c] destroy (error: Connection failed.) +0ms
  simple-peer [bbed62c] destroy (error: Connection failed.) +0ms
  simple-peer [bbed62c] destroy (error: Connection failed.) +0ms
RangeError: Maximum call stack size exceeded
    at String.replace (<anonymous>)
    at debug (/Users/hr/GitHub/sd/src/main/lib/simple-peer/node_modules/debug/src/common.js:92:22)
    at Peer._debug (/Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:849:11)
    at Peer._destroy (/Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:422:10)
    at Peer.destroy (/Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:416:10)
    at /Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:425:15
    at Array.forEach (<anonymous>)
    at Peer._destroy (/Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:424:20)
    at Peer.destroy (/Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:416:10)
    at /Users/hr/GitHub/sd/src/main/lib/simple-peer/index.js:425:15

[Khivar]

@HR

Thanks for the feedback and sorry for the late reply. It should be fixed, I also updated the code to use the JavaScript Standard Style and synced with the upstream master branch.

[samuelmaddock]

@t-mullen What are your thoughts on including this work behind an experimental flag until all browsers support it without major bugs?

[t-mullen]

My concern is with interop between browsers. I'd want to run extensive testing again (you can see how many bugs came up the first time). I'd like to see interop unit tests similar to the ones in #575. I'll start work, seeing as this feature is almost 2 years overdue.

I don't understand the idea of experimental flags in an open-source project. The version of simple-peer with these features is available in this PR or in @Khivar's fork - anyone is free to use them at their own risk and they aren't officially supported.

[t-mullen]

Closed in favour of #694