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

BUG: Tests on windows can be flaky #313

Closed
maxking opened this issue Nov 1, 2022 · 7 comments · Fixed by #315
Closed

BUG: Tests on windows can be flaky #313

maxking opened this issue Nov 1, 2022 · 7 comments · Fixed by #315

Comments

@maxking
Copy link
Contributor

maxking commented Nov 1, 2022

=========================== short test summary info ===========================
FAILED aiosmtpd/tests/test_main.py::TestMain::test_debug_3 - RuntimeError: Event loop stopped before Future completed.
================== 1 failed, 553 passed, 9 skipped in 52.16s ==================

https://github.com/aio-libs/aiosmtpd/actions/runs/3365228777/jobs/5580522249
https://github.com/aio-libs/aiosmtpd/actions/runs/3371138286/jobs/5593013500

@maxking
Copy link
Contributor Author

maxking commented Nov 1, 2022

I did some searches some time ago and I feel it is related to EvenLoop implementations on Windows: https://docs.python.org/3/library/asyncio-eventloop.html#event-loop-implementations

Asyncio uses a different, ProactorEventLoop by default on Windows, which could be causing some differences with Unix setups. I'll try to dig more to see if I can find something more concrete for it.

@maxking
Copy link
Contributor Author

maxking commented Nov 1, 2022

Many folks seem to work-around the issue by replacing the default ProactorEventLoop on windows with the SelectorEventLoop (default for Unix, but supports Windows too).

encode/httpx#914 (comment)

@waynew
Copy link
Collaborator

waynew commented Nov 1, 2022

@maxking would that be something easy to do at the very least just in the test suite when on Windows? My guess is that it should be but it's been a while since I looked at the code 😅 If it is, that seems like a reasonable thing to do - but we should definitely document it both in the documentation (e.g. "Just a warning folks, if you're using Windows you might run into problems here!") as well as note the why in the code.

But... unless there's a super obvious answer or fix in upstream asyncio, I'd be 👍 on merging a well documented workaround.

@maxking
Copy link
Contributor Author

maxking commented Nov 1, 2022

Searching a bit more, it looks like interaction between loop.run_until_complete() and loop.stop causes this RuntimeError where they both are fundamentally in-compatible.

We do however use loop.stop in various places in tests, I can find several in test_main.py. My thought is that the delays for loop.stop in loop.call_later(DELAY, loop.stop) is long enough that the original coro just finishes, which makes this error intermittent. It could just be that slowness in the coro, caused by either resource constraints on the CI machine, could invoke loop.stop causing this RuntimeError.

In our case, we are using the fixture autostop_loop, which uses AUTOSTOP_DELAY of 1 second. It seems like with GH Actions, 1second might not be enough. This fixture is applied to the test that keep failing (https://github.com/aio-libs/aiosmtpd/blob/master/aiosmtpd/tests/test_main.py#L193).

A rather simple fix would be to just bump this to 2 (or see how many failures we see with 1.5 and then go to 2 as they will add-up with multiple tests). If that sounds reasonable, I can open a PR with whatever updated delay you prefer.

@waynew
Copy link
Collaborator

waynew commented Nov 2, 2022

That makes sense - and I prefer that to changing out the event loop, if it solves our issue 👍

@maxking
Copy link
Contributor Author

maxking commented Nov 2, 2022

Awesome, I created #315 to bump it to 1.5s for now.

@pepoluan
Copy link
Collaborator

Since the PR is merged, let's close this :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants