cli: replace read_stream() with StreamRunner #5024
Merged
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
Rename `Progress.put()` to `Progress.write()`, to make it consistent with other output related interfaces: `PlayerOutput`, `FileOutput` and `HTTPServer`
Replace `read_stream()` with a new `StreamRunner` class, refactor stream reads and output writes, as well as the progress thread data feeding, and move player polling into a separate thread which closes the stream once the player process gets terminated/killed. This fixes the player polling issue, or rather the detection of its broken pipe while reading the stream, as stream read calls can stall for various reasons, e.g. due to segmented streams or the filtering of stream data which pauses the stream output and disables any timeouts. - Implement `StreamRunner` and `PlayerPollThread` classes in dedicated `streamlink_cli.streamrunner` module - Remove old `streamlink_cli.main.read_stream()` implementation - Keep the same log messages - Make `StreamRunner.run()` raise `OSError` on read/write error and catch `OSError`s in main module where `console.exit()` gets called - Remove `Progress.iter()`, as it's not needed anymore - Add extensive tests with full code coverage
bastimeyer
force-pushed
the
cli/streamrunner
branch
from
a8f8565 to
098a601
Compare
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.
Replace
read_stream()with a newStreamRunnerclass, refactor stream reads and output writes, as well as the progress thread data feeding, and move player polling into a separate thread which closes the stream once the player process gets terminated/killed.This fixes the player polling issue, or rather the detection of its broken pipe while reading the stream, as stream read calls can stall for various reasons, e.g. due to segmented streams or the filtering of stream data which pauses the stream output and disables any timeouts.
StreamRunnerandPlayerPollThreadclasses in dedicatedstreamlink_cli.streamrunnermodulestreamlink_cli.main.read_stream()implementationStreamRunner.run()raiseOSErroron read/write error and catchOSErrors in main module whereconsole.exit()gets calledProgress.iter(), as it's not needed anymoreResolves #4642
Replaces #4974
Follow-up of #5017
As described in #4642, this re-implements the heart of Streamlink's CLI, namely the main loop where the resolved stream gets read and where data gets written to the output. It is important that there are no issues in this new implementation, so before this gets merged, this needs to be double- and triple-checked. I've taken the time to write extensive tests which cover all code branches of the new implementation, but this doesn't mean that I didn't miss something.
As you can see in the code, there are a couple of TODO comments for stuff that should get changed and fixed in the future. For example, the error messages have not been changed in order to not introduce any breaking changes of the CLI output, which is one of the few parts of the CLI which is considered a "stable" interface for third party applications to use (e.g. the Twitch GUI).
The error handling itself also needs to get fixed at some point. This is basically kept from the old implementation, which simply called
console.exit(), but it's been moved to different parts of the main module now where theStreamRunnergets initialized. The entireconsole.exit()stuff is pretty bad, which I've already commented on a few times, and it should be replaced with proper exception propagation and one single point where the CLI exits on any error, so error handling can be done gracefully and proper tests can be written.The output implementations (unaffected by this PR) are also bad. The output instance which gets passed to the
StreamRunneris either aPlayerOutput,FileOutput, orHTTPServer, butHTTPServerdoesn't inherit the baseOutputclass and its interface.In regards to the new
StreamRunnerandPlayerPollThread, I've set the polling time to 0.5s, which should be fine.The stream will get closed as soon as the player polling fails, even if the stream output is paused, which is the main intention of these changes. However, this doesn't mean that Streamlink will terminate immediately, because closing the stream can still take a bit depending on the stream implementation. For example, segmented streams like HLS can still wait until all segment downloads have finished. This needs to be fixed separately, if possible. The PR however is already a major improvement.