Rewrite asyncio-based utils.processoutput code based on trio and remove pytest-asyncio dev-requirement
#6208
Conversation
This file contains hidden or 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
This comment was marked as outdated.
This comment was marked as outdated.
- Replace asyncio-based code with trio-based code - Gracefully terminate process instead of just killing it - Run actual subprocess in tests
bastimeyer
force-pushed
the
remove-asyncio
branch
from
11d874d to
eb76da1
Compare
5 tasks
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.
The
utils.processoutpututility is currently used byFFMPEGMuxerto validate the ffmpeg binary by reading its version string and compile flags.This was previously done using
asynciofrom the stdlib, hence the requirement ofpytest-asyncioin the tests. The only other tests making use of theasynciorun loop were the CLI'sStreamRunnertests (just for convenience though) and thehandshaketest utilities for making assertions about thread synchronization.Since Streamlink's webbrowser API is based on
trio, it doesn't make sense mixing async run loops from two different libs. On Windows, this also raises a warning when running Streamlink's testsuite.The rewritten
utils.processoutputmodule and its tests are now much cleaner and easier to read due to trio's structured concurrency. No other changes were made here apart from gracefully terminating the spawned subprocess now, instead of just killing it when all tasks were completed (in case it was still running).A nice side effect of the
pytest-asyncioremoval is that it reduces the run time of the whole testsuite on my Arch Linux host system from 9s to 7s, and in my W10 VM from 35s to 29s.I will have another look at the diff later before merging, in case I missed something.