chrysn / aiocoap

The Python CoAP library
Other
264 stars 119 forks source link

API break in client/server creation #271

Closed roysjosh closed 2 years ago

roysjosh commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Thanks, that was not intended -- 0.x or not, reasonable use of aiocoap should not break w/o a deprecation over a number.

I'll look into it.

Jc2k commented 2 years ago

@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 commented 2 years ago

I'm presently on it :-)

Jc2k commented 2 years ago

Yay! Thank you very much

chrysn commented 2 years ago

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 commented 2 years ago

Proposed a fix in https://github.com/home-assistant-libs/pytradfri/pull/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 commented 2 years ago

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

Jc2k commented 2 years ago

Great stuff, thank you!

chrysn commented 2 years ago

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?