Skip to content

Conversation

@richo
Copy link
Owner

@richo richo commented Dec 31, 2025

Alternative implementation to #13 , would allow for a slightly nicer fix for richo/homeassistant-franklinwh#53

essentially, the idea is that consumers who don't understand or care that this exists do nothing, if you do care you just have to do something like:

from homeassistant.helpers.httpx_client import get_async_client
from franklinwh import HttpClientFactory, Client
HttpClientFactory.set_client_factory(get_async_client)

and then everything after that works as though by magic.

The one thing I want to chase up is whether this erases the http2 support that someone added a while back.

@jkt628
Copy link
Contributor

jkt628 commented Dec 31, 2025

@richo
i tried this this morning but still get the WARNING and, yes, erases HTTP/2 for Client but not TokenFetcher.

since get_async_client requires hass set_client_factory must be called from async_setup_platform and i suspect there is a very early get_client from the original factory.

@jkt628
Copy link
Contributor

jkt628 commented Dec 31, 2025

found it: TokenFetcher._login calls httpx.AsyncClient directly at line 342.

@richo
Copy link
Owner Author

richo commented Dec 31, 2025

Great catch thankyou!

After rolling this out creating the hass AsyncClient directly (So I can enable http2) I got some suspicious http2 shaped errors in other modules which doesn't make a ton of sense to me, but I'm gunna look further into it today. In the worst case I did just realise that franklinwh could try importing hass, and if it succeeds then use their client.

Your patch isn't bad at all and I'm not ruling it out, it's just a bit frustrating to add so much API surface for such a specific and limited usecase.

@richo
Copy link
Owner Author

richo commented Dec 31, 2025

Ok good news and bad news! That was the problem: switching that class up a little to always use the factory now makes it load cleanly. Buuuuut, turning on http2 here is causing other clients to have it enabled, for reasons I don't fully understand :(

@richo
Copy link
Owner Author

richo commented Dec 31, 2025

2025-12-31 12:17:54.682 ERROR (MainThread) [homeassistant.components.homeassistant_alerts.coordinator] Error requesting homeassistant_alerts data: 400, message="Expected HTTP/, RTSP/ or ICE/:\n\n  b''\n    ^", url='https://alerts.home-assistant.io/alerts.json'
2025-12-31 12:17:55.109 ERROR (MainThread) [metno] Access to https://aa015h6buqvih86i1.api.met.no/weatherapi/locationforecast/2.0/complete returned error 'ClientResponseError'

I get these in my logs, commenting out http2=True in both client creations makes it go away.

@richo
Copy link
Owner Author

richo commented Dec 31, 2025

I asked for help in the discord, hoping someone just knows why it's doing that off the top of their head so I don't have to figure out how to attach a debugger to homeassistant.

@richo
Copy link
Owner Author

richo commented Dec 31, 2025

Oh my god, so I looked into the homeassistant_alerts since it's in core and a little easier to poke at, and it's using a whole separate framework (aiohttp). I am totally baffled why turning on http2 in a client we notionally own is affecting other integrations.

It looks like we could theoretically deep copy the client and then reinitialize its transport but doing so would make us dependant on a private api.

@richo
Copy link
Owner Author

richo commented Dec 31, 2025

I opened home-assistant/core#160074

@jkt628
Copy link
Contributor

jkt628 commented Jan 2, 2026

@richo
i have tried unsuccessfully for two days to bridge synchronous get_client() with hass.async_add_executor_job() required by the integration. i end up stranded with RuntimeError: await wasn't used with future because await can only be used in async functions.

the big advantage of #13 is knowing the context in the caller instead of being ground down in the grimy morass that is python asyncio through callbacks. like #14, #13 also inflicts no change on the caller UNLESS they require it. the burden of richo/homeassistant-franklinwh#54 with #13 is simpler than will be required by #14, assuming the bridge can even be solved.

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

Successfully merging this pull request may close these issues.

3 participants