fix(core): ref-count store event listener to fix owner-first close leak#3058
fix(core): ref-count store event listener to fix owner-first close leak#3058dpol1 wants to merge 4 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
left a comment
imbajin
left a comment
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
left a comment
imbajin
left a comment
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.
|
@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. |
VGalaxies
left a comment
VGalaxies
left a comment
There was a problem hiding this comment.
Review summary
- Blocking: no
- Summary: One low-severity compatibility risk remains from removing a protected field.
- Evidence:
git diff --unified=0 origin/master...HEAD -- hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java
…tenStatus Document in-source that the ref-counted STORE_EVENT_LISTENERS registry supersedes the removed protected static storeEventListenStatus field on GraphTransaction, addressing the API-removal review thread on apache#3058.
|
Addressed - Also updated PR description |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: No obvious issues found in the current head. Evidence: git diff --check; CachedGraphTransactionTest passed locally (13 tests); latest-head checks pass.
3 participants
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.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.Breaking change (internal API)
This removes the
protected static final ConcurrentHashMap<String, Boolean> storeEventListenStatusfield fromGraphTransaction. The removal isintentional and mirrors #3017, which already removed the sibling
graphCacheListenStatus(sameprotected static, same 1.7.0 vintage) with nocompatibility shim when graph-cache listening moved to the ref-counted holder.
CachedGraphTransaction; no external extension point uses it.@Deprecatedno-op field cannot preserve the oldper-graph boolean semantics (now replaced by ref-counting), so it would be
dead, misleading static state.
GraphTransactionand reads this internal field directly. Store-eventlistening is now handled transparently by
STORE_EVENT_LISTENERS.Verifying these changes
NoSuchFieldExceptiononSTORE_EVENT_LISTENERS), green after fix.CachedGraphTransactionTestCachedGraphTransactionTest,CachedSchemaTransactionTest,CacheManagerTest,CacheTestDoes this PR potentially affect the following parts?
protected staticstoreEventListenStatusfield fromGraphTransaction— an intentional,internal-only breaking API change (see "Breaking change (internal API)"
above), mirroring fix(server): align cache event actions in legacy EventHub path #3017. No known external use.
Documentation Status
Doc - No Need