test: Add failing test for thread leak when construction fails - #500
Open
aviadr1 wants to merge 1 commit into
Open
test: Add failing test for thread leak when construction fails#500aviadr1 wants to merge 1 commit into
aviadr1 wants to merge 1 commit into
Conversation
__start_up() creates the event processor - starting a dispatcher thread, a pool of flush workers and two repeating timer threads - and only then starts the data system. If the data system fails to start, the exception propagates out of the constructor, the caller never receives a client object, and there is no handle on which to call close(). Everything already started keeps running. The test uses a configured update_processor_class that raises in start(), and currently reports eight leaked threads: ldclient.events.context-flush.repeating, ldclient.events.flush.repeating, ldclient.events.processor, ldclient.flush.1 .. ldclient.flush.5 plus the event processor's HTTP connection pool. These are daemon threads so they do not prevent process exit, but they leak steadily in any application that retries client construction, and postfork() re-runs this same path. Marked xfail strict so it fails loudly once the behaviour is fixed and the marker can be removed. No fix is proposed here. 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 #497. Test only — no fix.
Found while auditing the shutdown path after #493, a production incident where
LDClient.close()hung and left worker pods alive for hours to days. I have not observed this particular leak in our own production, but it reproduces reliably.What the test pins
__start_up()creates the event processor atclient.py:320— starting a dispatcher thread, five flush workers, two repeating timer threads and an HTTP connection pool — and only then starts the data system. If the data system fails to start, the exception propagates out of the constructor, the caller never receives a client object, and there is no handle on which to callclose().test_failed_construction_does_not_leak_background_threadsconfigures anupdate_processor_classthat raises instart()— a documented configuration hook — and reports:These are daemon threads, so this does not block process exit. It leaks steadily in any application that retries client construction, and
postfork()re-runs this same path.Why xfail
Marked
@pytest.mark.xfail(strict=True)so CI stays green while the defect is documented; being strict, it fails loudly once fixed. To see the real failure:Why no fix
Where the teardown belongs — a try/except in
__start_up(), or restructuring so the event processor starts last — is a design call.Noted in #497 and not covered by this test:
postfork()re-runs__start_up()without stopping the previous components, so the old urllib3 pools' inherited socket descriptors are never released in the child. I read that rather than tested it.Note
Overview
Adds a test-only change (no fix) that documents a resource leak when synchronous
LDClientconstruction raises after__start_up()has already started the event processor.The new test
test_failed_construction_does_not_leak_background_threadsuses a customupdate_processor_classthat fails instart()— a documented hook — so startup fails after the default event processor has spun up dispatcher, flush workers, and repeating timer threads. The test asserts the invariant that a failed constructor leaves noldclient.*threads running; today it fails with seven leaked thread names.The test is marked
@pytest.mark.xfail(strict=True)so CI stays green until teardown is implemented (e.g. try/except in__start_up()or reordering startup). AsyncLDClient already tears down on failedstart(); this pins the gap on syncLDClient.Reviewed by Cursor Bugbot for commit 2ebef9d. Bugbot is set up for automated code reviews on this repo. Configure here.