Skip to content

Commit 8e11162

Browse files
committed
fix: Validate data source intervals in the retry state and bound the wait
1 parent 1b50827 commit 8e11162

13 files changed

Lines changed: 269 additions & 104 deletions

‎ldclient/async_config.py‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
from ldclient.impl.util import (
3131
log,
3232
validate_application_info,
33-
validate_positive_finite,
3433
validate_sdk_key_format
3534
)
3635
from ldclient.interfaces import (
@@ -258,13 +257,8 @@ def __init__(
258257
self.__stream_uri = stream_uri.rstrip('/')
259258
self.__update_processor_class = update_processor_class
260259
self.__stream = stream
261-
self.__initial_reconnect_delay = validate_positive_finite(
262-
initial_reconnect_delay, DEFAULT_INITIAL_RECONNECT_DELAY, 'initial_reconnect_delay', log
263-
)
264-
self.__poll_interval = max(
265-
validate_positive_finite(poll_interval, DEFAULT_POLL_INTERVAL, 'poll_interval', log),
266-
DEFAULT_POLL_INTERVAL,
267-
)
260+
self.__initial_reconnect_delay = initial_reconnect_delay
261+
self.__poll_interval = max(poll_interval, DEFAULT_POLL_INTERVAL)
268262
self.__use_ldd = use_ldd
269263
self.__feature_store = AsyncInMemoryFeatureStore() if not feature_store else feature_store
270264
self.__event_processor_class = event_processor_class

‎ldclient/config.py‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
from ldclient.impl.util import (
1616
log,
1717
validate_application_info,
18-
validate_positive_finite,
1918
validate_sdk_key_format
2019
)
2120
from ldclient.interfaces import (
@@ -407,13 +406,8 @@ def __init__(
407406
self.__stream_uri = stream_uri.rstrip('/')
408407
self.__update_processor_class = update_processor_class
409408
self.__stream = stream
410-
self.__initial_reconnect_delay = validate_positive_finite(
411-
initial_reconnect_delay, DEFAULT_INITIAL_RECONNECT_DELAY, 'initial_reconnect_delay', log
412-
)
413-
self.__poll_interval = max(
414-
validate_positive_finite(poll_interval, DEFAULT_POLL_INTERVAL, 'poll_interval', log),
415-
DEFAULT_POLL_INTERVAL,
416-
)
409+
self.__initial_reconnect_delay = initial_reconnect_delay
410+
self.__poll_interval = max(poll_interval, DEFAULT_POLL_INTERVAL)
417411
self.__use_ldd = use_ldd
418412
self.__feature_store = InMemoryFeatureStore() if not feature_store else feature_store
419413
self.__event_processor_class = event_processor_class

‎ldclient/impl/datasource/streaming.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import json
22
import time
3+
from threading import TIMEOUT_MAX
34
from threading import Event as ThreadEvent
45
from threading import Thread
56
from typing import Callable, Optional
@@ -253,7 +254,7 @@ def _handle_error(self, error: Exception) -> bool:
253254
self._data_source_update_sink.update_status(DataSourceState.INTERRUPTED, error_info)
254255

255256
self._connection_attempt_start_time = time.time() + delay
256-
return not self._stop_event.wait(delay)
257+
return not self._stop_event.wait(min(delay, TIMEOUT_MAX))
257258

258259
# magic methods for "with" statement (used in testing)
259260
def __enter__(self):

‎ldclient/impl/repeating_task.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
from threading import Event, Thread
1+
from threading import TIMEOUT_MAX, Event, Thread
22
from typing import Any, Callable
33

44
from ldclient.impl.delay import DelaySource, FixedDelay
@@ -66,7 +66,7 @@ def stop(self):
6666

6767
def _run(self):
6868
if self.__initial_delay > 0:
69-
if self.__stop.wait(self.__initial_delay):
69+
if self.__stop.wait(min(self.__initial_delay, TIMEOUT_MAX)):
7070
return
7171
stopped = self.__stop.is_set()
7272
while not stopped:
@@ -77,4 +77,4 @@ def _run(self):
7777
# The wait starts when the callback returns, so a slow callback
7878
# never shortens it.
7979
delay = self.__delays.next_delay
80-
stopped = self.__stop.wait(delay) if delay > 0 else self.__stop.is_set()
80+
stopped = self.__stop.wait(min(delay, TIMEOUT_MAX)) if delay > 0 else self.__stop.is_set()

‎ldclient/impl/retry.py‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
# currently excluded from documentation - see docs/README.md
2020

21+
import math
2122
import random
2223
import time
2324
from enum import Enum
@@ -27,7 +28,7 @@
2728
DEFAULT_INITIAL_RECONNECT_DELAY,
2829
DEFAULT_POLL_INTERVAL
2930
)
30-
from ldclient.impl.util import log, validate_positive_finite
31+
from ldclient.impl.util import log
3132

3233
# The delay ceiling of the normal regime for streaming, in seconds.
3334
NORMAL_STREAMING_CEILING_DELAY = 30
@@ -53,6 +54,23 @@
5354
_MAX_BACKOFF_EXPONENT = 30
5455

5556

57+
def _usable_delay(value: float, default: float, name: str, ceiling: float = math.inf) -> float:
58+
"""
59+
Returns the delay to use, clamped to the ceiling. A value that is
60+
not a positive, finite number of seconds is replaced by the default.
61+
62+
:param value: the configured number of seconds
63+
:param default: the value to use when ``value`` is not usable
64+
:param name: the option name, for the warning message
65+
:param ceiling: the longest delay allowed
66+
"""
67+
68+
if value > 0 and math.isfinite(value):
69+
return min(value, ceiling)
70+
log.warning("%s must be a positive, finite number of seconds; using the default of %ss" % (name, default))
71+
return default
72+
73+
5674
class FailureKind(Enum):
5775
"""How a failure is classified, which decides how long the next wait is."""
5876

@@ -257,13 +275,14 @@ def for_streaming(initial_reconnect_delay: float) -> RetryState:
257275
healthy operation after establishing a successful connection with no
258276
failures during the ``STREAMING_RESET_INTERVAL``.
259277
260-
``Config`` validates the configured delay, so this guard only catches a
261-
state built without it.
262-
263-
The extended regime never starts below the configured delay.
278+
The configured delay is validated here, because ``Config`` reports it as
279+
given. It is not clamped to either ceiling: a delay that applies after an
280+
unexpected failure may never be shorter than the configured delay, so a
281+
configured delay longer than a ceiling raises the bound rather than being
282+
cut down to it.
264283
"""
265-
initial_reconnect_delay = validate_positive_finite(
266-
initial_reconnect_delay, DEFAULT_INITIAL_RECONNECT_DELAY, 'initial_reconnect_delay', log
284+
initial_reconnect_delay = _usable_delay(
285+
initial_reconnect_delay, DEFAULT_INITIAL_RECONNECT_DELAY, 'initial_reconnect_delay'
267286
)
268287
return RetryState(
269288
normal_initial_delay=initial_reconnect_delay,
@@ -285,10 +304,14 @@ def for_polling(poll_interval: float) -> RetryState:
285304
schedule. Polling is healthy on any successful poll, and resets after two
286305
in a row.
287306
288-
``Config`` validates and clamps the poll interval, so this guard only
289-
catches a state built without it.
307+
The poll interval is validated here, because ``Config`` reports it as
308+
given. It is not clamped to a ceiling, unlike the streaming delay. The
309+
normal ceiling is the poll interval itself, so a clamp would do nothing,
310+
and the extended regime has to keep the cadence even where that is longer
311+
than an hour: a wait may never be shorter than the interval the caller
312+
configured, so the cadence wins over the extended ceiling.
290313
"""
291-
poll_interval = validate_positive_finite(poll_interval, DEFAULT_POLL_INTERVAL, 'poll_interval', log)
314+
poll_interval = _usable_delay(poll_interval, DEFAULT_POLL_INTERVAL, 'poll_interval')
292315
return RetryState(
293316
normal_initial_delay=poll_interval,
294317
normal_ceiling_delay=poll_interval,

‎ldclient/impl/util.py‎

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import logging
2-
import math
32
import re
43
import sys
54
import time
@@ -61,25 +60,6 @@ def validate_application_value(value: Any, name: str, logger: logging.Logger) ->
6160
return value
6261

6362

64-
def validate_positive_finite(value: float, default: float, name: str, logger: logging.Logger) -> float:
65-
"""
66-
Validates that a number of seconds is positive and finite.
67-
68-
A non-finite value makes later arithmetic produce NaN, and a non-positive
69-
one makes a wait no wait at all.
70-
71-
:param value: the number of seconds to validate
72-
:param default: the value to use when ``value`` is not usable
73-
:param name: the option name, for the warning message
74-
:param logger: the logger to use for logging warnings
75-
:return: ``value``, or ``default`` if ``value`` is not positive and finite
76-
"""
77-
if value > 0 and math.isfinite(value):
78-
return value
79-
logger.warning("%s must be a positive, finite number of seconds; using the default of %ss" % (name, default))
80-
return default
81-
82-
8363
def validate_sdk_key_format(sdk_key: str, logger: logging.Logger) -> str:
8464
"""
8565
Validates that an SDK key does not contain invalid characters and is not too long for our systems.
@@ -155,14 +135,11 @@ def throw_if_unsuccessful_response(resp):
155135

156136
def is_http_error_recoverable(status):
157137
"""
158-
Reports whether a component that treats some statuses as fatal should
159-
keep going.
160-
161138
Deprecated. Use :func:`ldclient.impl.retry.classify_http_status` instead.
162139
"""
163140
if status >= 400 and status < 500:
164-
return status in _RETRYABLE_STATUSES # all other 4xx besides these are treated as fatal
165-
return True
141+
return status in _RETRYABLE_STATUSES # all other 4xx besides these are unrecoverable
142+
return True # all other errors are recoverable
166143

167144

168145
def http_error_description(status):
@@ -171,11 +148,8 @@ def http_error_description(status):
171148

172149
def http_error_message(status, context, retryable_message="will retry"):
173150
"""
174-
Builds the log message for an HTTP failure in a component that stops on
175-
some statuses.
176-
177-
Deprecated. The FDv1 data sources build their own message instead, so
178-
that it can report the real retry delay.
151+
Deprecated. The FDv1 data sources build their own message, so that it can
152+
report the real retry delay.
179153
"""
180154
return "Received %s for %s - %s" % (http_error_description(status), context, retryable_message if is_http_error_recoverable(status) else "giving up permanently")
181155

‎ldclient/interfaces.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1013,7 +1013,8 @@ class DataSourceState(Enum):
10131013
"""
10141014
Indicates that the data source has been permanently shut down.
10151015
1016-
This means the SDK client was explicitly shut down, or that its configuration could not be parsed.
1016+
This could be because the SDK client was explicitly shut down, because its configuration could not
1017+
be parsed, or because the data source encountered a condition it will not retry.
10171018
"""
10181019

10191020

‎ldclient/testing/impl/datasource/test_async_polling.py‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import asyncio
66
import logging
77
import ssl
8+
import time
89
from unittest.mock import AsyncMock, MagicMock, patch
910

1011
import aiohttp
@@ -58,9 +59,12 @@ def make_config(**kwargs):
5859
)
5960

6061

62+
ONE_HOUR = 60 * 60
63+
64+
6165
def fast_retry_state(delay=0.001):
62-
"""A retry state with tiny delays, so a test does not have to wait out the
63-
real extended-regime delay of five minutes."""
66+
"""A retry state whose every delay is ``delay``: small enough to skip the
67+
real extended-regime wait, or large enough to prove a stop interrupts one."""
6468
return RetryState(
6569
normal_initial_delay=delay,
6670
normal_ceiling_delay=delay,
@@ -469,6 +473,30 @@ async def close():
469473

470474
assert order == ['poll_done', 'transport_closed']
471475

476+
@pytest.mark.asyncio
477+
async def test_an_extended_regime_wait_is_cut_short_by_stop(self):
478+
"""Shutdown must not sit through an hour-long backoff. The in-flight-poll
479+
case is test_stop_cancels_polling_task_cleanly; this one stops while the
480+
task is waiting between polls."""
481+
retry = fast_retry_state(ONE_HOUR)
482+
processor = make_processor(retry_state=retry)
483+
processor._requester.get_all_data = AsyncMock(
484+
side_effect=UnsuccessfulResponseException(401)
485+
)
486+
487+
processor.start()
488+
# Confirm the wait under test really is long before measuring the stop.
489+
deadline = time.time() + 2
490+
while retry.next_delay <= 60 and time.time() < deadline:
491+
await asyncio.sleep(0.01)
492+
assert retry.next_delay > 60, "the wait under test should be minutes long"
493+
494+
started = time.time()
495+
await processor.stop()
496+
elapsed = time.time() - started
497+
498+
assert elapsed < 2, "stop() took %.2fs" % elapsed
499+
472500
@pytest.mark.asyncio
473501
async def test_stop_cancels_polling_task_cleanly(self):
474502
store = MockAsyncFeatureStore()

‎ldclient/testing/impl/datasource/test_async_streaming.py‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import json
1010
import logging
1111
import ssl
12+
import time
1213
from unittest import mock
1314

1415
import aiohttp
@@ -105,9 +106,12 @@ def _retry_state_with(policy: AfterHealthyFor) -> RetryState:
105106
)
106107

107108

109+
ONE_HOUR = 60 * 60
110+
111+
108112
def _fast_retry_state(delay: float = 0.001) -> RetryState:
109-
"""A retry state with tiny delays, so a test does not have to wait out the
110-
real extended-regime delay of five minutes."""
113+
"""A retry state whose every delay is ``delay``: small enough to skip the
114+
real extended-regime wait, or large enough to prove a stop interrupts one."""
111115
return RetryState(
112116
normal_initial_delay=delay,
113117
normal_ceiling_delay=delay,
@@ -595,6 +599,37 @@ async def test_a_fresh_stream_starts_a_new_reset_window():
595599
assert len(set(windows)) == 2, "the second stream reused the first window"
596600

597601

602+
@pytest.mark.asyncio
603+
async def test_an_extended_regime_wait_is_cut_short_by_stop():
604+
"""Shutdown must not sit through an hour-long backoff. The healthy-stop case
605+
is test_stop_closes_sse_and_finishes_task; this one stops mid-wait."""
606+
from ld_eventsource.errors import HTTPStatusError
607+
608+
retry = _fast_retry_state(ONE_HOUR)
609+
proc, store, ready, _ = _make_processor(
610+
[_start(), _fault(error=HTTPStatusError(401))], retry_state=retry
611+
)
612+
proc.start()
613+
# Confirm the wait under test really is long before measuring the stop.
614+
await _wait_until(lambda: retry.next_delay > 60)
615+
616+
started = time.time()
617+
await proc.stop()
618+
elapsed = time.time() - started
619+
620+
assert elapsed < 2, "stop() took %.2fs" % elapsed
621+
leaked = [t for t in proc._runner._tasks if not t.done()]
622+
assert leaked == [], "stop() returned with the task still running: %r" % leaked
623+
624+
625+
@pytest.mark.asyncio
626+
async def test_stop_before_start_and_stop_twice_are_safe():
627+
proc, store, ready, _ = _make_processor([])
628+
629+
await proc.stop()
630+
await proc.stop()
631+
632+
598633
@pytest.mark.asyncio
599634
async def test_stop_closes_sse_and_finishes_task():
600635
flag = FlagBuilder('f1').version(1).build()

0 commit comments

Comments
 (0)