Maksim ShulgaandMaxim Shulga
a2887fa6db
* Bound HTTP request timeouts and retry budget With no request timeout, Retry(total=30, backoff_factor=2, backoff_max=60) let a single unreachable server hang the client for around 26 minutes: backoff sleeps alone sum to that, and each of the 30 connection attempts could itself stall indefinitely without a timeout. Measured a black-holed connect attempt taking 75s on this machine with no timeout set, confirming the mechanism. - Add HTTP_TIMEOUT (connect, read) and a tighter retry budget (HTTP_RETRY_TOTAL/BACKOFF_FACTOR/BACKOFF_MAX) in dsc/const.py, bounding the worst case to a few minutes. - Pass timeout=HTTP_TIMEOUT on both SeafileClient HTTP calls (token fetch, remote library listing). - Add NetworkError(DscError) so a network failure is caught by start.py's existing top-level DscError handling instead of producing a bare traceback. - Add tests/test_http.py covering the retry policy values, timeout propagation, and error wrapping. - Check off the corresponding IMPROVEMENTS.md items. * Include URL and status code in NetworkError messages Copilot review on PR #28 flagged that the non-200 path for remote library listing raised NetworkError with only the raw response body, losing which request failed and its status code. The token-fetch error had the same gap. Both now include the URL and HTTP status. --------- Co-authored-by: Maxim Shulga <maxim.shulga@alludo.com>
74 lines
2.8 KiB
Python
74 lines
2.8 KiB
Python
"""HTTP requests to the Seafile server must be bounded, not hang forever.
|
|
|
|
Regression coverage for a measured issue: with no request timeout, the
|
|
retry policy waited about 26 minutes against an unreachable host before
|
|
giving up. See dsc/const.py for the bounds this enforces.
|
|
"""
|
|
|
|
from unittest import mock
|
|
|
|
import pytest
|
|
import requests
|
|
|
|
from dsc import const
|
|
from dsc.client import SeafileClient
|
|
from dsc.errors import NetworkError
|
|
|
|
|
|
@pytest.fixture
|
|
def client():
|
|
return SeafileClient("seafile.example", "user", "pw", "/dsc")
|
|
|
|
|
|
def test_client_mounts_a_bounded_retry_policy(client):
|
|
adapter = client.session.get_adapter("https://seafile.example")
|
|
retry = adapter.max_retries
|
|
assert retry.total == const.HTTP_RETRY_TOTAL
|
|
assert retry.backoff_factor == const.HTTP_RETRY_BACKOFF_FACTOR
|
|
assert retry.backoff_max == const.HTTP_RETRY_BACKOFF_MAX
|
|
|
|
|
|
def test_fetching_a_token_passes_a_request_timeout(client):
|
|
with mock.patch.object(client.session, "post") as post:
|
|
post.return_value = mock.Mock(status_code=200, json=lambda: {"token": "tok"})
|
|
assert client.token == "tok"
|
|
_, kwargs = post.call_args
|
|
assert kwargs["timeout"] == const.HTTP_TIMEOUT
|
|
|
|
|
|
def test_fetching_a_token_wraps_a_network_failure(client):
|
|
with mock.patch.object(client.session, "post", side_effect=requests.exceptions.Timeout):
|
|
with pytest.raises(NetworkError):
|
|
_ = client.token
|
|
|
|
|
|
def test_fetching_a_token_reports_url_and_status_on_an_error_response(client):
|
|
with mock.patch.object(client.session, "post") as post:
|
|
post.return_value = mock.Mock(status_code=403, text="Forbidden")
|
|
with pytest.raises(NetworkError, match="auth-token.*403.*Forbidden"):
|
|
_ = client.token
|
|
|
|
|
|
def test_listing_remote_libraries_passes_a_request_timeout(client, monkeypatch):
|
|
monkeypatch.setattr(type(client), "token", "tok")
|
|
with mock.patch.object(client.session, "get") as get:
|
|
get.return_value = mock.Mock(status_code=200, json=lambda: [])
|
|
assert client.remote_libraries == {}
|
|
_, kwargs = get.call_args
|
|
assert kwargs["timeout"] == const.HTTP_TIMEOUT
|
|
|
|
|
|
def test_listing_remote_libraries_wraps_a_network_failure(client, monkeypatch):
|
|
monkeypatch.setattr(type(client), "token", "tok")
|
|
with mock.patch.object(client.session, "get", side_effect=requests.exceptions.ConnectionError):
|
|
with pytest.raises(NetworkError):
|
|
_ = client.remote_libraries
|
|
|
|
|
|
def test_listing_remote_libraries_reports_url_and_status_on_an_error_response(client, monkeypatch):
|
|
monkeypatch.setattr(type(client), "token", "tok")
|
|
with mock.patch.object(client.session, "get") as get:
|
|
get.return_value = mock.Mock(status_code=500, text="Internal Server Error")
|
|
with pytest.raises(NetworkError, match="api2/repos.*500.*Internal Server Error"):
|
|
_ = client.remote_libraries
|