pymodbus-dev / pymodbus

A full modbus protocol written in python
Other
2.16k stars 891 forks source link

eliminate asyncio.sleep() and replace time.sleep() with a timeout #2034

Closed alexrudd2 closed 4 months ago

alexrudd2 commented 4 months ago

asyncio sometimes behaves strangely on Windows, so longer delays were added to prevent flaky tests. However, time.sleep(10) will always wait the full 10s, even though we really only need to wait until the server is stopped with async_stop().

Using future.result with a timeout waits only as long as necessary. This speeds up the CI on Windows significantly.

Also, having an await asyncio.sleep() immediately after an await cls.active_server.server.shutdown() is redundant. Control flow will already yield back to the event loop with the first await

See https://github.com/pymodbus-dev/pymodbus/pull/2033#discussion_r1495400566

alexrudd2 commented 4 months ago

We have had severe problems in the past, but not forcing a task switch which is essentially what asyncio.sleep(0) does. Not having a task switch mean e.g. that the task is cancelled but not yet terminated, not always but sometimes (especially on windows).

_Originally posted by @janiversen in https://github.com/pymodbus-dev/pymodbus/pull/2033#discussion_r1496050039_

I believe you were talking about the sync code. asyncio.run_coroutine_threadsafe(cls.async_stop(), cls.active_server.loop) will only schedule async.stop() and not wait until termination. However, this should be avoided by the use of future.result. If not, the "health check" will catch any regressions:

https://github.com/pymodbus-dev/pymodbus/blob/a152762c247301152051bfad59dd2ab0ffab781c/test/conftest.py#L173-L174

janiversen commented 4 months ago

Looks like you are correct on that part.