search

rndusr / stig

TUI and CLI for the BitTorrent client Transmission
GNU General Public License v3.0
554 stars 24 forks source link

proxy support via aiohttp-socks #169

Closed apexo closed 4 years ago

apexo commented 4 years ago

https://github.com/rndusr/stig/issues/121

rndusr commented 4 years ago

Thanks! Looks pretty good.

  1. The description for the "connect.proxy" setting seems a bit sparse, but that may be because I know nothing about proxies. I guess this only works with SOCKS proxies? Does "SOCKS" mean "SOCKS5"?

  2. ProxyConnector.from_url() can raise ValueError and maybe other exceptions. (e.g. :set connect.proxy foo)

  3. I'd prefer if this would be an optional feature. See "setproctitle" in setup.py for an example. If importing aiohttp_socks fails, a ConnectionError should be raised that mentions the missing dependency.

apexo commented 4 years ago
  1. The description for the "connect.proxy" setting seems a bit sparse, but that may be because I know nothing about proxies. I guess this only works with SOCKS proxies? Does "SOCKS" mean "SOCKS5"?

According to https://github.com/romis2012/aiohttp-socks/blob/master/aiohttp_socks/proxy/helpers.py#L46-L58 this supports SOCKS5 (socks5://), SOCKS4 (socks4://) and HTTP proxies (http://), including credentials (user:password@). Don't know how much documentation can reasonably be included there.

  1. ProxyConnector.from_url() can raise ValueError and maybe other exceptions. (e.g. :set connect.proxy foo)

I guess this should be validated in the @proxy.setter?

  1. I'd prefer if this would be an optional feature. See "setproctitle" in setup.py for an example. If importing aiohttp_socks fails, a ConnectionError should be raised that mentions the missing dependency.

Okay, shouldn't be much of a problem.

apexo commented 4 years ago

Also: is there a reason why aiohttp isn't included globally in client/aiotransmission/rpc.py? It's not an optional dependency, after all. Plus, importing it in a kind-of-hot codepath (like in _post) seems wasteful.

rndusr commented 4 years ago

According to https://github.com/romis2012/aiohttp-socks/blob/master/aiohttp_socks/proxy/helpers.py#L46-L58 this supports SOCKS5 (socks5://), SOCKS4 (socks4://) and HTTP proxies (http://), including credentials (user:password@). Don't know how much documentation can reasonably be included there.

OK, thanks for the reference. I should be able to find a concise description.

  1. ProxyConnector.from_url() can raise ValueError and maybe other exceptions. (e.g. :set connect.proxy foo)

I guess this should be validated in the @proxy.setter?

Yes, that's probably better. I forgot what the error behaviour for invalid values is, but there should be enough examples.

rndusr commented 4 years ago

IIRC, aiohttp takes something like 300ms to import, and it's not always needed (e.g. "stig help").

Importing an already imported module should be a dictionary lookup (AFAIK), and they are very fast.

I don't think this makes much of a differencce either way. Feel free to take measurements if you would like to change this.

rndusr commented 4 years ago

I haven't tested your new commit yet, but it looks like you're still not catching exceptions from ProxyConnector.from_url().

Try setting connect.proxy to some.domain.that.doesnt.resolve or an http://invalid:url/ or localhost:12345 (assuming that port is closed). I was getting different exceptions when I did some quick tests earlier.

apexo commented 4 years ago

I haven't tested your new commit yet, but it looks like you're still not catching exceptions from ProxyConnector.from_url().

As far as I can tell, ProxyConnector.from_url() only raises ValueError, and the proxy connector is now created in the @proxy.setter, where ValueErrors seem to be handled fine.

Try setting connect.proxy to some.domain.that.doesnt.resolve or an http://invalid:url/ or localhost:12345 (assuming that port is closed). I was getting different exceptions when I did some quick tests earlier.

Yes, I've seen socket.gaierror, aiohttp_socks.proxy.errors.ProxyConnectionError and

  File "/usr/lib/python3/dist-packages/aiohttp/client.py", line 357, in _request
    raise RuntimeError('Session is closed')
RuntimeError: Session is closed

But those are all triggered in _post when doing the actual request, i.e.: not related to the construction of the proxy connector. Guess we just need to catch socket.gaierror and aiohttp_socks.proxy.errors.ProxyConnectionError …