Skip to content

Commit 60bef61

Browse files
committed
fix: Detect changes from the first read and validate the poll interval of the file override source
Change detection is set up before the initial load, so an edit made while the files are first read is picked up. Close stops the reloader even when the change detector fails to close. The builder rejects a poll interval that is not a number or not finite.
1 parent c30407a commit 60bef61

3 files changed

Lines changed: 88 additions & 15 deletions

File tree

‎ldclient/impl/integrations/overrides/file_override_source.py‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,19 @@ def apply(merged: MergeResult) -> None:
7575
retry_delay=DEFAULT_RETRY_DELAY,
7676
skip_unchanged=True,
7777
)
78+
# Change detection is set up before the initial load, so an edit made while the files
79+
# are first read is picked up by the reload it triggers instead of waiting for a later
80+
# change.
7881
with self._lock:
7982
if self._closed:
8083
return
8184
self._reloader = reloader
85+
if self._change_detection == ChangeDetection.WATCHING:
86+
self._change_detector = Watcher(self._paths, reloader.trigger)
87+
else:
88+
poller = Poller(self._paths, self._poll_interval, reloader.trigger)
89+
poller.start()
90+
self._change_detector = poller
8291

8392
# The initial load runs synchronously, so overrides present in the files are in effect
8493
# by the time the client constructor returns. A file that does not exist yet
@@ -87,16 +96,6 @@ def apply(merged: MergeResult) -> None:
8796
# the file is readable.
8897
reloader.reload_now()
8998

90-
with self._lock:
91-
if self._closed:
92-
return
93-
if self._change_detection == ChangeDetection.WATCHING:
94-
self._change_detector = Watcher(self._paths, reloader.trigger)
95-
else:
96-
poller = Poller(self._paths, self._poll_interval, reloader.trigger)
97-
poller.start()
98-
self._change_detector = poller
99-
10099
def close(self) -> None:
101100
with self._lock:
102101
if self._closed:
@@ -106,10 +105,14 @@ def close(self) -> None:
106105
reloader = self._reloader
107106
self._change_detector = None
108107
self._reloader = None
109-
if change_detector is not None:
110-
change_detector.close()
111-
if reloader is not None:
112-
reloader.close()
108+
try:
109+
if change_detector is not None:
110+
change_detector.close()
111+
finally:
112+
# The reloader stops even when the change detector fails to close, so no reload
113+
# can run after close.
114+
if reloader is not None:
115+
reloader.close()
113116

114117

115118
def _log_overrides_in_effect(merged: MergeResult) -> None:

‎ldclient/integrations/overrides.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
counters, so LaunchDarkly can distinguish them. They produce no individual evaluation events.
2727
"""
2828

29+
import math
2930
from typing import List, Optional, Union
3031

3132
from ldclient.config import DataSourceBuilderConfig, OverrideSourceBuilder
@@ -126,7 +127,13 @@ def poll_interval(self, seconds: float) -> 'FileOverrideSourceBuilder':
126127
:attr:`MINIMUM_POLL_INTERVAL` is raised to the minimum.
127128
128129
:param seconds: the interval in seconds
130+
:raises TypeError: when the interval is not a number
131+
:raises ValueError: when the interval is not a finite number
129132
"""
133+
if isinstance(seconds, bool) or not isinstance(seconds, (int, float)):
134+
raise TypeError("the poll interval must be a number of seconds")
135+
if not math.isfinite(seconds):
136+
raise ValueError("the poll interval must be a finite number of seconds")
130137
self.__poll_interval = seconds
131138
return self
132139

