Using get_default_context() for ssl context to avoid blocking call #791
Conversation
|
@ollo69 Please review |
|
You should also remove line 15 ( |
| @@ -27,6 +26,7 @@ | |||
| import uuid | |||
|
|
|||
| import aiohttp | |||
| from homeassistant.util.ssl import get_default_context | |||
There was a problem hiding this comment.
This does not make sense because core_async should not depend on home assistant. If in the future this will be implemented as a separate library, cannot have this dependencies.
There is already a way to directly use the aiohttp session created in ha, probably the best solution is to use this as default option (at this moment is available as advanced option during initial configuration)
To implement this in the library in not blocking way we should implement the ha get_default_context and not import the HA method.
There was a problem hiding this comment.
I checked the implementation of get_default_context in homeassistant.util.ssl and I couldn't see any significant difference with what the current ha-smartthinq-sensors code is doing 🤔
Maybe the @cache decorator helps somehow?
There was a problem hiding this comment.
It could be, you should check what @cache decorator do...
There was a problem hiding this comment.
I checked. It basically adds a thin wrapper to cache the responses if there is another call to the same function with the same parameters https://docs.python.org/3/library/functools.html#functools.cache
but you are kind of doing that already with _get_session()
Looks like HA is not complaining about using ssl.create_default_context(), it's complaining about where it's being used. Unless the session can be created somewhere else outside of the event loop and then re-used here it's going be complicated.
Also, about this PR... isn't it changing the SSL ciphers of the default context for all HA? Isn't that risky?
Do we still need the _LG_SSL_CIPHERS ? Maybe they updated the servers and the defaults work now
There was a problem hiding this comment.
There is no cipher set in get_default_context so that part should be the same.
Yes, the warning can be resolved by creating the context outside the event loop but I don't think that can be done in the life cycle of an integration initialization and without referencing ha.
This addresses the I/O blocking warning as recommended in https://developers.home-assistant.io/docs/asyncio_blocking_operations/#load_default_certs
Closes #790