fix(core): ref-count store event listener to fix owner-first close leak#3058
Open
dpol1 wants to merge 3 commits into
Open
fix(core): ref-count store event listener to fix owner-first close leak#3058dpol1 wants to merge 3 commits into
dpol1 wants to merge 3 commits into
Conversation
…ak (apache#3033) storeEventListenStatus had the same owner-first-close bug apache#3017 fixed for graph cache listeners: non-owner close() dropped the entry and skipped unlisten() as a no-op, leaking the owner's listener. Apply CacheListenerHolder ref-count pattern: StoreListenerHolder + STORE_EVENT_LISTENERS in CachedGraphTransaction, acquire/release via compute() with provider-identity guard for close/reopen. Removes storeEventListenStatus from GraphTransaction and restoreStoreListenerStatusForKnownTeardownBug workaround from tests. CachedSchemaTransaction* unaffected (per-instance, balanced 1:1).
dosubot
Bot
added
size:L
imbajin
reviewed
Jun 10, 2026
imbajin
left a comment
imbajin
left a comment
Member
There was a problem hiding this comment.
Blocking: no. Summary: Please add one store-listener lifecycle test for the owner-first close path. Evidence: static review of CachedGraphTransactionTest; local Maven test was blocked by unresolved ${revision} dependency while current CI checks pass.
- Add testStoreListenerSurvivesOwnerClose: close the owner transaction first, fire STORE_CLEAR through the provider, and assert the surviving transaction still clears its cache via the ref-counted provider listener. - Add testReopenGraphReRegistersStoreListener: assert last close drops the store registry entry and unregisters the listener, and reopen re-registers a fresh holder. The provider instance is pooled across reopen, so the provider-replacement branch stays defensive and is not exercised here.
The ref-count refactor moved listener ownership into StoreListenerHolder, leaving the storeEventListener instance field assigned but never read. Remove it; cacheEventListener stays as notifyChanges() still reads it.
imbajin
approved these changes
Jun 11, 2026
imbajin
left a comment
imbajin
left a comment
Member
There was a problem hiding this comment.
Blocking: no. Summary: No obvious issues found in the current head. Evidence: CachedGraphTransactionTest passed locally and latest-head CI checks are green.
Member
Author
|
@imbajin This PR covers the CGT fix. A follow-up improvement for CachedSchemaTransaction will be tracked separately. Not a bug or a hurry issue, but an improvement that will be calmly implemented. |
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
2 participants
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.
Purpose of the PR
close #3033
storeEventListenStatushad the same owner-first-close bug #3017 fixedfor graph cache listeners: non-owner
close()dropped the tracking entryand skipped
unlisten()as a no-op, leaving the owner's listenerregistered but untracked — leak + no store-event cache invalidation.
Main Changes
CachedGraphTransaction: addStoreListenerHolderinner class(listener, provider, refCount) +
STORE_EVENT_LISTENERSstaticregistry; acquire/release via
ConcurrentMap.compute()withprovider-identity guard for graph close/reopen. TODO block removed.
Drop write-only
storeEventListenerinstance field — listenerownership moved to
StoreListenerHolder, field was assigned butnever read.
GraphTransaction: removestoreEventListenStatusdeclarationand orphaned
ConcurrentHashMapimport. Note:storeEventListenStatuswas
protected static— minor API surface removal, internal only,no known external use.
CachedGraphTransactionTest: deleterestoreStoreListenerStatusForKnownTeardownBugworkaround; repointreflection helper to
STORE_EVENT_LISTENERS; add 5 regression tests:non-owner close keeps listener registered, last close removes listener,
owner-first close with real
STORE_CLEARfires through provider andsurviving tx clears cache, reopen re-registers a fresh holder. Provider
is pooled across reopen — provider-replacement branch stays defensive,
not exercised.
CachedSchemaTransaction*unaffected — per-instance, balanced 1:1.Verifying these changes
NoSuchFieldExceptiononSTORE_EVENT_LISTENERS), green after fix.CachedGraphTransactionTestCachedGraphTransactionTest,CachedSchemaTransactionTest,CacheManagerTest,CacheTestDoes this PR potentially affect the following parts?
Documentation Status
Doc - No Need