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 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
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
| @@ -140,8 +139,6 @@ public class GraphTransaction extends IndexableTransaction { | |||
|
|
|||
| private final int verticesCapacity; | |||
| private final int edgesCapacity; | |||
There was a problem hiding this comment.
Low: Protected listener state field was removed
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:141
Evidence
- The diff removes
protected static final ConcurrentHashMap<String, Boolean> storeEventListenStatusfromGraphTransaction;origin/masterexposed it at old lines 143-144.
Impact
- External subclasses or same-package extensions compiled against this protected member can fail to compile, and already-compiled code that references the field can fail linkage after upgrade.
Requested fix
- Keep a deprecated compatibility field, or explicitly document this as a breaking internal API change in the release notes.
There was a problem hiding this comment.
@VGalaxies The removal is intentional and scoped. This PR only drops storeEventListenStatus, mirroring #3017, which already removed the sibling graphCacheListenStatus from this same class (also protected static final, same 1.7.0 vintage) when it moved graph-cache listening to the ref-counted GRAPH_CACHE_EVENT_LISTENERS holder. The // TODO (follow-up): storeEventListenStatus has the same owner-first close bug… comment #3017 left in CachedGraphTransaction scoped exactly this change.
I'd prefer not to keep a deprecated compatibility field:
- It's internal listen-tracking state consumed only by the in-tree
CachedGraphTransaction; no external extension point uses it, and fix(server): align cache event actions in legacy EventHub path #3017 already set the precedent of removing the equivalent field outright without a shim. - A
@Deprecatedno-op field can't preserve the old per-graph boolean semantics (now replaced by ref-counting), so it would be dead, misleading static state.
The PR description already notes this as an intentional internal-API removal; I can expand that into an explicit breaking-change/release-note line if you'd prefer. Does that work?
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. 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