Bug fixed. Dependency inverted. Pre-existing flaky test fixed#845
Open
Ni7ram wants to merge 3 commits into
Open
Bug fixed. Dependency inverted. Pre-existing flaky test fixed#845Ni7ram wants to merge 3 commits into
Ni7ram wants to merge 3 commits into
Conversation
gthea
approved these changes
May 21, 2026
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.
Problem
Users who held only a reference to
SplitClient(releasing theSplitFactory) couldexperience the SDK never reaching
.sdkReady. The factory was the sole strong owner ofSplitClientManager, which in turn ownedeventsManagerCoordinator,syncManagerandall the infrastructure required to drive the client to ready. Once the factory was
released, everything died and the client became inert.
A workaround existed in
SplitEventActionTask, which kept a strongfactory: SplitFactoryproperty purely to keep the factory alive via the cycle
factory → clientManager → eventsManagerCoordinator → eventsManager → task → factory.This worked only as long as the user registered at least one event listener and coupled
an event-running task to factory ownership.
Changes
This PR replaces the workaround with a direct ownership change:
DefaultSplitClient.clientManager:weak→ strong (let). The client now keeps itsclient manager (and the rest of the infra) alive on its own.
SplitEventActionTask.factoryremoved (and from all call sites).SplitClientManager.splitFactoryremoved (only consumer was the task workaround).LocalhostSplitClient.clientManagerremoved (was unused).Existing cycles between
DefaultClientManager ↔ defaultClientand throughbyKeyRegistryare preserved by design; they get broken bydestroy()exactly asbefore, so no leak semantics change. The difference is that the cycle now reflects the
real ownership model rather than an opaque workaround.
Bonus
Fixes a pre-existing flaky test in
LocalhostTests.testUsingYamlFromApi: the.sdkUpdatedlistener was being registered afterupdateLocalhost(yaml:)was called,which meant the event could fire before the listener was attached and be silently
dropped by
DefaultSplitClient.on(event:)'seventAlreadyTriggeredguard. Reordered toregister first, trigger second.