‎ldclient/testing/integrations/test_file_override_source.py‎

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44
"""
55
import logging
66
import os
7+
import threading
78
import time
89
from queue import Empty, Queue
9-
from typing import Any, Dict, List, Optional, Tuple
10+
from typing import Any, Dict, List, Optional, Set, Tuple
1011

1112
import pytest
1213

@@ -164,6 +165,18 @@ def test_builder_clamps_poll_interval_to_the_minimum(caplog):
164165
assert source._poll_interval == 2.5
165166

166167

168+
@pytest.mark.parametrize('seconds', ['1', None, True], ids=['string', 'none', 'bool'])
169+
def test_builder_rejects_a_poll_interval_that_is_not_a_number(seconds):
170+
with pytest.raises(TypeError):
171+
FileOverrideSourceBuilder(['a']).poll_interval(seconds)
172+
173+
174+
@pytest.mark.parametrize('seconds', [float('nan'), float('inf'), float('-inf')], ids=['nan', 'inf', '-inf'])
175+
def test_builder_rejects_a_poll_interval_that_is_not_finite(seconds):
176+
with pytest.raises(ValueError):
177+
FileOverrideSourceBuilder(['a']).poll_interval(seconds)
178+
179+
167180
def test_builder_rejects_watching_without_watchdog(monkeypatch):
168181
import ldclient.integrations.overrides as module
169182
monkeypatch.setattr(module, 'have_watchdog', False)
@@ -369,6 +382,56 @@ def test_source_retries_a_failed_load_without_a_change_signal(tmp_path, sources,
369382
assert flag_values(sink.require_snapshot()) == {'flag1': False}
370383

371384

385+
@pytest.mark.parametrize("mode", [ChangeDetection.POLLING, pytest.param(ChangeDetection.WATCHING, marks=watchdog_required)])
386+
def test_source_detects_an_edit_made_during_the_initial_load(tmp_path, sources, monkeypatch, mode):
387+
path = os.path.join(str(tmp_path), 'overrides.json')
388+
write_file(path, '{"flagValues": {"flag1": "initial"}}')
389+
original_reload_now = filedata.Reloader.reload_now
390+
391+
def load_then_edit(reloader: filedata.Reloader) -> None:
392+
# The edit lands after the files were read and before start returns.
393+
original_reload_now(reloader)
394+
write_file(path, '{"flagValues": {"flag1": "edited-after-the-first-read"}}')
395+
396+
monkeypatch.setattr(filedata.Reloader, 'reload_now', load_then_edit)
397+
_, sink = build_source(sources, [path], lambda b: b.change_detection(mode).poll_interval(FileOverrideSourceBuilder.MINIMUM_POLL_INTERVAL))
398+
399+
# The initial snapshot holds what the first read saw. The edit is detected and applied
400+
# without any further change to the file.
401+
assert flag_values(sink.require_snapshot()) == {'flag1': 'initial'}
402+
assert flag_values(sink.require_snapshot()) == {'flag1': 'edited-after-the-first-read'}
403+
404+
405+
def reloader_threads() -> Set[threading.Thread]:
406+
return {t for t in threading.enumerate() if t.name == 'ldclient.filedata.reloader'}
407+
408+
409+
def test_source_close_stops_the_reloader_when_the_change_detector_fails_to_close(tmp_path, sources, monkeypatch):
410+
path = os.path.join(str(tmp_path), 'overrides.json')
411+
write_file(path, '{}')
412+
before = reloader_threads()
413+
source, sink = build_source(sources, [path])
414+
sink.require_snapshot()
415+
started = reloader_threads() - before
416+
assert len(started) == 1
417+
418+
detector: Any = source._change_detector
419+
original_close = detector.close
420+
421+
def failing_close() -> None:
422+
original_close()
423+
raise RuntimeError("detector close failure")
424+
425+
monkeypatch.setattr(detector, 'close', failing_close)
426+
427+
# The failure propagates, and the reloader is stopped all the same: its worker ends.
428+
with pytest.raises(RuntimeError):
429+
source.close()
430+
worker = started.pop()
431+
worker.join(TEST_TIMEOUT)
432+
assert not worker.is_alive()
433+
434+
372435
def test_source_does_not_supply_snapshots_after_close(tmp_path, sources):
373436
path = os.path.join(str(tmp_path), 'overrides.json')
374437
write_file(path, '{"flagValues": {"flag1": true}}')

0 commit comments

Comments
 (0)