aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

shouldn `env_proxy` affect after proxy that provided by users from code

Open NewUserHa opened this issue 1 year ago • 9 comments

Is your feature request related to a problem?

https://github.com/aio-libs/aiohttp/blob/e79b2d5df70a2644e81925cc49558962af91848d/aiohttp/client.py#L502-L507 https://github.com/aio-libs/aiohttp/blob/e79b2d5df70a2644e81925cc49558962af91848d/aiohttp/client.py#L609-L617

currently, the async def _request(...)

  • uses proxy from env first than from code
  • it at the same time causes calling get env proxy from env and registry in each request(aka _request()) call.

Describe the solution you'd like

keep env proxy in session, and use after code proxy that provided by users also should throw error for socks5 proxy since aiohttp does not work with socks5 currently

Describe alternatives you've considered

N/A

Related component

Client

Additional context

No response

Code of Conduct

  • [X] I agree to follow the aio-libs Code of Conduct

NewUserHa avatar Nov 21 '24 22:11 NewUserHa

Sorry, I re-read the message several times and still cannot figure out what is the question.

asvetlov avatar Nov 22 '24 11:11 asvetlov

  • ~~when both proxy settings in environment variable and ClientSession(proxy='...') exist, the async def _request(...) will use the former. see line 614 and 504, the _default_proxy in 504 is read from latter.~~
  • also here get_env_proxy_for_url(url) will read env and registry each time _request(...) is called, however the env proxy should be kept in session rather than lookup for each request
  • aiohttp should explicitly throw error for socks5 proxy if provided, since aiohttp only support HTTP proxy does not support socks5 proxy at all

NewUserHa avatar Nov 22 '24 15:11 NewUserHa

Thanks for the clarification.

asvetlov avatar Nov 25 '24 17:11 asvetlov

Can you create a test in a PR to reproduce the issue? Looks to me like the provided proxy parameter will override the env var, which I think is the expected behaviour.

Dreamsorcerer avatar Nov 30 '24 00:11 Dreamsorcerer

was mistake. correct, the proxy user provided does will override the env var by

 if proxy is not None: 
     proxy = URL(proxy) 
 elif self._trust_env: 
     with suppress(LookupError): 
         proxy, proxy_auth = get_env_proxy_for_url(url) 

the issues only are:

  • get_env_proxy_for_url(url) will read env and registry each time _request(...) is called
  • the env proxy maybe should be kept in session rather than lookup for each request
  • aiohttp should explicitly throw error if socks5 proxy provided, since aiohttp only support HTTP proxy and does not support socks5 proxy at all, and currently will ignore the schema of URL of the provided proxy cause the proxy server directly close the connection

NewUserHa avatar Nov 30 '24 00:11 NewUserHa

Could probably cache one of those functions if you wanted to open a PR. Not sure if there was any reason to not cache them originally (maybe memory use, or being able to update the file while an application is running)...

Dreamsorcerer avatar Dec 01 '24 14:12 Dreamsorcerer

https://github.com/aio-libs/aiohttp/blob/1fa237ffc9e7aa70cbabb68ab64d6fe03255cbc0/aiohttp/helpers.py#L306-L320 but the proxies_from_env() is no parameter, compared to cache it, I think it can just be stored in session level. and the explicitly throw error for socks5 proxy not supported should be in a different PR?

NewUserHa avatar Dec 01 '24 15:12 NewUserHa

Yeah, I think that sounds reasonable.

Dreamsorcerer avatar Dec 01 '24 16:12 Dreamsorcerer

the get_env_proxy_for_url issue part needs to wait https://github.com/python/cpython/pull/127767 to work

NewUserHa avatar Dec 09 '24 16:12 NewUserHa