Switch to pyserial-asyncio-fast to fix event loop being blocked

[bdraco]

fixes #33

related PR home-assistant/core#116635

[bdraco]

I'm not sure why its passing locally but failing in the CI

[cottsay]

I'm not sure why its passing locally but failing in the CI

It fails locally on my machine with the same error. Python 3.12 on Fedora 39.

[bdraco]

ah, the test is skipped on darwin, which is what I'm testing on

[bdraco]

It might be due to this change pyserial/pyserial-asyncio@7987e6e which was never released in pyserial before it went unmaintained but made it into pyserial-asyncio-fast

[bdraco]

I'm not 100% sure what the test is trying to test here. We really need to get this fixed because sleep in the event loop is going to be blocked in HA 2023.7.x which will break this integration (its the last one to be fixed)

[cottsay]

I'm not 100% sure what the test is trying to test here.

The test uses a socket serial device to verify that when an already opened aioraven device is disconnected, attempts to get data raise an exception. It appears that under pyserial-asyncio-fast, the socket serial device behaves differently.

I can set up a test using socat to see if this change in behavior is specific to socket serial devices, but I don't have time to do it right now. Clearly I would have a biased perspective here, but if pyserial-asyncio-fast claims to be a drop-in replacement for pyserial-asyncio, it would be nice if it behaved the same way here.

[bdraco]

I can set up a test using socat to see if this change in behavior is specific to socket serial devices, but I don't have time to do it right now. Clearly I would have a biased perspective here, but if pyserial-asyncio-fast claims to be a drop-in replacement for pyserial-asyncio, it would be nice if it behaved the same way here.

Its intended to be but since its branched from a later point in after pyserial-asyncio stopped releasing its likely that commit that never made it out into release changed the behavior so its only a drop in replacement for the head commit there, and not the released version.

Ideally we fix pyserial-asyncio-fast to behave the same as it did before that commit was pulled in into pyserial-asyncio assuming that behavior is desirable.

[bdraco]

If I revert the unreleased pyserial-asyncio commit pyserial/pyserial-asyncio@7987e6e, the tests pass

[bdraco]

I think the issue might be that the test is using a socket to mock a serial connection but since that PR now sees socket:// its now using asyncio.open_connection. I don't know this library or use it so I may be way off, but not sure the scenario in the test can actually happen in production.

[bdraco]

Sadly I don't have the right test setup to be able to move this PR forward.

[cottsay]

Okay, it sounds like a change to pyserial-asyncio-fast would align its behavior with the released pyserial-asyncio. Wouldn't that be the right path forward here?

The intent of the test was to use as much of pyserial-asyncio as possible to mimic a real device. Using a socket serial connection worked great at the time. If that's no longer the case I can disable the test, but it would still probably be a good idea if pyserial-asyncio-fast was more compatible with the released version of pyserial-asyncio.

[bdraco]

Okay, it sounds like a change to pyserial-asyncio-fast would align its behavior with the released pyserial-asyncio. Wouldn't that be the right path forward here?

The intent of the test was to use as much of pyserial-asyncio as possible to mimic a real device. Using a socket serial connection worked great at the time. If that's no longer the case I can disable the test, but it would still probably be a good idea if pyserial-asyncio-fast was more compatible with the released version of pyserial-asyncio.

I don't think we can change it because it would likely regress the problems that were solved by that commit which Home Assistant uses for zha. @puddly -- sorry for the ping but I don't know the full history of that commit

[puddly]

We use our own wrapper in zigpy (https://github.com/zigpy/zigpy/blob/150e88c774c062e8321822437bc7101ca26f7dff/zigpy/serial.py#L40) and do not rely on pyserial-asyncio for network communication so you can change whatever is necessary.

I'll look more into this. It'd be best to maintain compatibility if possible.

The commit was created because pyserial performs a blocking time.sleep(0.2) when a network connection is lost and is also significantly slower than just an async TCP connection.

[puddly]

I believe this may be a bad mixing of asyncio.start_server, stream reader/writer, and closing semantics. The following change causes a deadlock:

diff --git a/test/mock_device.py b/test/mock_device.py
index c9b71ec..50bb30a 100644
--- a/test/mock_device.py
+++ b/test/mock_device.py
@@ -198,18 +198,8 @@ async def mock_device(
         return asyncio.wait_for(task, None)

     server = await asyncio.start_server(client_connected, host='127.0.0.1')
-    try:
+    async with server:
         yield server.sockets[0].getsockname()
-    finally:
-        server.close()
-    if not connections:
-        return
-
-    _, pending = await asyncio.wait(connections, timeout=0.1)
-    for task in pending:
-        task.cancel()
-    if pending:
-        await asyncio.wait(pending)

I believe the test case now fails because the TCP server is still actually in a listening state when the context manager exits. pyserial-asyncio's current behavior of introducing a 0.2s synchronous block on TCP disconnect may have hidden a cleanup bug.

[cottsay]

I switched the test we were having trouble with to use a pty instead of socket serial. Because the windows socket serial didn't work right to begin with, it actually yields better test coverage on Darwin anyway.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.14%. Comparing base (9f51048) to head (83601b0).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #34   +/-   ##
=======================================
  Coverage   87.14%   87.14%           
=======================================
  Files           7        7           
  Lines         809      809           
=======================================
  Hits          705      705           
  Misses        104      104           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.