aiocoap: Adjust to subtle API change

[chrysn]

This is an attempt to make pytradfri robust against the change in chrysn/aiocoap#271.

I didn't make myself familiar with the pytradfri testing setup yet -- would CI cover this one?

[MartinHjelmare]

I suggest another approach.

Side note: Not being able to create a task from a coroutine is very unexpected in a library API. I'd recommend to think about that change carefully.

[chrysn]

On the API change policy, I'd have considered "awaitable" the more relevant distinction than by which kind of awaitable that is backed. Thing is, the creation should used an async context manager right away -- any better suggestions for migrating? (I've tried to wrap things as good as possible to support both ctx = await create_client_context() and async with create_client_context() as ctx:, just like open does, but I can't subclass or otherwise mock or modify a coroutine object)

[MartinHjelmare]

How common is it that applications want to use a context manager for the protocol to shutdown?

[chrysn]

I honestly don't know, but especially when dealing with beginner programs, I found that the shutdown as often not performed, leading to warnings at cleanup and protocol misbehavior (observations not cancelled). I haven't switched over the CLI components yet (essentially waiting for this to not cause trouble, which it now does), but I expect things to be easier there too.

Is there any documentation or other rough consensus of the await self.foo.shutdown() pattern that it would be preferable to asynchronous context managers?

[edit: To clarify, I'm still considering reverting that change in aiocoap to ... somehow use a different migration strategy to async context managers, such that it would not break the original code here.]

[MartinHjelmare]

In my experience, context managers for clients and similar APIs work well and is nice for smaller applications, eg CLIs, but are not useful for larger applications that have longer runtimes and more complex needs around setting up and shutting down. The context manager requires the application to shutdown in the same code context that sets up, but that's often not convenient or possible in larger applications.

Maybe instead extend the Context signature with parameters matching Context.create_client_context and await create_client_context (or similar non classmethod) in __aenter__ of Context?

That way users can conveniently use a context manager to get a protocol, but existing applications that don't need that retains the existing API.

[chrysn]

I'm reverting the breakage in aiocoap and will go for some different contstructor (possibly really just Context() as you suggest) for the context manager. Thus, there is no further need for this PR.

Thanks for your input!

[chrysn]

I usually keep forks and branches of closed PRs around for reference, but in this case that's impractical as pytradfri forks suffer from dependabot/dependabot-core#2804.

To keep the patch around, this was it:

commit 81983dd309ea9b14b0e60e7c30d4b37d5a91ce47 (HEAD -> aiocoap-next, mine/aiocoap-next)
Author: chrysn <chrysn@fsfe.org>
Date:   Tue Aug 16 10:16:13 2022 +0200

    aiocoap: Adjust to subtle API change
    
    See-Also: https://github.com/chrysn/aiocoap/issues/271
    
    Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

diff --git a/pytradfri/api/aiocoap_api.py b/pytradfri/api/aiocoap_api.py
index 314192d..31ec5f3 100644
--- a/pytradfri/api/aiocoap_api.py
+++ b/pytradfri/api/aiocoap_api.py
@@ -74,7 +74,8 @@ class APIFactory:
         self._host = host
         self._psk_id = psk_id
         self._observations_err_callbacks: list[Callable[[Exception], None]] = []
-        self._protocol: asyncio.Task[Context] | None = None
+        self._protocol: Context | None = None
+        self._protocol_creation_lock = asyncio.Lock()
         self._reset_lock = asyncio.Lock()
         self._shutdown = False
 
@@ -100,9 +101,10 @@ class APIFactory:
 
     async def _get_protocol(self) -> Context:
         """Get the protocol for the request."""
-        if self._protocol is None:
-            self._protocol = asyncio.create_task(Context.create_client_context())
-        return await self._protocol
+        async with self._protocol_creation_lock:
+            if self._protocol is None:
+                self._protocol = await Context.create_client_context()
+        return self._protocol
 
     async def _reset_protocol(self, exc: BaseException | None = None) -> None:
         """Reset the protocol if an error occurs."""