test: Add failing test for set_config() blocking concurrent get() - #499
Open
aviadr1 wants to merge 1 commit into
Open
test: Add failing test for set_config() blocking concurrent get()#499aviadr1 wants to merge 1 commit into
aviadr1 wants to merge 1 commit into
Conversation
set_config() holds the global write lock (ldclient/__init__.py) across both the construction of the replacement client and old_client.close(). ReadWriteLock holds the underlying mutex for the whole write section, so every concurrent ldclient.get() - which takes a read lock, and which sits on the hot path of every flag evaluation - blocks until both finish. The test drives a 3 second close() and measures how long a concurrent get() blocks; it currently reports 2.8s. Marked xfail strict so it fails loudly once the behaviour is fixed and the marker can be removed. This couples a network-dependent shutdown to a lock that every evaluating thread needs, so a slow or stalled close() stalls the whole application rather than just the thread that called set_config(). No fix is proposed here; this only pins the contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Failing test for #496. Test only — no fix.
Found while investigating #493, a production incident where
LDClient.close()hung and left worker pods alive for hours to days. This issue is what makes that class of hang so damaging under the singleton API. I have not observed this specific path in our own production — we do not callset_config()after startup — but the coupling is real and reproduces reliably.What the test pins
set_config()holds the global write lock across both the construction of the replacement client andold_client.close()(ldclient/__init__.py:38-47).ReadWriteLock.lock()holds the underlying mutex for the whole write section andrlock()needs that same mutex, so every concurrentldclient.get()blocks — andget()is on the hot path of every flag evaluation in an application using the singleton API.test_set_config_does_not_block_concurrent_getdrives a 3 secondclose()and measures a concurrentget():Before a shutdown timeout existed, an unbounded
close()here would block everyget()in the process indefinitely. Even bounded, the window isstart_wait(default 5s) plus the shutdown timeout, so a reconfigure can stall all evaluating threads for roughly ten seconds.Why xfail
Marked
@pytest.mark.xfail(strict=True)so CI stays green while the defect is documented; being strict, it fails loudly once fixed so the marker gets removed. To see the real failure:Why no fix
Moving construction and
close()outside the lock is the obvious direction, but doing it without introducing a lost-update race between concurrentset_config()callers is a design decision I would rather leave with you.Note
Overview
Test-only change for issue #496: documents a singleton API defect where
set_config()keeps the global write lock while it builds the replacement client and runsold_client.close(), so concurrentldclient.get()(read lock) stalls on the same mutex.Adds
test_set_config_does_not_block_concurrent_get, which patchesLDClient.closeto sleep 3s during reconfigure and asserts a concurrentget()finishes in under half that time. The test fails today (~2.8s block) and is marked@pytest.mark.xfail(strict=True)so CI stays green until a fix lands.No library changes; the PR pins the invariant that reconfigure should not couple slow shutdown to the flag-evaluation hot path.
Reviewed by Cursor Bugbot for commit ce3e272. Bugbot is set up for automated code reviews on this repo. Configure here.