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

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

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

alexrudd2
Copy link
Collaborator

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 #2033 (comment)

@janiversen janiversen merged commit a152762 into dev Feb 20, 2024
1 check passed
@janiversen janiversen deleted the future-result branch February 20, 2024 15:51
@alexrudd2
Copy link
Collaborator Author

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 #2033 (comment)

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:

pymodbus/test/conftest.py

Lines 173 to 174 in a152762

@pytest.fixture(name="system_health_check", autouse=True)
async def _check_system_health():

@janiversen
Copy link
Collaborator

Looks like you are correct on that part.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants