Repository navigation
perf(flow-chat): guard stale subscription cleanup and add MRU fast path for reader state - #3831
Open
Raing5Days wants to merge 1 commit into
Open
Raing5Days wants to merge 1 commit into
Raing5Days wants to merge 1 commit into
Conversation
…th for reader state
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.
Problem
The reader store had three cleanup paths that only checked whether an owner set had become empty — never whether the set being deleted was still the one registered for that key. A stale lease release or unsubscribe closure that ran after the same turn/group/scope had been re-created could therefore evict the newer bucket, silently dropping live subscribers and leaving
interaction:*/group:*keys stuck at their previous value. On top of that, everysetand everynotifyProjectionallocated a fresh dispatch closure viaforEach(listener => listener()),subscribeallocated aSetand an extramap.seteven when a bucket already existed, and scope resolution always performed adelete+setLRU bump on the backingMap, turning the hottest lookup path into needless map churn.Solution
!owners.size && this.turnInteractions.get(turn) === owners(same pattern forgroupHolds). Thesubscribedisposer returns early whendeletewas a no-op and only removes the key whenthis.listeners.get(key) === listeners, so a stale closure can never evict a newer bucket that happens to share the key.notifyProjectionand the listener fan-out insetiterate the liveSetwithfor..ofinstead offorEach+ arrow. No closure per notification, andSetiteration is mutation-tolerant, so a listener that unsubscribes mid-dispatch is handled correctly.get, and the bucket is created asnew Set([listener])only on a miss.mostRecentScopeskips thedelete+setbump when the resolved scope is already the most recently used one — correct becauseMap#seton an existing key does not reorder. The registry stays capped atMAX_REMEMBERED_VIEWS = 24; nothing became unbounded and no counters or caches were added beyond that single string.The public surface is unchanged: callers see the same synchronous read/write semantics, the same
revisionbumps, and the same cleanup contracts. Internally each mutation still replaces state rather than mutating shared structures, so immutability at the boundary is preserved.Verification
Mapin identical contents and ordering, and that the registry never exceeds its cap.