Skip to content

Conversation

@yciabaud
Copy link
Contributor

@yciabaud yciabaud commented Jun 5, 2016

Looking at webtorrent/webtorrent#807, I am trying to add proxy support to tracker clients.

For now, I am thinking about adding an http agent option to allow proxying HTTP and WebSocket request by using a Socks or a HTTP proxy agent.

For UDP, I am using the Socks module directly, @feross you suggested to use a socks instance to avoid adding a dependency but the module is not designed that way.

We could add a layer but this seemed a bit complicated for this feature...
I finally achieved to run a working socks proxy for UDP, so I could test all trackers successfully.

I believe it's ready to merge now, let me know what you think.

@yciabaud yciabaud changed the title Allow proxy usage on tracker clients [WIP] Allow proxy usage on tracker clients Jun 10, 2016
@yciabaud
Copy link
Contributor Author

After some testing, looks like the UDP implementation is not ready yet.

@yciabaud yciabaud changed the title [WIP] Allow proxy usage on tracker clients Allow proxy usage on tracker clients Jul 20, 2016
@yciabaud yciabaud force-pushed the proxy branch 2 times, most recently from 678c301 to 35d3d2a Compare July 20, 2016 16:54
@yciabaud
Copy link
Contributor Author

OK UDP is working now, tested it with a local dante server and one provided by https://www.privateinternetaccess.com (thanks to @romaincointepas)

Reviews are welcome!

@feross @DiegoRBaquero @dcposch

@dcposch
Copy link

dcposch commented Jul 21, 2016

This is awesome. After this is in, I'll add proxy options to the Prefs page in WebTorrent Desktop

function send (message) {
if (!parsedUrl.port) {
parsedUrl.port = 80
function send (message, info) {
Copy link

@dcposch dcposch Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it proxyInfo or similar

@yciabaud
Copy link
Contributor Author

OK changed the structure a little bit, tested more thoroughly and look good.

What do you think about the new structure?

yciabaud added a commit to yciabaud/webtorrent that referenced this pull request Jul 24, 2016
yciabaud added a commit to yciabaud/webtorrent that referenced this pull request Aug 9, 2016
yciabaud added a commit to yciabaud/webtorrent that referenced this pull request Aug 9, 2016
yciabaud added a commit to yciabaud/webtorrent that referenced this pull request Aug 24, 2016
client.js Outdated
self._getAnnounceOpts = opts.getAnnounceOpts
self._proxyOpts = opts.proxyOpts

// Create HTTP agents from socks proxy if needed
Copy link

@romaincointepas romaincointepas Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to delete the part when you automatically create the http(s)Agents from the socksProxy.proxy object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@dcposch
Copy link

dcposch commented Sep 4, 2016

LGTM

@yciabaud currently there's one style error -- https://travis-ci.org/feross/bittorrent-tracker/builds/155375449

Afterward I think we can merge it, release a new version, and then merge webtorrent/webtorrent#874

CC @feross

@yciabaud
Copy link
Contributor Author

yciabaud commented Sep 5, 2016

You're right @dcposch , I have missed that. Fixed.

@dcposch
Copy link

dcposch commented Sep 6, 2016

@feross want to merge this? (I'm not on this repo)

@yciabaud
Copy link
Contributor Author

yciabaud commented Sep 6, 2016

I can merge this but I wanted @feross feedback. He have to make the release afterwards anyway.

@yciabaud yciabaud mentioned this pull request Oct 23, 2016
@Miserlou
Copy link

Lots of good changes in here, and I'd like to use this functionality for another project now, so it'd be great if we could please have this reviewed, merged and published @feross!

@helenclarko
Copy link

Will someone please Merge this.

@tahnik
Copy link

tahnik commented Jan 7, 2021

Is there any reason why you did not merge this @feross?

@DiegoRBaquero
Copy link
Member

Closed in favor of #356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants