Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .sampo/changesets/flags-no-retry-connection-refused.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
pypi/posthog: patch
---

Stop retrying `/flags` requests when the connection is refused, so the flag error surfaces right away instead of after the retry backoff.
32 changes: 30 additions & 2 deletions posthog/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import requests
from requests.adapters import HTTPAdapter
from urllib3.connection import HTTPConnection
from urllib3.exceptions import NewConnectionError
from urllib3.util.retry import Retry

from posthog._logging import _configure_posthog_logging
Expand Down Expand Up @@ -322,6 +323,29 @@ def _process_response(
raise APIError(res.status_code, res.text, retry_after=retry_after)


def _is_connection_refused(exc: BaseException) -> bool:
"""Return True when a requests transport error wraps a refused connection."""
pending: List[Any] = [exc]
seen = set()
while pending:
current = pending.pop()
if current is None or id(current) in seen:
continue
seen.add(id(current))
if isinstance(current, ConnectionRefusedError):
return True
if isinstance(current, BaseException):
pending.extend(current.args)
pending.append(current.__cause__)
# __context__ records whatever exception was being handled, which may
# have nothing to do with this request. urllib3 1.x only links the
# socket error to NewConnectionError that way, so follow it just there.
if isinstance(current, NewConnectionError):
pending.append(current.__context__)
pending.append(getattr(current, "reason", None))
return False


def _feature_flags_retry_delay(failed_attempt: int) -> float:
return _FEATURE_FLAGS_RETRY_BACKOFF_SECONDS * (2**failed_attempt)

Expand Down Expand Up @@ -354,8 +378,12 @@ def flags(
return _process_response(
res, success_message="Feature flags evaluated successfully"
)
except (requests.exceptions.ConnectionError, requests.exceptions.Timeout):
if failed_attempt >= retries:
except (
requests.exceptions.ConnectionError,
requests.exceptions.Timeout,
) as exc:
# A refused connection is not transient, so it is not retried.
if failed_attempt >= retries or _is_connection_refused(exc):
raise
except APIError as exc:
if (
Expand Down
55 changes: 55 additions & 0 deletions posthog/test/test_request.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import gzip
import json
import socket
import unittest
import zlib
from datetime import date, datetime, timedelta
Expand Down Expand Up @@ -702,6 +703,60 @@ def test_flags_retries_transport_errors_once_by_default(
self.assertEqual(mock_session.post.call_count, 2)
mock_sleep.assert_called_once_with(0.3)

@mock.patch("posthog.request.time.sleep")
def test_flags_does_not_retry_refused_connection(self, mock_sleep):
# Bind and close a socket so the port refuses connections.
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
sock.bind(("127.0.0.1", 0))
port = sock.getsockname()[1]

with mock.patch("posthog.request.post", wraps=request_module.post) as mock_post:
with self.assertRaises(requests.exceptions.ConnectionError):
flags(
"test-key",
f"http://127.0.0.1:{port}",
max_retries=2,
distinct_id="user123",
)

self.assertEqual(mock_post.call_count, 1)
mock_sleep.assert_not_called()

@mock.patch("posthog.request.time.sleep")
@mock.patch("posthog.request._get_flags_session")
def test_flags_retries_when_handling_an_unrelated_refused_connection(
self, mock_get_flags_session, mock_sleep
):
mock_response = requests.Response()
mock_response.status_code = 200
mock_response._content = json.dumps(
{
"featureFlags": {"test-flag": True},
"featureFlagPayloads": {},
"errorsWhileComputingFlags": False,
}
).encode("utf-8")

mock_session = mock.MagicMock()
mock_session.post.side_effect = [
requests.exceptions.Timeout("timed out"),
mock_response,
]
mock_get_flags_session.return_value = mock_session

# The application evaluates flags while handling its own refused
# connection, so the timeout inherits that error as its __context__.
try:
raise ConnectionRefusedError("database is down")
except ConnectionRefusedError:
response = flags(
"test-key", "https://test.posthog.com", distinct_id="user123"
)

self.assertEqual(response["featureFlags"], {"test-flag": True})
self.assertEqual(mock_session.post.call_count, 2)
mock_sleep.assert_called_once_with(0.3)

@mock.patch("posthog.request.time.sleep")
@mock.patch("posthog.request._get_flags_session")
def test_flags_retry_count_zero_disables_retries(
Expand Down
Loading