mtest: improve interrupting tests on Win32 #14311
Open
+42
−10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the subsequent commit we're about to adapt how the event loop on Windows is terminated such that we can handle keyboard interrupts better on that platform. With this change, it can happen that we're trying to await the progress task of the `ConsoleLogger` class, but at that time the loop would've already been in cancelled state and thus awaiting the task fails. The end result is that we never print the summary at all in such cases. Prepare for this change by splitting up the logic to stop loggers in the event loop and printing the summary. Like this, we can call `finish()` on the logger outside of the event loop regardless of whether or not we have awaited whatever internal state the logger has. Signed-off-by: Patrick Steinhardt <[email protected]>
Interrupting tests on Windows is a bit of a mess right now: instead of cleanly shutting down, Meson tends to send you heaps of exceptions of various kinds. This seems to stem from the fact that on Windows, asyncio does not properly support handling of signals, and thus we don't install any signal handlers at all that would allow us to shut down the test processes. Work around the issue by using the low-level `run_until_complete()` function instead of `asyncio.run()`. This low-level function does not perform the same level of careful teardown, but due to that it also causes us to not spew out heaps of errors. We should likely roll back these changes if asyncio ever was to gain support for proper signal handling, or if we were able to ever address all of the exceptions. But until then, allowing for a clean shutdown that doesn't overwhelm the user with errors feels preferable. Signed-off-by: Patrick Steinhardt <[email protected]>
|
I don't understand asyncio that deeply myself but LGTM. |
|
I will check a bit against the python sources, thanks for proposing a solution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Interrupting tests on Windows is a bit of a mess right now: instead of
cleanly shutting down, Meson tends to send you heaps of exceptions of
various kinds. This seems to stem from the fact that on Windows, asyncio
does not properly support handling of signals, and thus we don't install
any signal handlers at all that would allow us to shut down the test
processes.
Work around the issue by using the low-level
run_until_complete()function instead of
asyncio.run(). This low-level function does notperform the same level of careful teardown, but due to that it also
causes us to not spew out heaps of errors.
We should likely roll back these changes if asyncio ever was to gain
support for proper signal handling, or if we were able to ever address
all of the exceptions. But until then, allowing for a clean shutdown
that doesn't overwhelm the user with errors feels preferable.
I'm not hugely familiar with asyncio, so I would very much ask you to be extra critical of the approach chosen in this PR. It works for me on Windows and I do see clean shutdown. One omission is that tests that have been cancelled do not get marked accordingly anymore, but first I'd like to get some feedback on the current approach before trying to fix that.