fix: read the external resource cache under the event source monitor - #3524
Draft
csviri wants to merge 1 commit into
Draft
fix: read the external resource cache under the event source monitor#3524csviri wants to merge 1 commit into
csviri wants to merge 1 commit into
Conversation
`ExternalResourceCachingEventSource` mutates its cache from `synchronized`
methods (`handleResources`, `handleDelete`,
`handleRecentResourceCreate/Update`), but the read paths were not
synchronized. The outer map is a `ConcurrentHashMap`; the nested
per-primary maps are plain `HashMap`s that `handleDelete` mutates in
place, so a reconciler thread reading them while a poll or informer
thread writes can observe a corrupted map or throw.
`getSecondaryResources(ResourceID)` additionally looked the primary up
twice:
var cachedValues = cache.get(primaryID);
if (cachedValues == null) { return Collections.emptySet(); }
else { return new HashSet<>(cache.get(primaryID).values()); }
If a concurrent `handleDelete` removes the entry between the two calls,
the second `get` returns null and this throws a NullPointerException.
Adds a `cachedResourcesFor` helper that snapshots the cached resources
while holding the monitor, and routes `getSecondaryResources` plus the
`PerResourcePollingEventSource` and `CachingInboundEventSource` overrides
(and `checkAndRegisterTask`) through it. The helper only copies, so the
potentially slow `ResourceFetcher` calls in those overrides still run
outside the lock and cannot block the informer or poll threads.
`getCache()` still returns a live view for backwards compatibility, but
now documents that iterating the nested maps requires synchronizing on the
event source.
Adds a test asserting `getSecondaryResources` returns a snapshot rather
than a live view.
16 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrency correctness issue in ExternalResourceCachingEventSource by ensuring cache read paths take a synchronized snapshot of per-primary cached resources (the nested maps are HashMaps and are mutated under the event source monitor). This avoids races that could lead to corrupted reads or TOCTOU NullPointerExceptions.
Changes:
- Added a
cachedResourcesFor(ResourceID)helper that synchronizes on the event source and returns a snapshot copy of cached secondary resources. - Routed
getSecondaryResources(ResourceID)and the relevant overrides in polling/inbound event sources through the snapshot helper. - Documented that
getCache()returns a live view and that iterating nested maps requires synchronizing on the event source; added a regression test asserting snapshot behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java | Introduces synchronized snapshot helper for cache reads and documents thread-safety expectations of getCache(). |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PerResourcePollingEventSource.java | Uses cachedResourcesFor to safely snapshot cached resources when registering tasks and when serving getSecondaryResources. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/inbound/CachingInboundEventSource.java | Uses cachedResourcesFor to safely snapshot cached resources in getSecondaryResources. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java | Adds a test verifying getSecondaryResources returns a snapshot, not a live view. |
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.
ExternalResourceCachingEventSourcemutates its cache fromsynchronizedmethods (
handleResources,handleDelete,handleRecentResourceCreate/Update), but the read paths were notsynchronized. The outer map is a
ConcurrentHashMap; the nestedper-primary maps are plain
HashMaps thathandleDeletemutates inplace, so a reconciler thread reading them while a poll or informer
thread writes can observe a corrupted map or throw.
getSecondaryResources(ResourceID)additionally looked the primary uptwice:
If a concurrent
handleDeleteremoves the entry between the two calls,the second
getreturns null and this throws a NullPointerException.Adds a
cachedResourcesForhelper that snapshots the cached resourceswhile holding the monitor, and routes
getSecondaryResourcesplus thePerResourcePollingEventSourceandCachingInboundEventSourceoverrides(and
checkAndRegisterTask) through it. The helper only copies, so thepotentially slow
ResourceFetchercalls in those overrides still runoutside the lock and cannot block the informer or poll threads.
getCache()still returns a live view for backwards compatibility, butnow documents that iterating the nested maps requires synchronizing on the
event source.
Adds a test asserting
getSecondaryResourcesreturns a snapshot ratherthan a live view.
Part of #3517