Skip to content

Commit 0011d08

Browse files
committed
feat: Warn when the SDK is used in a forked process without postfork
Threads do not survive a fork. A child process that inherits an LDClient has no stream thread, no event dispatcher thread, and no timer tasks, so flag values freeze, no analytics events are sent, and close() can block forever. The SDK gave no signal. This adds a warning, logged once per process, on the first use of the client in a process whose pid differs from the pid recorded when the client was created or when postfork() was last called. Trigger points: variation, variation_detail, migration_variation (all three pass through the private hook wrapper, so one check covers them), all_flags_state, identify, track, track_migration_op, flush, and close. The warning is skipped when a fork breaks nothing: offline mode, or LDD mode with events disabled. Implementation choice: os.getpid() compared with the stored pid on each trigger. A benchmark with the sdk-stress-tester showed no regression above noise, so os.register_at_fork() was not needed. The "already warned" state is keyed by the pid in which the warning was logged, not stored as a boolean, so a grandchild process warns again. The state is a dict written with dict.setdefault, which is atomic in CPython, rather than a threading.Lock. A lock held at the moment of a fork is copied into the child in the locked state and the child would block forever on its first evaluation, which is the class of failure this change is meant to expose, not cause. AsyncLDClient is unchanged. It has no postfork(), the sync/async parity test already lists postfork as sync-only, and forking a process that owns a running asyncio event loop is not a supported pattern. Any fork-related support for the async client would need its own design. Mirrors launchdarkly/ruby-server-sdk#430.
1 parent 2e095c5 commit 0011d08

2 files changed

Lines changed: 317 additions & 2 deletions

File tree

‎ldclient/client.py‎

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@
22
This submodule contains the client class that provides most of the SDK functionality.
33
"""
44

5+
import os
56
import threading
67
import traceback
7-
from typing import Any, Callable, List, Optional, Tuple
8+
from typing import Any, Callable, Dict, List, Optional, Tuple
89
from uuid import uuid4
910

1011
from ldclient.config import Config
@@ -50,6 +51,11 @@
5051

5152
from .impl import AnyNum
5253

54+
_FORK_WARNING_MESSAGE = (
55+
"This process was forked after the LDClient was created. Background threads do not survive a fork, "
56+
"so flag updates and analytics events will not work in this process. Call LDClient.postfork() after forking."
57+
)
58+
5359

5460
class LDClient:
5561
"""The LaunchDarkly SDK client object.
@@ -78,6 +84,9 @@ def __init__(self, config: Config, start_wait: float = 5):
7884
self._event_factory_default = EventFactory(False)
7985
self._event_factory_with_reasons = EventFactory(True)
8086

87+
self._owner_pid = os.getpid()
88+
self._fork_warned_pids: Dict[int, object] = {}
89+
8190
self.__start_up(start_wait)
8291

8392
def postfork(self, start_wait: float = 5):
@@ -100,10 +109,50 @@ def postfork(self, start_wait: float = 5):
100109
that any listener or hook integrations be added postfork unless you are
101110
certain it can survive the forking process.
102111
112+
If the SDK is used in a forked child process before this method is
113+
called, it logs a warning once per process. Calling this method clears
114+
that condition for the current process.
115+
103116
:param start_wait: the number of seconds to wait for a successful connection to LaunchDarkly
104117
"""
118+
self._owner_pid = os.getpid()
119+
self._fork_warned_pids = {}
105120
self.__start_up(start_wait)
106121

122+
def _check_forked(self):
123+
"""
124+
Log a warning once per process if this client is used in a process that was forked after
125+
the client was created.
126+
127+
The "already warned" state is keyed by pid, not stored as a boolean. A boolean would be
128+
copied into every child on fork, so a grandchild process would never get its own warning.
129+
130+
The state is a dict written with ``dict.setdefault``, not a ``threading.Lock``. A lock held
131+
at the moment of a fork is inherited locked by the child and would hang its first
132+
evaluation. ``dict.setdefault`` is atomic in CPython, so many threads log exactly one warning.
133+
"""
134+
pid = os.getpid()
135+
if pid == self._owner_pid:
136+
return
137+
if not self._fork_breaks_client():
138+
return
139+
140+
token = object()
141+
if self._fork_warned_pids.setdefault(pid, token) is token:
142+
log.warning(_FORK_WARNING_MESSAGE)
143+
144+
def _fork_breaks_client(self) -> bool:
145+
"""
146+
Return true if this configuration needs background threads. Offline mode has none. LDD mode
147+
with events disabled reads flags from the persistent store and sends nothing, so a fork does
148+
not break it.
149+
"""
150+
if self._config.offline:
151+
return False
152+
if self._config.use_ldd and not self._config.send_events:
153+
return False
154+
return True
155+
107156
def __start_up(self, start_wait: float):
108157
environment_metadata = get_environment_metadata(self._config, "python-server-sdk")
109158
plugin_hooks = get_plugin_hooks(self._config.plugins, environment_metadata)
@@ -220,6 +269,7 @@ def close(self):
220269
221270
Do not attempt to use the client after calling this method.
222271
"""
272+
self._check_forked()
223273
log.info("Closing LaunchDarkly client..")
224274
self._event_processor.stop()
225275
self._data_system.stop()
@@ -247,6 +297,7 @@ def track_migration_op(self, tracker: OpTracker):
247297
Customers not using the builder should provide this method with the
248298
tracker returned from calling :func:`migration_variation`.
249299
"""
300+
self._check_forked()
250301
event = tracker.build()
251302

252303
if isinstance(event, str):
@@ -271,6 +322,7 @@ def track(self, event_name: str, context: Context, data: Optional[Any] = None, m
271322
:param metric_value: a numeric value used by the LaunchDarkly experimentation feature in
272323
numeric custom metrics; can be omitted if this event is used by only non-numeric metrics
273324
"""
325+
self._check_forked()
274326
if not context.valid:
275327
log.warning("Invalid context for track (%s)" % context.error)
276328
else:
@@ -289,7 +341,7 @@ def identify(self, context: Context):
289341
290342
:param context: the context to register
291343
"""
292-
344+
self._check_forked()
293345
if not context.valid:
294346
log.warning("Invalid context for identify (%s)" % context.error)
295347
else:
@@ -331,6 +383,7 @@ def flush(self):
331383
schedules the next event delivery to be as soon as possible; however, the delivery still
332384
happens asynchronously on a worker thread, so this method will return immediately.
333385
"""
386+
self._check_forked()
334387
if self._config.offline:
335388
return
336389
return self._event_processor.flush()
@@ -480,6 +533,8 @@ def all_flags_state(self, context: Context, **kwargs) -> FeatureFlagsState:
480533
log.warning("all_flags_state() called, but client is in offline mode. Returning empty state")
481534
return FeatureFlagsState(False)
482535

536+
self._check_forked()
537+
483538
availability = self._data_system.data_availability
484539
if availability != DataAvailability.REFRESHED:
485540
if availability == DataAvailability.CACHED:
@@ -572,6 +627,10 @@ def __evaluate_with_hooks(self, key: str, context: Context, default_value: Any,
572627
# :param block:
573628
# :return:
574629
"""
630+
# variation, variation_detail, and migration_variation all pass through here, so this one
631+
# check covers every evaluation entry point.
632+
self._check_forked()
633+
575634
hooks = [] # type: List[Hook]
576635
with self.__hooks_lock.read():
577636
if len(self.__hooks) == 0:

0 commit comments

Comments
 (0)