From 4029a90466595bd069afad12fcf6c25471801925 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sat, 21 Mar 2026 10:02:40 +0100 Subject: [PATCH] fix: catch and retry on ConnectionError/Timeout from niquests (issue #647) niquests uses lazy HTTP/2 response gathering: accessing r.status_code or r.headers can raise ConnectionError if the underlying connection has gone stale. This produces a confusing nested-exception traceback and an intermittent failure for callers (c.events(), etc.). Changes: - Add DAVNetworkError(DAVError) to caldav/lib/error.py to represent network-level failures wrapping the underlying library exception. - In DAVClient._sync_request(), wrap the session.request() call and all subsequent response-attribute access in a try/except that catches requests.exceptions.ConnectionError and requests.exceptions.Timeout, re-raising as DAVNetworkError. - In DAVClient.request(), catch DAVNetworkError and retry once for idempotent methods (GET, HEAD, OPTIONS, PROPFIND, REPORT, PUT, DELETE, MKCOL, MKCALENDAR). POST is the only non-idempotent method and is not retried. - Apply the same pattern to AsyncDAVClient._async_request() using an outer try/except around the existing auth-workaround try/except, with a _connection_retried flag to prevent infinite loops. The retry transparently recovers from stale HTTP/2 connection reuse, which is the root cause of the "sometimes works, sometimes times out" symptom. Ref: https://github.com/python-caldav/caldav/issues/647 Co-Authored-By: Claude Sonnet 4.6 --- caldav/async_davclient.py | 189 +++++++++++++++++++++----------------- caldav/davclient.py | 75 ++++++++++----- caldav/lib/error.py | 13 +++ tests/test_caldav_unit.py | 96 +++++++++++++++++++ 4 files changed, 264 insertions(+), 109 deletions(-) diff --git a/caldav/async_davclient.py b/caldav/async_davclient.py index d7630e14..3f63d56b 100644 --- a/caldav/async_davclient.py +++ b/caldav/async_davclient.py @@ -69,6 +69,7 @@ def auth_flow(self, request): from caldav.base_client import BaseDAVClient from caldav.base_client import get_davclient as _base_get_davclient from caldav.compatibility_hints import FeatureSet +from caldav.davclient import _SAFE_METHODS from caldav.lib import error from caldav.lib.python_utilities import to_wire from caldav.lib.url import URL @@ -92,6 +93,12 @@ def auth_flow(self, request): log = logging.getLogger("caldav") +# Network-level exceptions for the active async HTTP library. +if _USE_HTTPX: + _NETWORK_EXCEPTIONS: tuple = (httpx.ConnectError, httpx.TimeoutException, httpx.NetworkError) +else: + _NETWORK_EXCEPTIONS = (niquests.exceptions.ConnectionError, niquests.exceptions.Timeout) + if sys.version_info < (3, 11): from typing_extensions import Self else: @@ -404,6 +411,7 @@ async def _async_request( method: str = "GET", body: str = "", headers: Mapping[str, str] | None = None, + _connection_retried: bool = False, ) -> AsyncDAVResponse: """ Async HTTP request implementation with auth negotiation. @@ -441,96 +449,109 @@ async def _async_request( } try: - r = await self.session.request(**request_kwargs) - reason = r.reason_phrase if _USE_HTTPX else r.reason - log.debug(f"server responded with {r.status_code} {reason}") - if ( + try: + r = await self.session.request(**request_kwargs) + reason = r.reason_phrase if _USE_HTTPX else r.reason + log.debug(f"server responded with {r.status_code} {reason}") + if ( + r.status_code == 401 + and "text/html" in self.headers.get("Content-Type", "") + and not self.auth + ): + msg = ( + "No authentication object was provided. " + "HTML was returned when probing the server for supported authentication types. " + "To avoid logging errors, consider passing the auth_type connection parameter" + ) + if r.headers.get("WWW-Authenticate"): + auth_types = [ + t + for t in self.extract_auth_types(r.headers["WWW-Authenticate"]) + if t in ["basic", "digest", "bearer"] + ] + if auth_types: + msg += "\nSupported authentication types: {}".format( + ", ".join(auth_types) + ) + log.warning(msg) + response = AsyncDAVResponse(r, self) + except Exception: + # Workaround for servers that abort connection on unauthenticated requests + # ref https://github.com/python-caldav/caldav/issues/158 + if self.auth or not self.password: + raise + # Build minimal request for auth detection + if _USE_HTTPX: + r = await self.session.request( + method="GET", + url=str(url_obj), + headers=combined_headers, + timeout=self.timeout, + ) + else: + proxies = None + if self.proxy is not None: + proxies = {url_obj.scheme: self.proxy} + r = await self.session.request( + method="GET", + url=str(url_obj), + headers=combined_headers, + timeout=self.timeout, + proxies=proxies, + verify=self.ssl_verify_cert, + cert=self.ssl_cert, + ) + reason = r.reason_phrase if _USE_HTTPX else r.reason + log.debug(f"auth type detection: server responded with {r.status_code} {reason}") + if r.status_code == 401 and r.headers.get("WWW-Authenticate"): + auth_types = self.extract_auth_types(r.headers["WWW-Authenticate"]) + self.build_auth_object(auth_types) + # Retry original request with auth + request_kwargs["auth"] = self.auth + r = await self.session.request(**request_kwargs) + response = AsyncDAVResponse(r, self) + + # Handle 429/503 rate-limit responses + error.raise_if_rate_limited(r.status_code, str(url_obj), r.headers.get("Retry-After")) + + # Handle 401: negotiate auth then retry + if self._should_negotiate_auth(r.status_code, r.headers): + self._build_auth_from_401(r.headers["WWW-Authenticate"]) + return await self._async_request(url, method, body, headers) + + elif ( r.status_code == 401 - and "text/html" in self.headers.get("Content-Type", "") - and not self.auth + and "WWW-Authenticate" in r.headers + and self.auth + and self.features.is_supported("http.multiplexing", return_defaults=False) is None ): - msg = ( - "No authentication object was provided. " - "HTML was returned when probing the server for supported authentication types. " - "To avoid logging errors, consider passing the auth_type connection parameter" - ) - if r.headers.get("WWW-Authenticate"): - auth_types = [ - t - for t in self.extract_auth_types(r.headers["WWW-Authenticate"]) - if t in ["basic", "digest", "bearer"] - ] - if auth_types: - msg += "\nSupported authentication types: {}".format(", ".join(auth_types)) - log.warning(msg) - response = AsyncDAVResponse(r, self) - except Exception: - # Workaround for servers that abort connection on unauthenticated requests - # ref https://github.com/python-caldav/caldav/issues/158 - if self.auth or not self.password: - raise - # Build minimal request for auth detection - if _USE_HTTPX: - r = await self.session.request( - method="GET", - url=str(url_obj), - headers=combined_headers, - timeout=self.timeout, - ) - else: - proxies = None - if self.proxy is not None: - proxies = {url_obj.scheme: self.proxy} - r = await self.session.request( - method="GET", - url=str(url_obj), - headers=combined_headers, - timeout=self.timeout, - proxies=proxies, - verify=self.ssl_verify_cert, - cert=self.ssl_cert, - ) - reason = r.reason_phrase if _USE_HTTPX else r.reason - log.debug(f"auth type detection: server responded with {r.status_code} {reason}") - if r.status_code == 401 and r.headers.get("WWW-Authenticate"): - auth_types = self.extract_auth_types(r.headers["WWW-Authenticate"]) - self.build_auth_object(auth_types) - # Retry original request with auth - request_kwargs["auth"] = self.auth - r = await self.session.request(**request_kwargs) - response = AsyncDAVResponse(r, self) + # Handle HTTP/2 multiplexing issue: most likely wrong username/password, but could + # be an HTTP/2 problem. Retry with HTTP/2 disabled if multiplexing was auto-detected. + await self.close() + self._http2 = False + self._create_session() + # Set multiplexing to False BEFORE retry to prevent infinite loop + self.features.set_feature("http.multiplexing", False) + return await self._async_request(str(url_obj), method, body, headers) - # Handle 429/503 rate-limit responses - error.raise_if_rate_limited(r.status_code, str(url_obj), r.headers.get("Retry-After")) + # Raise AuthorizationError for 401/403 responses + if response.status in (401, 403): + self._raise_authorization_error(str(url_obj), response) - # Handle 401: negotiate auth then retry - if self._should_negotiate_auth(r.status_code, r.headers): - self._build_auth_from_401(r.headers["WWW-Authenticate"]) - return await self._async_request(url, method, body, headers) - - elif ( - r.status_code == 401 - and "WWW-Authenticate" in r.headers - and self.auth - and self.features.is_supported("http.multiplexing", return_defaults=False) is None - ): - # Handle HTTP/2 multiplexing issue: most likely wrong username/password, but could - # be an HTTP/2 problem. Retry with HTTP/2 disabled if multiplexing was auto-detected. - await self.close() - self._http2 = False - self._create_session() - # Set multiplexing to False BEFORE retry to prevent infinite loop - self.features.set_feature("http.multiplexing", False) - return await self._async_request(str(url_obj), method, body, headers) + if error.debug_dump_communication: + error._dump_communication(method, url, combined_headers, body, response) - # Raise AuthorizationError for 401/403 responses - if response.status in (401, 403): - self._raise_authorization_error(str(url_obj), response) + return response - if error.debug_dump_communication: - error._dump_communication(method, url, combined_headers, body, response) - - return response + except error.DAVError: + raise + except _NETWORK_EXCEPTIONS as e: + if not _connection_retried and method.upper() in _SAFE_METHODS: + log.warning("Network error on %s %s, retrying once", method, url) + return await self._async_request( + url, method, body, headers, _connection_retried=True + ) + raise error.DAVNetworkError(str(url_obj), str(e)) from e # ==================== HTTP Method Wrappers ==================== # Query methods (URL optional - defaults to self.url) diff --git a/caldav/davclient.py b/caldav/davclient.py index 25b6dc99..328291b5 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -89,6 +89,15 @@ from caldav.config import resolve_features as _resolve_features +# CalDAV/WebDAV methods that are safe to retry on transient network errors. +# POST is the only non-idempotent method and is excluded. All others are +# idempotent: re-sending yields the same server state. Note that retrying +# DELETE after a silent success will return 404; this is arguably correct +# (the resource is gone as intended), though callers may observe the error. +_SAFE_METHODS: frozenset[str] = frozenset( + {"GET", "HEAD", "OPTIONS", "PROPFIND", "REPORT", "PUT", "DELETE", "MKCOL", "MKCALENDAR"} +) + def _auto_url( url, @@ -857,6 +866,7 @@ def request( body: str = "", headers: Mapping[str, str] = None, rate_limit_time_slept=0, + _connection_retried: bool = False, ) -> DAVResponse: """ Send a generic HTTP request. @@ -891,6 +901,13 @@ def request( raise time.sleep(sleep_seconds) return self.request(url, method, body, headers, rate_limit_time_slept + sleep_seconds) + except error.DAVNetworkError: + if not _connection_retried and method.upper() in _SAFE_METHODS: + log.warning("Network error on %s %s, retrying once", method, url) + return self.request( + url, method, body, headers, rate_limit_time_slept, _connection_retried=True + ) + raise def _sync_request( self, @@ -909,38 +926,46 @@ def _sync_request( proxies = {url_obj.scheme: self.proxy} log.debug("using proxy - %s" % (proxies)) - r = self.session.request( - method, - str(url_obj), - data=to_wire(body), - headers=combined_headers, - proxies=proxies, - auth=self.auth, - timeout=self.timeout, - verify=self.ssl_verify_cert, - cert=self.ssl_cert, - ) + try: + r = self.session.request( + method, + str(url_obj), + data=to_wire(body), + headers=combined_headers, + proxies=proxies, + auth=self.auth, + timeout=self.timeout, + verify=self.ssl_verify_cert, + cert=self.ssl_cert, + ) - r_headers = CaseInsensitiveDict(r.headers) + r_headers = CaseInsensitiveDict(r.headers) - # Handle 429/503 responses: raise RateLimitError so the caller can decide whether to retry - error.raise_if_rate_limited(r.status_code, str(url_obj), r_headers.get("Retry-After")) + # Handle 429/503 responses: raise RateLimitError so the caller can decide whether to retry + error.raise_if_rate_limited(r.status_code, str(url_obj), r_headers.get("Retry-After")) - # Handle 401: negotiate auth then retry - if self._should_negotiate_auth(r.status_code, r_headers): - self._build_auth_from_401(r_headers["WWW-Authenticate"]) - return self._sync_request(url, method, body, headers) + # Handle 401: negotiate auth then retry + if self._should_negotiate_auth(r.status_code, r_headers): + self._build_auth_from_401(r_headers["WWW-Authenticate"]) + return self._sync_request(url, method, body, headers) - # Raise AuthorizationError for 401/403 after auth attempt - if r.status_code in (401, 403): - self._raise_authorization_error(str(url_obj), r) + # Raise AuthorizationError for 401/403 after auth attempt + if r.status_code in (401, 403): + self._raise_authorization_error(str(url_obj), r) - response = DAVResponse(r, self) + response = DAVResponse(r, self) - if error.debug_dump_communication: - error._dump_communication(method, url, combined_headers, body, response) + if error.debug_dump_communication: + error._dump_communication(method, url, combined_headers, body, response) - return response + return response + + except error.DAVError: + raise + except requests.exceptions.ConnectionError as e: + raise error.DAVNetworkError(str(url_obj), str(e)) from e + except requests.exceptions.Timeout as e: + raise error.DAVNetworkError(str(url_obj), str(e)) from e def get_calendars(**kwargs) -> list["Calendar"]: diff --git a/caldav/lib/error.py b/caldav/lib/error.py index 1d4754b7..8a1cd120 100644 --- a/caldav/lib/error.py +++ b/caldav/lib/error.py @@ -186,6 +186,19 @@ def __init__( self.retry_after_seconds = retry_after_seconds +class DAVNetworkError(DAVError): + """Raised on a network-level failure (timeout, connection reset, etc.) + while communicating with the server. + + This typically wraps a ``requests.exceptions.ConnectionError`` or + ``requests.exceptions.Timeout`` from the underlying HTTP library. The + original exception is chained via ``__cause__`` so the full traceback is + still available when debugging. + """ + + pass + + def parse_retry_after(retry_after_header: str) -> float | None: """Parse a Retry-After header value into seconds from now. diff --git a/tests/test_caldav_unit.py b/tests/test_caldav_unit.py index 5777a73a..f440d724 100755 --- a/tests/test_caldav_unit.py +++ b/tests/test_caldav_unit.py @@ -2629,3 +2629,99 @@ def test_meta_section_returns_multiple_dicts(self, tmp_path): "https://work.example.com/dav/", "https://personal.example.com/dav/", } + + +class TestConnectionErrorHandling: + """ + Unit tests for connection error / timeout handling (issue #647). + + niquests can raise ConnectionError or Timeout when accessing response + attributes (lazy gather). caldav should catch these, wrap them as + DAVNetworkError, and retry once for all idempotent methods (everything + except POST). + """ + + def _make_ok_response(self): + r = mock.MagicMock() + r.status_code = 207 + r.headers = {} + r.reason = "Multi-Status" + return r + + @mock.patch("caldav.davclient.requests.Session.request") + def test_connection_error_report_retries(self, mocked): + """On ConnectionError, REPORT is retried once and succeeds on second attempt.""" + from caldav.davclient import requests as _requests + + mocked.side_effect = [ + _requests.exceptions.ConnectionError("Read timed out."), + self._make_ok_response(), + ] + client = DAVClient(url="http://cal.example.com/") + response = client.request("/", "REPORT") + assert response.status == 207 + assert mocked.call_count == 2 + + @mock.patch("caldav.davclient.requests.Session.request") + def test_timeout_propfind_retries(self, mocked): + """On Timeout, PROPFIND is retried once and succeeds on second attempt.""" + from caldav.davclient import requests as _requests + + mocked.side_effect = [ + _requests.exceptions.Timeout("Read timed out."), + self._make_ok_response(), + ] + client = DAVClient(url="http://cal.example.com/") + response = client.request("/", "PROPFIND") + assert response.status == 207 + assert mocked.call_count == 2 + + @mock.patch("caldav.davclient.requests.Session.request") + def test_connection_error_second_fail_raises(self, mocked): + """If both attempts fail, DAVNetworkError is raised.""" + from caldav.davclient import requests as _requests + + mocked.side_effect = _requests.exceptions.ConnectionError("Read timed out.") + client = DAVClient(url="http://cal.example.com/") + with pytest.raises(error.DAVNetworkError): + client.request("/", "REPORT") + assert mocked.call_count == 2 + + @mock.patch("caldav.davclient.requests.Session.request") + def test_connection_error_put_retries(self, mocked): + """PUT is idempotent and should be retried.""" + from caldav.davclient import requests as _requests + + mocked.side_effect = [ + _requests.exceptions.ConnectionError("Read timed out."), + self._make_ok_response(), + ] + client = DAVClient(url="http://cal.example.com/") + response = client.request("/", "PUT") + assert response.status == 207 + assert mocked.call_count == 2 + + @mock.patch("caldav.davclient.requests.Session.request") + def test_connection_error_post_no_retry(self, mocked): + """POST is not idempotent — DAVNetworkError raised immediately without retry.""" + from caldav.davclient import requests as _requests + + mocked.side_effect = _requests.exceptions.ConnectionError("Read timed out.") + client = DAVClient(url="http://cal.example.com/") + with pytest.raises(error.DAVNetworkError): + client.request("/", "POST") + assert mocked.call_count == 1 + + @mock.patch("caldav.davclient.requests.Session.request") + def test_get_method_retries(self, mocked): + """GET is in the safe set and should also be retried.""" + from caldav.davclient import requests as _requests + + mocked.side_effect = [ + _requests.exceptions.ConnectionError("Read timed out."), + self._make_ok_response(), + ] + client = DAVClient(url="http://cal.example.com/") + response = client.request("/", "GET") + assert response.status == 207 + assert mocked.call_count == 2