Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider replacing electron-webrtc with wrtc #76

Closed
feross opened this issue Apr 18, 2018 · 16 comments
Closed

Consider replacing electron-webrtc with wrtc #76

feross opened this issue Apr 18, 2018 · 16 comments

Comments

@feross
Copy link
Member

feross commented Apr 18, 2018

I see much more active work on wrtc these days. It might be worth testing it out again.

@feross
Copy link
Member Author

feross commented Apr 26, 2018

Just out wrtc and it failed to even connect to an https://instant.io peer in Chrome. I may be missing something obvious, but for simply seeding a single torrent, it appears electron-webrtc is still the best option.

Would be happy if someone else could take a look at swapping in wrtc in case I was doing something silly.

@Fabiencdp
Copy link

I tried with wrtc version 0.1.0 with webtorrent and it work ! I noticed it lose connection some time, mainly just after start seeding. so, as a quickfix i check if wire still exist on the server side webtorrent client, if not i destroy torrent and add it again, then the wire looks stable.

It don't work with higher version of wrtc, i try to find why.

@Fabiencdp
Copy link

Fabiencdp commented Apr 26, 2018

I created a test project, it's a simple express server with webtorrent version 0.99.2 and wrtc module.
I use it as a WebRTC seedbox with instant.io as client.
Here is a simple test repo https://github.com/Fabiencdp/webtorrent-wrtc-test
I tried to change wrtc version and noticed the following:

wrtc v0.1.0 & wrtc v0.1.1
Seems better when torrent is added to the server first.
Peer connects fast between server and client.
Lost connection after 5/10 seconds of seeding.
Connection come back after some random time, and become stable.
It's look like it works a little better with version 0.1.0, but it's maybe randomness

wrtc >= v0.1.2
Don't work...

NOTE
I used a clone of the webtorrent repo and added a quick&dirty fix to ./dependencies/webtorrent/lib/torrent.js, line 944, because i was getting this error :
Error: invalid addr: [2a01:NaN at addrToIPPort (/home/fabiencdp/www/webtorrent-test/server/node_modules/addr-to-ip-port/index.js:13:19) at Torrent._onWire (/home/fabiencdp/www/webtorrent-test/server/node_modules/webtorrent/lib/torrent.js:943:17) at Peer.onHandshake (/home/fabiencdp/www/webtorrent-test/server/node_modules/webtorrent/lib/peer.js:180:14) at Wire.<anonymous> (/home/fabiencdp/www/webtorrent-test/server/node_modules/webtorrent/lib/peer.js:142:10) at Object.onceWrapper (events.js:320:30) at emitThree (events.js:135:13) at Wire.emit (events.js:216:7) at Wire._onHandshake (/home/fabiencdp/www/webtorrent-test/server/node_modules/bittorrent-protocol/index.js:439:8) at Wire._parser (/home/fabiencdp/www/webtorrent-test/server/node_modules/bittorrent-protocol/index.js:705:12) at Wire._write (/home/fabiencdp/www/webtorrent-test/server/node_modules/bittorrent-protocol/index.js:596:10)

@feross
Copy link
Member Author

feross commented Apr 27, 2018

@Fabiencdp That's interesting. I didn't get any errors at all when I tried wrtc.

The Error: invalid addr 1:NaN at addrToIPPort error looks it's caused by peer.conn.remotePort being set to NaN. I just added an extra check to prevent that from happening and released a new version of webtorrent.

Can you try again with webtorrent 0.99.3? Since you're using webtorrent-hyrbid, you can just delete node_modules and reinstall webtorrent-hybrid to force the latest webtorrent to be used.

@Fabiencdp
Copy link

Fabiencdp commented Apr 27, 2018

Thanks @feross ! It fix the invalid addr error.
I don't use webtorrent-hybrid on my test repo, i simply do something like :

const WebTorrent = require('webtorrent'); // 0.99.3
global.WRTC = require('wrtc');
...

But it's the same, no ?

It still lost connection with wrtc 0.1.0, but wrtc looks promising and better than electron.
It's also easier to setup, need less dependencies, and consume less memory.

I'm really not an expert about RTCPeerConnection and stuff so it's hard to find where are the problems. Anyway I'd like to help if you want to switch to wrtc.

I will try to dig a little more next week.

@markandrus
Copy link

I think it'd be cool if webtorrent-hybrid worked with wrtc again—it should help us get a compliant/bug-free implementation of wrtc. But I think it's important to consider user experience, especially on install. Assuming wrtc irons out all bugs, the install experience can still be annoying:

  1. Best scenario: you use a supported version of Node on an already supported platform, so wrtc's postinstall script simply downloads the right wrtc.node using node-pre-gyp.
  2. Bad scenario: you use an unsupported version of Node on a platform that libwebrtc supports and you have CMake installed, so wrtc's postinstall script falls back to downloading the right libwebrtc.a and then builds wrtc.node using node-cmake.
  3. Worse scenario: you use an unsupported version of Node on a platform that libwebrtc supports but you do not have CMake installed, so wrtc fails to install.
  4. Worst scenario: you use any version of Node on a platform that libwebrtc does not support, so wrtc fails to install.

electron-webrtc uses electron and therefore its installation method. There should be similar issues trying to install electron-webrtc on unsupported platforms. Also,

npm install --ignore-scripts

or

npm config set ignore-scripts true

would break either wrtc or electron-webrtc's installers.

@feross
Copy link
Member Author

feross commented Apr 27, 2018

@Fabiencdp Thanks for your eagerness to help. I'd really like to see a working example of webtorrent-hybrid working with wrtc. I've been unable to get it working so far. Would love it if someone could dig in here and figure it out.

@markandrus Every native module breaks if the user uses --ignore-scripts so there's nothing we can do about that. I like the way you laid out the various scenarios with wrtc. I think that a webtorrent-hybrid implementation that uses less memory and has the latest WebRTC implementation, but runs on fewer platforms might be a better tradeoff. If someone wants an easy-to-install hybrid client, they can always install WebTorrent Desktop.

@Fabiencdp
Copy link

@feross, did you take a look at this test repo ?
https://github.com/Fabiencdp/webtorrent-wrtc-test
i just updated it to use webtorrent-hybrid and provided an easy switch between the two versions of wrtc.
Did it work for you ?

@feross
Copy link
Member Author

feross commented May 3, 2018

@Fabiencdp It's a problem that it's still not working with wrtc >= 0.1.2. We can't merge until we figure out what's going on there. Let me know if you (or anyone else!) figures anything out.

@nazar-pc
Copy link

@markandrus, I think it should be possible to be in Best scenario most of the time if Node version is specified in package.json: https://docs.npmjs.com/files/package.json#engines

@feross
Copy link
Member Author

feross commented May 23, 2018

@nazar-pc The engines field is just a recommendation, so it won't block users on unsupported Node.js versions from installing webtorrent-hybrid, but it will let them know that they shouldn't expect it to work ;-)

@nazar-pc
Copy link

Another option might be https://www.npmjs.com/package/node, that would definitely install Node.js version you need

@feross
Copy link
Member Author

feross commented May 23, 2018

@nazar-pc Hah! That's one brute force approach. I think we'll stick with the user-installed Node.

@neuronsupport
Copy link

neuronsupport commented Jul 26, 2018

@feross could you confirm the #83? If this is solved, it would really really help.

@pimboli
Copy link

pimboli commented Dec 3, 2018

Hello,

any news about the migration to wrtc? with the electron version its impossible to seed more than 30 files.

@DiegoRBaquero
Copy link
Member

Released as 2.0.0 :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants