fix(sdk-core): avoid duplicate concurrent endpoint-discovery refresh calls - #7284
Open
Adityaj0 wants to merge 2 commits into
Open
fix(sdk-core): avoid duplicate concurrent endpoint-discovery refresh calls#7284Adityaj0 wants to merge 2 commits into
Adityaj0 wants to merge 2 commits into
Conversation
…calls EndpointDiscoveryRefreshCache#returnCachedOrDefaultEndpoint decides whether to kick off a background refresh of an expired cached endpoint with a plain check-then-act: read the cached entry's expirationTime, and if it's in the past, cache.put() a bumped-expiration copy and call refreshCacheAsync. Neither the read nor the put/decision is guarded, unlike the sibling "no cached entry yet" branch a few lines above, which correctly uses cache.putIfAbsent as a compare-and-swap so only one caller wins and triggers discovery. Every thread that reads the same expired entry before any of them writes back independently decides it's expired and independently calls refreshCacheAsync, firing duplicate calls against the real endpoint-discovery API for the same cache key -- exactly at the moment of highest concurrent load, right when an entry expires. Guard the refresh decision with cache.replace(key, oldValue, newValue), a compare-and-swap against the exact stale value each caller read, so only the thread that actually wins the race replaces the entry and triggers the refresh.
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.
Motivation and Context
fixes #7283
EndpointDiscoveryRefreshCache#returnCachedOrDefaultEndpointdecides whether to refresh an expired cached endpoint with an unguarded check-then-act:Every thread that reads the same expired entry before any of them writes back independently decides to refresh, firing duplicate
refreshCacheAsynccalls against the real endpoint-discovery API for the same key. The sibling "no cached entry yet" branch a few lines above already handles this correctly withcache.putIfAbsentas a compare-and-swap; this branch never got the equivalent treatment.Modifications
Guard the refresh decision with
cache.replace(key, oldValue, newValue), a CAS against the exact staleendpointvalue the caller read fromcache.get(key). Only the thread whose CAS succeeds — i.e. the entry hasn't already been replaced by a racing thread — proceeds to callrefreshCacheAsync:EndpointDiscoveryEndpointdoesn't overrideequals(), soConcurrentHashMap#replace(key, oldValue, newValue)'s equality check falls back to reference identity — which is exactly right here, since all racing threads observe the same object reference from the earliercache.get(key)read inget()/getAsync().Testing
Added
get_concurrentCallsOnExpiredEntry_onlyRefreshesOncetoEndpointDiscoveryRefreshCacheTest: primes the cache with an already-expired entry (via reflection into the privatecachefield, simulating the exact moment an entry expires), then fires 50 threads atget()simultaneously and asserts the mocked loader'sdiscoverEndpointis invoked exactly once.I don't have
mvn/a Maven-resolvable environment available locally, so I wasn't able to run this exact test file through the project's normal build. Instead I verified the underlying fix independently: I extracted a byte-for-byte faithful copy of the realEndpointDiscoveryRefreshCache.javaandEndpointDiscoveryEndpoint.java(only the auxiliaryEndpointDiscoveryRequest/EndpointDiscoveryCacheLoader/EndpointDiscoveryFailedExceptiontypes were replaced with minimal stand-ins matching their real signatures, to avoid pulling in the rest ofsdk-core's dependency graph) into a standalone harness, compiled with plainjavac(Java 21), and ran the same 50-thread concurrent-get()-on-expired-entry scenario as the new test:discoverEndpointcalls across 5 runs (expected: 1).I'm flagging this explicitly so a maintainer/CI can confirm the added test file itself compiles and passes cleanly in the real build — the fix's correctness is verified, but the exact test file hasn't been run through the project's own toolchain by me.
Types of changes
Checklist
./mvnw packagesucceeds -- not verified locally (nomvnavailable in my environment); verified the underlying fix via an independent standalone harness instead, see Testing section