Skip to content

Commit

Permalink
[networking] Rewrite architecture (#2861)
Browse files Browse the repository at this point in the history 
New networking interface consists of a `RequestDirector` that directs
each `Request` to appropriate `RequestHandler` and returns the
`Response` or raises `RequestError`. The handlers define adapters to
transform its internal Request/Response/Errors to our interfaces.

User-facing changes:
- Fix issues with per request proxies on redirects for urllib
- Support for `ALL_PROXY` environment variable for proxy setting
- Support for `socks5h` proxy
   - Closes #6325, ytdl-org/youtube-dl#22618, ytdl-org/youtube-dl#28093
- Raise error when using `https` proxy instead of silently converting it to `http`

Authored by: coletdjnz
  • Loading branch information
@coletdjnz @pukkandan
coletdjnz authored and pukkandan committed Jul 15, 2023
1 parent c365dba commit 227bf1a
Show file tree
Hide file tree
Showing 16 changed files with 2,593 additions and 481 deletions.
9 changes: 3 additions & 6 deletions test/test_download.py
View file
Expand Up @@ -10,10 +10,7 @@

import collections
import hashlib
import http.client
import json
import socket
import urllib.error

from test.helper import (
assertGreaterEqual,
Expand All @@ -29,6 +26,7 @@

import yt_dlp.YoutubeDL # isort: split
from yt_dlp.extractor import get_info_extractor
from yt_dlp.networking.exceptions import HTTPError, TransportError
from yt_dlp.utils import (
DownloadError,
ExtractorError,
Expand Down Expand Up @@ -162,8 +160,7 @@ def try_rm_tcs_files(tcs=None):
force_generic_extractor=params.get('force_generic_extractor', False))
except (DownloadError, ExtractorError) as err:
# Check if the exception is not a network related one
if (err.exc_info[0] not in (urllib.error.URLError, socket.timeout, UnavailableVideoError, http.client.BadStatusLine)
or (err.exc_info[0] == urllib.error.HTTPError and err.exc_info[1].code == 503)):
if not isinstance(err.exc_info[1], (TransportError, UnavailableVideoError)) or (isinstance(err.exc_info[1], HTTPError) and err.exc_info[1].code == 503):
err.msg = f'{getattr(err, "msg", err)} ({tname})'
raise

Expand Down Expand Up @@ -249,7 +246,7 @@ def try_rm_tcs_files(tcs=None):
# extractor returns full results even with extract_flat
res_tcs = [{'info_dict': e} for e in res_dict['entries']]
try_rm_tcs_files(res_tcs)

ydl.close()
return test_template


Expand Down

3 comments on commit 227bf1a

@ortango
Copy link

Choose a reason for hiding this comment

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

# XXX: could there be a case where system certs are the same as certifi?

i believe this is the case with archlinux. ref

@samoht0
Copy link

Choose a reason for hiding this comment

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

XXX: could there be a case where system certs are the same as certifi?

i believe this is the case with archlinux. ref

It's definitely the case for Fedora:

Please note that this Fedora package does not actually include a certificate
collection at all. It reads the system shared certificate trust collection
instead. For more details on this system, see the ca-certificates package.

https://src.fedoraproject.org/rpms/python-certifi

TestNetworkingUtils.test_load_certifi broken for me.

____________________ TestNetworkingUtils.test_load_certifi _____________________

self = <test.test_networking_utils.TestNetworkingUtils object at 0x7f6f44717410>

    @pytest.mark.skipif(not certifi, reason='certifi is not installed')
    def test_load_certifi(self):
        context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
        context2 = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
        ssl_load_certs(context, use_certifi=True)
        context2.load_verify_locations(cafile=certifi.where())
        assert context.get_ca_certs() == context2.get_ca_certs()
    
        # Test load normal certs
        # XXX: could there be a case where system certs are the same as certifi?
        context3 = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
        ssl_load_certs(context3, use_certifi=False)
>       assert context3.get_ca_certs() != context.get_ca_certs()
E       AssertionError: assert [{'issuer': ((('countryName', 'FR'),), (('organizationName', 'Dhimyotis'),), (('commonName', 'Certigna'),)), 'notAfter...040 GMT', 'notBefore': 'May 26 00:00:00 2015 GMT', 'serialNumber': '066C9FD5749736663F3B0B9AD9E89E7603F24A', ...}, ...] != [{'issuer': ((('countryName', 'FR'),), (('organizationName', 'Dhimyotis'),), (('commonName', 'Certigna'),)), 'notAfter...040 GMT', 'notBefore': 'May 26 00:00:00 2015 GMT', 'serialNumber': '066C9FD5749736663F3B0B9AD9E89E7603F24A', ...}, ...]
E        +  where [{'issuer': ((('countryName', 'FR'),), (('organizationName', 'Dhimyotis'),), (('commonName', 'Certigna'),)), 'notAfter...040 GMT', 'notBefore': 'May 26 00:00:00 2015 GMT', 'serialNumber': '066C9FD5749736663F3B0B9AD9E89E7603F24A', ...}, ...] = <built-in method get_ca_certs of SSLContext object at 0x7f6f44ae7140>()
E        +    where <built-in method get_ca_certs of SSLContext object at 0x7f6f44ae7140> = <ssl.SSLContext object at 0x7f6f44ae7140>.get_ca_certs
E        +  and   [{'issuer': ((('countryName', 'FR'),), (('organizationName', 'Dhimyotis'),), (('commonName', 'Certigna'),)), 'notAfter...040 GMT', 'notBefore': 'May 26 00:00:00 2015 GMT', 'serialNumber': '066C9FD5749736663F3B0B9AD9E89E7603F24A', ...}, ...] = <built-in method get_ca_certs of SSLContext object at 0x7f6f4493fc80>()
E        +    where <built-in method get_ca_certs of SSLContext object at 0x7f6f4493fc80> = <ssl.SSLContext object at 0x7f6f4493fc80>.get_ca_certs

test/test_networking_utils.py:108: AssertionError

@bashonly
Copy link
Member

Choose a reason for hiding this comment

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

same test failure on arch

Please sign in to comment.