Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API break in client/server creation #271

Closed
roysjosh opened this issue Jan 21, 2022 · 10 comments
Closed

API break in client/server creation #271

roysjosh opened this issue Jan 21, 2022 · 10 comments

Comments

@roysjosh
Copy link
Contributor

In fixing #269 the same user tested my PR and found that a different Home Assistant component, pytradfri, broke due to an API change around client/server creation:

Stacktrace copy/pasted here:

Logger: homeassistant.config_entries
Source: components/tradfri/__init__.py:94

Error setting up entry 192.168.1.12 for tradfri
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/config_entries.py", line 327, in async_setup
    result = await component.async_setup_entry(hass, self)
  File "/usr/src/homeassistant/homeassistant/components/tradfri/__init__.py", line 94, in async_setup_entry
    factory = await APIFactory.init(
  File "/usr/local/lib/python3.9/site-packages/pytradfri/api/aiocoap_api.py", line 47, in init
    await instance._update_credentials()
  File "/usr/local/lib/python3.9/site-packages/pytradfri/api/aiocoap_api.py", line 266, in _update_credentials
    protocol = await self._get_protocol()
  File "/usr/local/lib/python3.9/site-packages/pytradfri/api/aiocoap_api.py", line 63, in _get_protocol
    self._protocol = asyncio.create_task(Context.create_client_context())
  File "/usr/local/lib/python3.9/asyncio/tasks.py", line 361, in create_task
    task = loop.create_task(coro)
  File "/usr/local/lib/python3.9/asyncio/base_events.py", line 433, in create_task
    task = tasks.Task(coro, loop=self, name=name)
TypeError: a coroutine was expected, got <aiocoap.util.asyncio.coro_or_contextmanager.AwaitOrAenter object at 0x7fc86d47e0a0>
@roysjosh
Copy link
Contributor Author

Obviously you are in 0.x.y semantic versioning so breakage is expected but just wanted to make sure you were aware. If this is intended, I'll open a ticket with pytradfri.

@chrysn
Copy link
Owner

chrysn commented Jan 21, 2022 via email

@Jc2k
Copy link

Jc2k commented Aug 16, 2022

@chrysn if you are working on a release for HA/aiohomekit in #270, it would be good to pick up this one too. I'm not 100% sure if it has been fixed in the meantime though.

@chrysn
Copy link
Owner

chrysn commented Aug 16, 2022

I'm presently on it :-)

@Jc2k
Copy link

Jc2k commented Aug 16, 2022

Yay! Thank you very much

@chrysn
Copy link
Owner

chrysn commented Aug 16, 2022

Phew, that's a tough one, for it seems I can't properly subclass a coroutine in Python -- and the preferred pattern for proper shutdown is would be async with Context.create_server_context(site): ... going forward.

I'm leaning towards just fessing up to this API change ("you can await it for compatibility reason, so your code will keep working unless you directly place the awaitable into a asyncio.create_task(), but consider using it as an asynchronous context manager right away").

Would it be feasible for you to lockstep this in a pytradfri update? I'm not sure what the intention behind the code is there as it is (I'd just make _get_protocol await the creation and set the context, it's not like asyncio code is typically multilooped / reentrant), otherwise I'd just propose a change there.

@chrysn
Copy link
Owner

chrysn commented Aug 16, 2022

Proposed a fix in home-assistant-libs/pytradfri#536 that AIU should be usable both for the old and the new aiocoap version. If it works out here, it'd reassure me in making this small breaking API change and point to that PR as a reference fix (for those who can't easily go the async context manager route).

@chrysn
Copy link
Owner

chrysn commented Aug 17, 2022

Based on discussion in home-assistant-libs/pytradfri#536 I'm fixing this by rolling back the offending commit. Thanks to you and @MartinHjelmare for your input on that matter.

@chrysn chrysn closed this as completed in 38a2f0b Aug 17, 2022
@Jc2k
Copy link

Jc2k commented Aug 17, 2022

Great stuff, thank you!

@chrysn
Copy link
Owner

chrysn commented Aug 17, 2022

The current master branch is about to become 0.4.4, could you give it a test run to verify that your issues were addressed before I push the button?

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

No branches or pull requests

3 participants