Skip to content

Fix channel cache GC mark/sweep and chanByCID leak - #215

Open
bisegni wants to merge 3 commits into
epics-base:masterfrom
bisegni:fix/pva-channel-cleanup
Open

bisegni wants to merge 3 commits into
epics-base:masterfrom
bisegni:fix/pva-channel-cleanup

Conversation

@bisegni

@bisegni bisegni commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Fix two issues in the client channel cache garbage collection:

  • chanByCID leak: Expired weak_ptr entries in chanByCID were never removed, causing the map to grow indefinitely and wasting CID space. Added compaction in cacheClean() to erase expired entries after sweeping chanByName.
  • GC mark/sweep logic: The cacheClean() mark/sweep correctly marks unused channels as garbage on the first pass and sweeps them on the second. cacheClear() already calls cacheClean() twice internally to ensure both phases run.

Tests (testgc.cpp):

  • Verify channel presence in cache before sweep and absence after sweep via report()
  • Verify channel reuse by checking single channel count and growing tx bytes across consecutive gets
  • Verify Disconnect action triggers immediate sweep and disconnect callback

The two-phase mark/sweep in cacheClean() was broken: garbage flag was
set unconditionally before the mark test, making the mark branch dead
code. Every idle channel was swept immediately with no grace period,
causing constant channel rebuild churn under polling workloads.

Move garbage=true into the mark branch so idle channels are marked on
one tick and swept only if still idle on the next. This restores the
intended grace period where Channel::build() can reset the flag.

Also compact chanByCID during cacheClean to remove expired weak_ptr
entries left by destroyed channels, preventing unbounded map growth.

Add testgc to verify mark/sweep, channel reuse, and disconnect behavior.
The connect() op may fire an initial onDisconnect before onConnect.
Use a polling loop on the current-state flag (matching testget.cpp
pattern) and compare disconnect counts relative to a snapshot instead
of assuming zero.
Verify channel presence/absence in cache via report() around
mark/sweep, and confirm channel reuse by asserting single channel
count and growing tx bytes across consecutive gets.
@bisegni
bisegni marked this pull request as ready for review September 11, 2026 19:22
@mdavidsaver

Copy link
Copy Markdown
Member

I picked off part of this PR as 928d572, and made a63627f to ensure consistence between the two chanBy* maps.

@bisegni

bisegni commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

great thanks!

@mdavidsaver mdavidsaver added this to the 1.5.3 milestone Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants