Async wrapper code for sync library

[pvizeli]

Context

Currently, there is no strict guideline for async/sync core calls from integrations and where it should handle the wrapper code for a sync library/interface. The fact is, our core already has a highly optimized async/sync wrapper to interface with sync interfaces/libraries.

It makes it hard to read and review code when one is looking at many async functions, and later on, one would discover that the library is not async at all.

Some integrations handle this in their own wrapper, basically recreating the same code as the core wrapper already provides. We should prevent creating duplicate code.

A synced library might also do I/O on a next release, causing blocks. It is almost impossible to check every method/function of a library on an update to ensure every called function is async safe or not.

The problem might be self-inflicted, as we announced asyncio is faster. Correct; however, this is only the case if all things are truly async.

It is not wrong to use blocking I/O on libraries and implement the sync Home Assistant interface instead of the async interface.

Proposal

The core is the only place that has async wrapper code for sync methods.

An integration that is not async must only use sync functions and also solely implement the sync interfaces of the Home Assistant entity.

One exception are locks. With lock entities, it's allowed to use a mix of sync and async until we have a solution for such constructs in our core.

Consequences

We don't merge PRs that implement new integrations or features that mix up sync and async or are using wrappers to work around it.

[Jc2k]

Am i right in thinking config flow stuff is async only as it stands? And with discovery being phased out in favour of zeroconf/ssdp there would be no way integrate with discovery and not mix async and sync API's in an integration right now? Is this an API gap that needs addressing?

[pvizeli]

I agree the config_flow is something that we need to address. We normally don't create a function which is not directly used on a code. The first sync integration would be needed to create a sync interface for it but for time reason, they were never done. There also some core integration they work a bit special like mqtt/zeroconf because they rely not on an interface and maybe also not a real integration like HTTP/frontend (I exclude this)

[andrewsayre]

It makes it hard to read and review code when one is looking at many async functions, and later on, one would discover that the library is not async at all.

I think that's a mistake in understanding asyncio not because someone "mixed" async and sync implementations in HA. It's not clear to me what the actual problem is that would be solved by adopting this new rule. All of the examples are potential issues in ANY integration and third-party library--whether async or not or whether async/sync methods were used consistently on the HA side. I've seen many "async" libaries that have sync code buried in the implementation because the user didn't fully understand asyncio.

[balloob]

Sometimes sync code will need to be implemented in async because it needs to use sleep or locks, which we don't want in sync-land.

[amelchio]

It is also unclear to me what this proposal is trying to solve. Maybe you can link to an example?

Actually, my impression was that the sync interface was mostly a compatibility wrapper and new core code would only be async. A simple example is async_added_to_hass(). The (IMHO overstated) performance argument has played a big role in forming that impression.

[balloob]

I think what it is trying to achieve is that we sometimes see integrations that use async methods and use that to wrap sync calls, instead of going through our sync layer. And then one of those async calls accidentally does something sync.

However, I feel like we can't force this with a hard rule, because we don't have sync API for everything (async_setup_entry comes to mind). Although that is solvable, things like request batching and locks are better solved in async land and so I don't know if setting a hard rule is going to help. We might instead improve our guiding in the developer docs.

[amelchio]

We should probably decide whether the sync interface is first class. We currently treat it poorly and IMHO the questionable wraps are a result of that.

[balloob]

The original plan was to have it be phased out and no longer be added to new APIs, and have all new contributions be done in async.

However, I think that time has thought us that it is too high of a barrier to expect all contributions to be in async.

[amelchio]

As an anecdote, I recently moved to a sync/async mix in the Sonos integration in order to be able to use a lock. It is an unpleasant way to develop an integration, always having to remember the thread you are in and adding jumps when it is the wrong one.

So I don't think we need rules to prohibit people doing that. Instead, we should provide tools so it is not necessary to walk down that path in the first place and automatically people will avoid doing it.

Probably the important thing in this issue thus is for core: that all interfaces must be available in both sync and async versions.