search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
153 stars 139 forks source link

FCM API Credential refresh mechanism doesn't support HTTP proxies and blocks the event loop #371

Closed reivilibre closed 1 month ago

reivilibre commented 1 month ago

title says it all: it doesn't look like we support HTTP proxies here and given no await I suspect this also blocks the event loop: https://github.com/matrix-org/sygnal/blob/c025ac1b16b347e31ae6c54d491c01c16b0de599/sygnal/gcmpushkin.py#L480

reivilibre commented 1 month ago

aiohttp supports proxies: https://docs.aiohttp.org/en/stable/client_advanced.html#proxy-support

as does HTTPX, which sounds like it could be the next big thing in Python, or maybe that's just some inflated hype — would need consideration: https://www.python-httpx.org/

(Either of these would probably be preferable to us hacking our own CONNECT proxy support into Twisted and asyncio using two similar but different bits of code. Realistically it doesn't make that much sense for us to be using Twisted in Sygnal anymore)

MatMaul commented 1 month ago

The google-auth lib seems to support a custom urllib3 pool: https://googleapis.dev/python/google-auth/latest/user-guide.html#urllib3

To my understanding we can replace an urllib3 PoolManager by a ProxyManager: https://urllib3.readthedocs.io/en/stable/advanced-usage.html#proxies

I guess it would be the straightest way to support a proxy here, what do you think ?

MatMaul commented 1 month ago

Oh urllib3 is blocking, but it seems we are using the requests backend for now, which is also blocking, hence the fact that there is no await I think.

We can also customize the requests Session, and use a proxy there: https://docs.python-requests.org/en/latest/user/advanced/#proxies

MatMaul commented 1 month ago

google-auth has an aiohttp backend even if it seems undocumented.

And it seems that it can support a custom Session which easily supports a proxy, so we may have an easy-ish async way forward.

MatMaul commented 1 month ago

To be clear so we don't double the work, we need proxy support on our deploy so I am currently working on that, I should be able to send a PR this week.