From 60051f18022675ad333f30a5171c45f8d1af04da Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 15 Jul 2024 13:43:11 -0400 Subject: [PATCH 1/4] Rewrite urllib3 retry warnings to be understandable, if possible Alright, so this is 100% a hack, but we don't really have any alternatives for improving the truly user-hostile warnings that urllib3 emits when retrying after a connection error. Upstream improvements aren't possible until we are able to catch up with urllib3 2.x, but we're stuck on the 1.x series until at least 2025 due to OpenSSL incompatibilties. Thus, the approach taken here is to install a logging filter on the connectionpool.py urllib3 module (don't worry, the hacks continue[^1]) which "sniffs" for the appropriate warnings and rewrites them as best as possible with the limited context urllib3 gives us. Under verbose mode, the original error is appended as a "here's all of the context, sorry that it's unreadable" aid. [^1]: I did have to resort to parsing the timeout error strings with regex in order to get the configured timeout (since logically they're set per request, not globally or per session... doing it this way results in less buggy behaviour and tests a lot less annoying to write). --- src/pip/_internal/network/session.py | 3 + src/pip/_internal/network/utils.py | 88 +++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/network/session.py b/src/pip/_internal/network/session.py index 1765b4f6bd7..aa817e732fc 100644 --- a/src/pip/_internal/network/session.py +++ b/src/pip/_internal/network/session.py @@ -43,6 +43,7 @@ from pip._internal.models.link import Link from pip._internal.network.auth import MultiDomainBasicAuth from pip._internal.network.cache import SafeFileCache +from pip._internal.network.utils import Urllib3RetryFilter # Import ssl from compat so the initial import occurs in only one place. from pip._internal.utils.compat import has_tls @@ -64,6 +65,8 @@ # Ignore warning raised when using --trusted-host. warnings.filterwarnings("ignore", category=InsecureRequestWarning) +# Install rewriting filter for urllib3's retrying warnings. +logging.getLogger("pip._vendor.urllib3.connectionpool").addFilter(Urllib3RetryFilter()) SECURE_ORIGINS: List[SecureOrigin] = [ # protocol, hostname, port diff --git a/src/pip/_internal/network/utils.py b/src/pip/_internal/network/utils.py index bba4c265e89..bb76e90a9bd 100644 --- a/src/pip/_internal/network/utils.py +++ b/src/pip/_internal/network/utils.py @@ -1,8 +1,15 @@ +import logging +import re +from http.client import RemoteDisconnected from typing import Dict, Generator +from pip._vendor import urllib3 from pip._vendor.requests.models import Response -from pip._internal.exceptions import NetworkConnectionError +from pip._internal.exceptions import ( + NetworkConnectionError, +) +from pip._internal.utils.logging import VERBOSE # The following comments and HTTP headers were originally added by # Donald Stufft in git commit 22c562429a61bb77172039e480873fb239dd8c03. @@ -27,6 +34,8 @@ DOWNLOAD_CHUNK_SIZE = 256 * 1024 +logger = logging.getLogger(__name__) + def raise_for_status(resp: Response) -> None: http_error_msg = "" @@ -96,3 +105,80 @@ def response_chunks( if not chunk: break yield chunk + + +class Urllib3RetryFilter: + """A logging filter which attempts to rewrite urllib3's retrying + warnings to be more readable and less technical. + + This is essentially one large hack. Please enjoy... + """ + + def filter(self, record: logging.LogRecord) -> bool: + # Attempt to "sniff out" the retrying warning. + if not isinstance(record.args, tuple): + return True + + retry = next( + (a for a in record.args if isinstance(a, urllib3.util.Retry)), None + ) + if record.levelno != logging.WARNING or retry is None: + # Not the right warning, leave it alone. + return True + + error = next((a for a in record.args if isinstance(a, Exception)), None) + if error is None: + # No error information available, leave it alone. + return True + + original_message = record.msg + if isinstance(error, urllib3.exceptions.NewConnectionError): + connection = error.pool + record.msg = f"failed to connect to {connection.host}" + if isinstance(connection, urllib3.connection.HTTPSConnection): + record.msg += " via HTTPS" + elif isinstance(connection, urllib3.connection.HTTPConnection): + record.msg += " via HTTP" + # After this point, urllib3 gives us very little information to work with + # so the rewritten warnings will be light on details. + elif isinstance(error, urllib3.exceptions.SSLError): + record.msg = "SSL verification failed" + elif isinstance(error, urllib3.exceptions.TimeoutError): + # Ugh. + pattern = r""" + timeout=(?P + \d+ # Whole number + (\.\d+)? # Decimal component (optional) + )""" + if match := re.search(pattern, str(error), re.VERBOSE): + timeout = match.group("value") + record.msg = f"server didn't respond within {timeout} seconds" + else: + record.msg = "server took too long to respond" + elif isinstance(error, urllib3.exceptions.ProtocolError): + try: + reason = error.args[1] + except IndexError: + pass + else: + if isinstance(reason, (RemoteDisconnected, ConnectionResetError)): + record.msg = "the connection was closed unexpectedly" + elif isinstance(error, urllib3.exceptions.ProxyError): + record.msg = "failed to connect to proxy" + + if record.msg != original_message: + # The total remaining retries is already decremented when this + # warning is raised. + retries_left = retry.total + 1 + if retries_left > 1: + record.msg += f", retrying {retries_left} more times" + elif retries_left == 1: + record.msg += ", retrying 1 last time" + + if logger.isEnabledFor(VERBOSE): + # As it's hard to provide enough detail, show the original + # error under verbose mode. + record.msg += f": {error!s}" + record.args = () + + return True From 03b1104d68a393eb4f7ba0233e21fdce5c8e5306 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 15 Jul 2024 13:51:06 -0400 Subject: [PATCH 2/4] Add four diagnostic connection errors - ConnectionFailedError - ConnectionTimeoutError - SSLVerificationError - ProxyConnectionError The first one is the most generic of the bunch. It's the error that is raised when is no better (or at least defined) error to raise. This sucks, but unfortunately requests and in particular urllib3 errors are terrible. They lack so much context that it is challenging to present specific, actionable errors. For example, there's no way to deduce that a urllib3 NewConnectionError is caused by DNS unless you inspect the error chain (for socket.gaierror) or parse the raw error string (which is too hacky for my taste). The other three are a lot better, although they are admittedly still rather vague. The SSLVerificationError can't discern whether it was caused by an expired or self-signed certificate for example. To work around this, I've opted to include the original error as additional context (with some limited rewriting if possible). Anyway, the main benefit here is that despite the coarseness, we're able to provide at least some actionable advice through the diagnostic hints. In addition, these errors are way more readable than whatever requests or urllib3 spits out (often being caught, stringified and presented in single-line screaming red by pip). --- src/pip/_internal/exceptions.py | 105 +++++++++++++++++++++++++++ src/pip/_internal/index/collector.py | 30 ++++---- src/pip/_internal/network/session.py | 7 +- src/pip/_internal/network/utils.py | 49 ++++++++++++- 4 files changed, 175 insertions(+), 16 deletions(-) diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py index 2587740f73a..970ac5bc220 100644 --- a/src/pip/_internal/exceptions.py +++ b/src/pip/_internal/exceptions.py @@ -12,6 +12,7 @@ import pathlib import re import sys +from http.client import RemoteDisconnected from itertools import chain, groupby, repeat from typing import TYPE_CHECKING, Dict, Iterator, List, Literal, Optional, Union @@ -22,6 +23,7 @@ if TYPE_CHECKING: from hashlib import _Hash + from pip._vendor import urllib3 from pip._vendor.requests.models import Request, Response from pip._internal.metadata import BaseDistribution @@ -312,6 +314,109 @@ def __str__(self) -> str: return str(self.error_msg) +class ConnectionFailedError(DiagnosticPipError): + reference = "connection-failed" + + def __init__(self, url: str, host: str, error: Exception) -> None: + from pip._vendor.urllib3.exceptions import NewConnectionError, ProtocolError + + details = str(error) + if isinstance(error, NewConnectionError): + parts = details.split("Failed to establish a new connection: ", maxsplit=1) + if len(parts) == 2: + _, details = parts + elif isinstance(error, ProtocolError): + try: + reason = error.args[1] + except IndexError: + pass + else: + if isinstance(reason, (RemoteDisconnected, ConnectionResetError)): + details = ( + "the connection was closed without a reply from the server." + ) + + super().__init__( + message=f"Failed to connect to [magenta]{host}[/] while fetching {url}", + context=Text(f"Details: {escape(details)}"), + hint_stmt=( + "This is likely a system or network issue. Are you connected to the " + "Internet? If so, check whether your system can connect to " + f"[magenta]{host}[/] before trying again. There may be a firewall or " + "proxy that's preventing the connection." + ), + ) + + +class ConnectionTimeoutError(DiagnosticPipError): + reference = "connection-timeout" + + def __init__( + self, url: str, host: str, *, kind: Literal["connect", "read"], timeout: float + ) -> None: + context = Text.assemble( + (host, "magenta"), f" didn't respond within {timeout} seconds" + ) + if kind == "connect": + context.append(" (while establishing a connection)") + super().__init__( + message=f"Unable to fetch {url}", + context=context, + hint_stmt=( + "This is probably a temporary issue with the remote server or the " + "network connection. If this error persists, check your system or " + "pip's network configuration. There may be a firewall or proxy that's " + "preventing the connection." + ), + ) + + +class SSLVerificationError(DiagnosticPipError): + reference = "ssl-verification-failed" + + def __init__( + self, + url: str, + host: str, + error: "urllib3.exceptions.SSLError", + *, + is_tls_available: bool, + ) -> None: + message = ( + "Failed to establish a secure connection to " + f"[magenta]{host}[/] while fetching {url}" + ) + if not is_tls_available: + # A lack of TLS support isn't guaranteed to be the cause of this error, + # but we'll assume that it's the culprit. + context = Text("The built-in ssl module is not available.") + hint = ( + "Your Python installation is missing SSL/TLS support, which is " + "required to access HTTPS URLs." + ) + else: + context = Text(f"Details: {escape(str(error))}") + hint = ( + "This was likely caused by the system or pip's network configuration." + ) + super().__init__(message=message, context=context, hint_stmt=hint) + + +class ProxyConnectionError(DiagnosticPipError): + reference = "proxy-connection-failed" + + def __init__( + self, url: str, proxy: str, error: "urllib3.exceptions.ProxyError" + ) -> None: + super().__init__( + message=( + f"Failed to connect to proxy [magenta]{proxy}[/] while fetching {url}" + ), + context=Text(f"Details: {escape(str(error.original_error))}"), + hint_stmt="This is likely a proxy configuration issue.", + ) + + class InvalidWheelFilename(InstallationError): """Invalid wheel filename.""" diff --git a/src/pip/_internal/index/collector.py b/src/pip/_internal/index/collector.py index 5f8fdee3d46..ef49fdffa61 100644 --- a/src/pip/_internal/index/collector.py +++ b/src/pip/_internal/index/collector.py @@ -28,11 +28,16 @@ Union, ) -from pip._vendor import requests from pip._vendor.requests import Response -from pip._vendor.requests.exceptions import RetryError, SSLError - -from pip._internal.exceptions import NetworkConnectionError +from pip._vendor.requests.exceptions import RetryError + +from pip._internal.exceptions import ( + ConnectionFailedError, + ConnectionTimeoutError, + NetworkConnectionError, + ProxyConnectionError, + SSLVerificationError, +) from pip._internal.models.link import Link from pip._internal.models.search_scope import SearchScope from pip._internal.network.session import PipSession @@ -365,17 +370,16 @@ def _get_index_content(link: Link, *, session: PipSession) -> Optional["IndexCon exc.request_desc, exc.content_type, ) - except NetworkConnectionError as exc: - _handle_get_simple_fail(link, exc) - except RetryError as exc: + except (RetryError, NetworkConnectionError) as exc: _handle_get_simple_fail(link, exc) - except SSLError as exc: - reason = "There was a problem confirming the ssl certificate: " - reason += str(exc) + except SSLVerificationError as exc: + reason = f"There was a problem confirming the ssl certificate: {exc.context}" _handle_get_simple_fail(link, reason, meth=logger.info) - except requests.ConnectionError as exc: - _handle_get_simple_fail(link, f"connection error: {exc}") - except requests.Timeout: + except ConnectionFailedError as exc: + _handle_get_simple_fail(link, f"connection error: {exc.context}") + except ProxyConnectionError as exc: + _handle_get_simple_fail(link, f"proxy connection error: {exc.context}") + except ConnectionTimeoutError: _handle_get_simple_fail(link, "timed out") else: return _make_index_content(resp, cache_link_parsing=link.cache_link_parsing) diff --git a/src/pip/_internal/network/session.py b/src/pip/_internal/network/session.py index aa817e732fc..f145b27b14e 100644 --- a/src/pip/_internal/network/session.py +++ b/src/pip/_internal/network/session.py @@ -43,7 +43,7 @@ from pip._internal.models.link import Link from pip._internal.network.auth import MultiDomainBasicAuth from pip._internal.network.cache import SafeFileCache -from pip._internal.network.utils import Urllib3RetryFilter +from pip._internal.network.utils import Urllib3RetryFilter, raise_connection_error # Import ssl from compat so the initial import occurs in only one place. from pip._internal.utils.compat import has_tls @@ -522,4 +522,7 @@ def request(self, method: str, url: str, *args: Any, **kwargs: Any) -> Response: kwargs.setdefault("proxies", self.proxies) # Dispatch the actual request - return super().request(method, url, *args, **kwargs) + try: + return super().request(method, url, *args, **kwargs) + except requests.ConnectionError as e: + raise_connection_error(e, timeout=kwargs["timeout"]) diff --git a/src/pip/_internal/network/utils.py b/src/pip/_internal/network/utils.py index bb76e90a9bd..bd2d652095b 100644 --- a/src/pip/_internal/network/utils.py +++ b/src/pip/_internal/network/utils.py @@ -1,14 +1,20 @@ import logging import re +import urllib.parse from http.client import RemoteDisconnected from typing import Dict, Generator -from pip._vendor import urllib3 +from pip._vendor import requests, urllib3 from pip._vendor.requests.models import Response from pip._internal.exceptions import ( + ConnectionFailedError, + ConnectionTimeoutError, NetworkConnectionError, + ProxyConnectionError, + SSLVerificationError, ) +from pip._internal.utils.compat import has_tls from pip._internal.utils.logging import VERBOSE # The following comments and HTTP headers were originally added by @@ -107,6 +113,47 @@ def response_chunks( yield chunk +def raise_connection_error(error: requests.ConnectionError, *, timeout: float) -> None: + """Raise a specific error for a given ConnectionError, if possible. + + Note: requests.ConnectionError is the parent class of + requests.ProxyError, requests.SSLError, and requests.ConnectTimeout + so these errors are also handled here. In addition, a ReadTimeout + wrapped in a requests.MayRetryError is converted into a + ConnectionError by requests internally. + """ + url = error.request.url + reason = error.args[0] + if not isinstance(reason, urllib3.exceptions.MaxRetryError): + # This is unlikely (or impossible as even --retries 0 still results in a + # MaxRetryError...?!), but being defensive can't hurt. + host = urllib.parse.urlsplit(url).netloc + raise ConnectionFailedError(url, host, reason) + + assert isinstance(reason.pool, urllib3.connectionpool.HTTPConnectionPool) + host = reason.pool.host + proxy = reason.pool.proxy + # Narrow the reason further to the specific error from the last retry. + reason = reason.reason + + if isinstance(reason, urllib3.exceptions.SSLError): + raise SSLVerificationError(url, host, reason, is_tls_available=has_tls()) + # NewConnectionError is a subclass of TimeoutError for some reason... + if isinstance(reason, urllib3.exceptions.TimeoutError) and not isinstance( + reason, urllib3.exceptions.NewConnectionError + ): + if isinstance(reason, urllib3.exceptions.ConnectTimeoutError): + raise ConnectionTimeoutError(url, host, kind="connect", timeout=timeout) + else: + raise ConnectionTimeoutError(url, host, kind="read", timeout=timeout) + if isinstance(reason, urllib3.exceptions.ProxyError): + assert proxy is not None + raise ProxyConnectionError(url, str(proxy), reason) + + # Unknown error, give up and raise a generic error. + raise ConnectionFailedError(url, host, reason) + + class Urllib3RetryFilter: """A logging filter which attempts to rewrite urllib3's retrying warnings to be more readable and less technical. From 52db1c9ab6286d243a6185f767f815bfdea3245a Mon Sep 17 00:00:00 2001 From: Richard Si Date: Sat, 20 Jul 2024 22:19:51 -0400 Subject: [PATCH 3/4] Add unit tests for urllib3 warning rewriting Frankly, this is essentially guaranteed to break the next time we upgrade urllib3 (esp. as we're due for a major bump to the 2.x series), but these unit tests serve to ensure the rewriting filters works *now* and demonstrates in what situations it does work. --- src/pip/_internal/network/utils.py | 2 +- tests/lib/server.py | 15 ++++++- tests/unit/test_network_session.py | 66 +++++++++++++++++++++++++++++- 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/network/utils.py b/src/pip/_internal/network/utils.py index bd2d652095b..47aaa3bddad 100644 --- a/src/pip/_internal/network/utils.py +++ b/src/pip/_internal/network/utils.py @@ -154,7 +154,7 @@ def raise_connection_error(error: requests.ConnectionError, *, timeout: float) - raise ConnectionFailedError(url, host, reason) -class Urllib3RetryFilter: +class Urllib3RetryFilter(logging.Filter): """A logging filter which attempts to rewrite urllib3's retrying warnings to be more readable and less technical. diff --git a/tests/lib/server.py b/tests/lib/server.py index 96ac5930dc9..99a196de783 100644 --- a/tests/lib/server.py +++ b/tests/lib/server.py @@ -3,8 +3,9 @@ import threading from base64 import b64encode from contextlib import ExitStack, contextmanager +from http.server import BaseHTTPRequestHandler, HTTPServer from textwrap import dedent -from typing import TYPE_CHECKING, Any, Callable, Dict, Iterable, Iterator, List +from typing import TYPE_CHECKING, Any, Callable, Dict, Iterable, Iterator, List, Union from unittest.mock import Mock from werkzeug.serving import BaseWSGIServer, WSGIRequestHandler @@ -104,7 +105,7 @@ def make_mock_server(**kwargs: Any) -> _MockServer: @contextmanager -def server_running(server: BaseWSGIServer) -> Iterator[None]: +def server_running(server: Union[BaseWSGIServer, HTTPServer]) -> Iterator[None]: """Context manager for running the provided server in a separate thread.""" thread = threading.Thread(target=server.serve_forever) thread.daemon = True @@ -232,3 +233,13 @@ def get_requests(self) -> List[Dict[str, str]]: # Legacy: replace call[0][0] with call.args[0] # when pip drops support for python3.7 return [call[0][0] for call in self._server.mock.call_args_list] + + +class InstantCloseHTTPHandler(BaseHTTPRequestHandler): + """A HTTP request handler that closes the underlying request TCP + socket immediately. Used for testing "connection reset by peer" and + similar errors. + """ + + def handle(self) -> None: + self.request.close() diff --git a/tests/unit/test_network_session.py b/tests/unit/test_network_session.py index 3f1a596ad8e..f8125ddce2e 100644 --- a/tests/unit/test_network_session.py +++ b/tests/unit/test_network_session.py @@ -1,7 +1,8 @@ import logging import os +from http.server import HTTPServer from pathlib import Path -from typing import Any, List, Optional +from typing import Any, Iterator, List, Optional from urllib.parse import urlparse from urllib.request import getproxies @@ -9,12 +10,15 @@ from pip._vendor import requests from pip import __version__ +from pip._internal.exceptions import DiagnosticPipError from pip._internal.models.link import Link from pip._internal.network.session import ( CI_ENVIRONMENT_VARIABLES, PipSession, user_agent, ) +from pip._internal.utils.logging import VERBOSE +from tests.lib.server import InstantCloseHTTPHandler, server_running def get_user_agent() -> str: @@ -281,3 +285,63 @@ def test_proxy(self, proxy: Optional[str]) -> None: f"Invalid proxy {proxy} or session.proxies: " f"{session.proxies} is not correctly passed to session.request." ) + + +@pytest.mark.network +class TestRetryWarningRewriting: + @pytest.fixture(autouse=True) + def setup_caplog_level(self, caplog: pytest.LogCaptureFixture) -> Iterator[None]: + with caplog.at_level(logging.WARNING): + yield + + @pytest.mark.parametrize( + "url, expected_message", + [ + ( + "https://404.example.com", + "failed to connect to 404.example.com via HTTPS", + ), + ("http://404.example.com", "failed to connect to 404.example.com via HTTP"), + ("https://expired.badssl.com", "SSL verification failed"), + ], + ) + def test_simple_urls( + self, caplog: pytest.LogCaptureFixture, url: str, expected_message: str + ) -> None: + with PipSession(retries=1) as session: + with pytest.raises(DiagnosticPipError): + session.get(url) + assert caplog.messages == [f"{expected_message}, retrying 1 last time"] + + def test_timeout(self, caplog: pytest.LogCaptureFixture) -> None: + with PipSession(retries=1) as session: + with pytest.raises(DiagnosticPipError): + session.get("https://httpstat.us/200?sleep=400", timeout=0.2) + assert caplog.messages == [ + "server didn't respond within 0.2 seconds, retrying 1 last time" + ] + + def test_connection_aborted(self, caplog: pytest.LogCaptureFixture) -> None: + with HTTPServer(("localhost", 0), InstantCloseHTTPHandler) as server: + with server_running(server), PipSession(retries=1) as session: + with pytest.raises(DiagnosticPipError): + session.get(f"http://{server.server_name}:{server.server_port}/") + assert caplog.messages == [ + "the connection was closed unexpectedly, retrying 1 last time" + ] + + def test_proxy(self, caplog: pytest.LogCaptureFixture) -> None: + with PipSession(retries=1) as session: + session.proxies = {"https": "https://404.example.com"} + with pytest.raises(DiagnosticPipError): + session.get("https://pypi.org") + assert caplog.messages == ["failed to connect to proxy, retrying 1 last time"] + + def test_verbose(self, caplog: pytest.LogCaptureFixture) -> None: + caplog.set_level(VERBOSE) + with PipSession(retries=1) as session: + with pytest.raises(DiagnosticPipError): + session.get("https://404.example.org") + warnings = [r.message for r in caplog.records if r.levelno == logging.WARNING] + assert len(warnings) == 1 + assert not warnings[0].endswith("retrying 1 last time") From 36ccb3473ec1e18b6cb5afac6574b85c010aaa69 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 22 Jul 2024 23:03:49 -0400 Subject: [PATCH 4/4] Add unit tests for new connection errors Also refactor two testing utilities: - I moved the rendered() helper which converts a rich renderable (or str equivalent) into pure text from test_exceptions.py to the test library. I also enabled soft wrap to make it a bit nicer for testing single sentences renderables (although it may make sense to make it configurable later on). - I extracted the "instant close" HTTP server into its own fixture so it could be easily reused for a connection error unit test as well. --- tests/lib/output.py | 19 +++++ tests/unit/test_exceptions.py | 12 +-- tests/unit/test_network_session.py | 128 ++++++++++++++++++++++++++--- 3 files changed, 138 insertions(+), 21 deletions(-) create mode 100644 tests/lib/output.py diff --git a/tests/lib/output.py b/tests/lib/output.py new file mode 100644 index 00000000000..fc819184489 --- /dev/null +++ b/tests/lib/output.py @@ -0,0 +1,19 @@ +from io import StringIO + +from pip._vendor.rich.console import Console, RenderableType + + +def render_to_text( + renderable: RenderableType, + *, + color: bool = False, +) -> str: + with StringIO() as stream: + console = Console( + force_terminal=False, + file=stream, + color_system="truecolor" if color else None, + soft_wrap=True, + ) + console.print(renderable) + return stream.getvalue() diff --git a/tests/unit/test_exceptions.py b/tests/unit/test_exceptions.py index 6510b569e5f..e11e7f49135 100644 --- a/tests/unit/test_exceptions.py +++ b/tests/unit/test_exceptions.py @@ -12,6 +12,7 @@ from pip._vendor import rich from pip._internal.exceptions import DiagnosticPipError, ExternallyManagedEnvironment +from tests.lib.output import render_to_text as rendered class TestDiagnosticPipErrorCreation: @@ -274,17 +275,6 @@ def test_no_hint_no_note_no_context(self) -> None: ) -def rendered(error: DiagnosticPipError, *, color: bool = False) -> str: - with io.StringIO() as stream: - console = rich.console.Console( - force_terminal=False, - file=stream, - color_system="truecolor" if color else None, - ) - console.print(error) - return stream.getvalue() - - class TestDiagnosticPipErrorPresentation_Unicode: def test_complete(self) -> None: err = DiagnosticPipError( diff --git a/tests/unit/test_network_session.py b/tests/unit/test_network_session.py index f8125ddce2e..a904eaf4740 100644 --- a/tests/unit/test_network_session.py +++ b/tests/unit/test_network_session.py @@ -1,8 +1,11 @@ import logging import os +import sys +from dataclasses import dataclass from http.server import HTTPServer from pathlib import Path -from typing import Any, Iterator, List, Optional +from typing import Any, Iterator, List, Optional, Tuple +from unittest.mock import patch from urllib.parse import urlparse from urllib.request import getproxies @@ -10,7 +13,13 @@ from pip._vendor import requests from pip import __version__ -from pip._internal.exceptions import DiagnosticPipError +from pip._internal.exceptions import ( + ConnectionFailedError, + ConnectionTimeoutError, + DiagnosticPipError, + ProxyConnectionError, + SSLVerificationError, +) from pip._internal.models.link import Link from pip._internal.network.session import ( CI_ENVIRONMENT_VARIABLES, @@ -18,9 +27,34 @@ user_agent, ) from pip._internal.utils.logging import VERBOSE +from tests.lib.output import render_to_text from tests.lib.server import InstantCloseHTTPHandler, server_running +def render_diagnostic_error(error: DiagnosticPipError) -> Tuple[str, Optional[str]]: + message = render_to_text(error.message).rstrip() + if error.context is None: + return (message, None) + return (message, render_to_text(error.context).rstrip()) + + +@dataclass(frozen=True) +class Address: + host: str + port: int + + @property + def url(self) -> str: + return f"http://{self.host}:{self.port}/" + + +@pytest.fixture(scope="module") +def instant_close_server() -> Iterator[Address]: + with HTTPServer(("127.0.0.1", 0), InstantCloseHTTPHandler) as server: + with server_running(server): + yield Address(server.server_name, server.server_port) + + def get_user_agent() -> str: # These tests are testing the computation of the user agent, so we want to # avoid reusing cached values. @@ -321,14 +355,18 @@ def test_timeout(self, caplog: pytest.LogCaptureFixture) -> None: "server didn't respond within 0.2 seconds, retrying 1 last time" ] - def test_connection_aborted(self, caplog: pytest.LogCaptureFixture) -> None: - with HTTPServer(("localhost", 0), InstantCloseHTTPHandler) as server: - with server_running(server), PipSession(retries=1) as session: - with pytest.raises(DiagnosticPipError): - session.get(f"http://{server.server_name}:{server.server_port}/") - assert caplog.messages == [ - "the connection was closed unexpectedly, retrying 1 last time" - ] + @pytest.mark.skipif( + sys.platform != "linux", reason="Only Linux raises the needed urllib3 error" + ) + def test_connection_closed_by_peer( + self, caplog: pytest.LogCaptureFixture, instant_close_server: Address + ) -> None: + with PipSession(retries=1) as session: + with pytest.raises(DiagnosticPipError): + session.get(instant_close_server.url) + assert caplog.messages == [ + "the connection was closed unexpectedly, retrying 1 last time" + ] def test_proxy(self, caplog: pytest.LogCaptureFixture) -> None: with PipSession(retries=1) as session: @@ -345,3 +383,73 @@ def test_verbose(self, caplog: pytest.LogCaptureFixture) -> None: warnings = [r.message for r in caplog.records if r.levelno == logging.WARNING] assert len(warnings) == 1 assert not warnings[0].endswith("retrying 1 last time") + + +@pytest.mark.network +class TestConnectionErrors: + @pytest.fixture + def session(self) -> Iterator[PipSession]: + with PipSession() as session: + yield session + + def test_non_existent_domain(self, session: PipSession) -> None: + url = "https://404.example.com/" + with pytest.raises(ConnectionFailedError) as e: + session.get(url) + message, _ = render_diagnostic_error(e.value) + assert message == f"Failed to connect to 404.example.com while fetching {url}" + + @pytest.mark.skipif( + sys.platform != "linux", reason="Only Linux raises the needed urllib3 error" + ) + def test_connection_closed_by_peer( + self, session: PipSession, instant_close_server: Address + ) -> None: + with pytest.raises(ConnectionFailedError) as e: + session.get(instant_close_server.url) + message, context = render_diagnostic_error(e.value) + assert message == ( + f"Failed to connect to {instant_close_server.host} " + f"while fetching {instant_close_server.url}" + ) + assert context == ( + "Details: the connection was closed without a reply from the server." + ) + + def test_timeout(self, session: PipSession) -> None: + url = "https://httpstat.us/200?sleep=400" + with pytest.raises(ConnectionTimeoutError) as e: + session.get(url, timeout=0.2) + message, context = render_diagnostic_error(e.value) + assert message == f"Unable to fetch {url}" + assert context is not None + assert context.startswith("httpstat.us didn't respond within 0.2 seconds") + + def test_expired_ssl(self, session: PipSession) -> None: + url = "https://expired.badssl.com/" + with pytest.raises(SSLVerificationError) as e: + session.get(url) + message, _ = render_diagnostic_error(e.value) + assert message == ( + "Failed to establish a secure connection to expired.badssl.com " + f"while fetching {url}" + ) + + @patch("pip._internal.network.utils.has_tls", lambda: False) + def test_missing_python_ssl_support(self, session: PipSession) -> None: + # So, this demonstrates a potentially invalid assumption: a SSL error + # when SSL support is missing is assumed to be caused by that. Not ideal + # but unlikely to cause issues in practice. + with pytest.raises(SSLVerificationError) as e: + session.get("https://expired.badssl.com/") + _, context = render_diagnostic_error(e.value) + assert context == "The built-in ssl module is not available." + + def test_broken_proxy(self, session: PipSession) -> None: + url = "https://pypi.org/" + proxy = "https://404.example.com" + session.proxies = {"https": proxy} + with pytest.raises(ProxyConnectionError) as e: + session.get(url) + message, _ = render_diagnostic_error(e.value) + assert message == f"Failed to connect to proxy {proxy}:443 while fetching {url}"