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

[rh:websockets] Migrate websockets to networking framework #7720

Merged
merged 30 commits into from
Nov 20, 2023

Conversation

coletdjnz
Copy link
Member

@coletdjnz coletdjnz commented Jul 29, 2023

#8361 blocks this. This requires websockets>=12 which is Py3.8+ only.

Moves our Websocket support (in the form of WebSocketsWrapper) into its own RequestHandler. This way extractors do not need to directly interact with the library.

As part of this I have also created a standard websocket framework - where the standard websocket response has send and recv functions.

Improvements

  • Deprecates and removes the Websocket wrapper from use (fixes WebsocketWrapper deprecation warning: remove loop argument #8439)
  • Unified websocket request interface not tied to any websocket library
  • Integration with yt-dlp networking options: cookies, headers, socket timeout, source address, print traffic, mtls, legacy server connect, no check cert, certifi, and proxies (socks proxy only) etc.

Known issues

  • No http proxy support. We'll need to add support for websocket-client to have proxies (among other things)
  • Older versions of websockets will not be supported as we move to the synchronous client.
  • Closing can hang due to the closing handshake. For now I've set close_timeout to 0, which isn't ideal.
  • Test suite improvements. For now I've just put websocket tests in another file.

Out of scope

  • Detecting if we can support websockets ahead of time and other helpful information for users.
  • Rewriting websocket fragment downloader to use the websocket handlers
  • Rewriting test suite
Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

Copilot Summary

🤖 Generated by Copilot at 04f8749

Summary

🌐🛠️🧪

This pull request adds support for websockets request handler and response in the networking module, and simplifies the websockets handling in some extractors. It adds the files _websockets.py and websocket.py that implement the websockets logic, and modifies the files test_networking.py, fc2.py, niconico.py, _legacy.py, _utils.py, and YoutubeDL.py to use the new websockets classes and methods. It also fixes a bug in the debug output of the downloader.

We're sailing on the web with websockets in our hand
We've moved the wrapper class to a new file and command
We've fixed the bugs and tests and made the code more neat
So heave away, me hearties, on the count of three

Walkthrough

  • Implement a websockets request handler and response for the downloader (link, link)
  • Use the websockets request handler and response in the FC2LiveIE and NiconicoLiveIE extractors, and remove the dependency on the websockets library (link, link, link, link, link, link)
  • Move the WebSocketsWrapper class from the _utils.py file to the _websockets.py file, and import it in the _legacy.py file for backward compatibility (link, link, link)
  • Add tests for the websockets request handler and response in the test_networking.py file, and test the validation of different URL schemes and options (link, link, link, link, link, link)
  • Print the request handlers used by the downloader in the debug header, and fix a bug in accessing the handlers attribute (link)

@pukkandan
Copy link
Member

  • older versions of websockets will not be supported as we move to the synchronous client.

Make sure to update requirements.txt. We could also try to detect in dependencies.py and issue warning?

@pukkandan

This comment was marked as outdated.

@coletdjnz

This comment was marked as outdated.

@coletdjnz

This comment was marked as outdated.

Comment on lines -937 to -938
if not websockets:
raise ExtractorError('websockets library is not available. Please install it.', expected=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to error early

Copy link
Member Author

@coletdjnz coletdjnz Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add an equivalent error in ydl.urlopen() when the unsupported request errors come back. Cause extractors shouldn't know about the underlying libs, and we may support many websocket libs in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will then still error only after the initial webpage request. Checking dependency and erroring before any request is better. We do the same for cryptodome deps

This comment was marked as outdated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add this to the "nice to have" list that we can work on later. It's not required for this to be merged imo. It requires a bit of work which is being started in #7595

@bashonly bashonly added the enhancement New feature or request label Jul 29, 2023
@bashonly bashonly linked an issue Oct 26, 2023 that may be closed by this pull request
10 tasks
test/test_networking.py Outdated Show resolved Hide resolved
yt_dlp/utils/_legacy.py Outdated Show resolved Hide resolved
@coletdjnz coletdjnz marked this pull request as ready for review November 4, 2023 04:38
yt_dlp/YoutubeDL.py Outdated Show resolved Hide resolved
coletdjnz and others added 3 commits November 9, 2023 07:56
Co-authored-by: pukkandan <pukkandan.ytdlp@gmail.com>
@coletdjnz coletdjnz changed the title [rh/websockets] Migrate websocket support to networking framework [rh:websockets] Migrate websockets to networking framework Nov 20, 2023
@coletdjnz coletdjnz merged commit ccfd70f into yt-dlp:master Nov 20, 2023
13 of 15 checks passed
@coletdjnz coletdjnz deleted the networking/websocket branch November 20, 2023 08:04
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
* Adds a basic WebSocket framework
* Introduces new minimum `websockets` version of 12.0
* Deprecates `WebSocketsWrapper`

Fixes yt-dlp#8439

Authored by: coletdjnz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

WebsocketWrapper deprecation warning: remove loop argument
3 participants