switch get_client to async #15
Conversation
| async def get_client(self) -> httpx.AsyncClient: | ||
| """Return the session or create a new session with optional debug logging.""" | ||
| if self.session is None: | ||
| self.session = await super().get_client() |
There was a problem hiding this comment.
I think you explicitly don't want to use super here? As it stands you can set different factories in the subclasses if that's seomthing you care about for whatever reason.
There was a problem hiding this comment.
Client IS a subclass of HttpClientFactory and this override of get_client() is classic, add something to super's implementation:
- cache the session
- add event hooks for detailed logging
thus the explicit call to super() at the time of our choosing.
i guess the same effect could be achieved by setting a different factory but how would that then conflict with hass, which also needs to set_client_factory() for different reasons?
There was a problem hiding this comment.
I'm with you now, sorry I was misreading what's going on a little. I'm with you now.
| """Factory for creating httpx.AsyncClient.""" | ||
|
|
||
| @staticmethod | ||
| async def _default_get_client() -> httpx.AsyncClient: |
There was a problem hiding this comment.
I'm not sure I understand what's made better by making this all be async? Is the idea here that it will loan cleanly in hass now and we don't have to use their helper?
I feel like at best creating a new client shouldn't do io and at worst it'll do it once.
There was a problem hiding this comment.
with the API shift to async all session activity occurs in async context, which also works best for hass. EXCEPT Client.__init__, which CANNOT be async, created the session in sync context and, to your point, did I/O to load the SSL certificates from filesystem. this causes the warning from hass because Client is created in async_setup_platform and hass notices the blocking I/O in async context.
this change pushes session creation to async context uniformly, removing it from __init__, so get_client() becomes async. but look at https://github.com/richo/homeassistant-franklinwh/pull/57/changes#diff-adf648fcc09089e05b962654454a3cbdfcfe9884f8feee8c10fb6bee086a859dR73-R76: due to the I/O we still require hass' helper.
There was a problem hiding this comment.
again, contrast with #13 where we create the session in known (hass, async) context.
| return response | ||
|
|
||
| self.session.event_hooks["request"].append(debug_request) | ||
| self.session.event_hooks["response"].append(debug_response) |
There was a problem hiding this comment.
I was just rooting around in httpx internals and realised we can do this instead of trying to build a new client, I'll make sure I cherry pick this regardless of which session injection approach merges.
2 participants
jump the ugly python sync / async chasm by switching get_client to async.