fix: make OsduClient service-client initialisation thread-safe - #87
Open
shjellvik wants to merge 2 commits into
Open
fix: make OsduClient service-client initialisation thread-safe#87shjellvik wants to merge 2 commits into
shjellvik wants to merge 2 commits into
Conversation
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.
What
Makes lazy initialisation of
OsduClient's service clients and request adapters thread-safe, and adds a regression suite that fails reliably without the fix.Why
Service clients are built on first property access. A singleton
OsduClient— the normal DI registration — is hit by many requests at once during cold start, so that first access races.Measured on
main, 300 trials × 16 threads racing on first.Searchaccess:So ~90% of concurrent cold starts silently built duplicate clients over duplicate
HttpClients, and the unsynchronisedList<HttpClient>could be corrupted badly enough thatDispose()threw an NRE. Same numbers with the fix applied:0 / 300on every counter.This is why
petrodb-apicarries anOsduFacadeWarmuphosted service that touches every service property single-threaded at startup (equinor/petrodb-api#148). Once this lands, that workaround can go.Changes
Lockguarding all lazy-init state. Contention is irrelevant — each service is built at most once per client instance.Build(ref field, attr)with a singleDictionary<Type, object>client cache behindClient<T>(serviceAttr). Keyed by client type, not service name, becauseWellboreDdmsandWellboreDdmsBulkshare thewellbore_ddmsadapter.GetOrCreateAdapternow takes the same lock, so_adaptersand_httpClientsare no longer mutated concurrently.Dispose()takes the lock and clears the caches.Behaviour change worth flagging
Touching a service property or
GetRequestAdapterafterDispose()now throwsObjectDisposedException. Previously it returned a client sitting on a disposedHttpClient, which failed later and more confusingly inside Kiota. I judged this part of a coherent thread-safe lifecycle rather than separate scope — happy to drop it if you'd rather keep this PR strictly to the race.Verification
dotnet test tests/OsduCsharpClient.Tests— 44/44 pass.The four new lifecycle/race tests fail on
mainin 3/3 runs and pass with the fix. Confirmed by revertingOsduClient.cstoorigin/mainand rerunning:Note that none of this runs in CI yet — a separate PR adds the test workflow.