fix: reuse the DDP subscription a stream already has (#372) - #373
Open
diegolmello wants to merge 11 commits into
Open
fix: reuse the DDP subscription a stream already has (#372)#373diegolmello wants to merge 11 commits into
diegolmello wants to merge 11 commits into
Conversation
`subscribe(name, params)` with no explicit id always minted a new DDP subscription id, even for a stream already recorded. `reopenNow()` does not close, so the records survive a reconnect, and any caller that re-subscribes as part of its own post-login flow landed a second record for the same stream. Each reconnect added another: one `sub` frame and one callback invocation per accumulated record. The reuse turns on stream identity, so it has to be exact. `findSubscriptions` matches params by prefix, which would collapse distinct `stream-notify-logged` streams into one, and its element comparison is by reference, which never matches the options object every `Driver.subscribe` rebuilds per call — the streams the issue measured growing. Identity is now the serialized name and params in full, with object keys sorted. Two callers sharing one DDP subscription means the first `unsubscribe` would end the stream for both, so holders are counted per id and the `unsub` waits for the last of them. A count is taken only when a caller is handed a subscription, so the entry an abandoned `sub` writes — which resolves `undefined` — has no holder. `unsubscribeAll` drops the counts, since a teardown of everything ends a stream whoever else holds it. An entry whose `unsub` is already on the wire is not reusable: reusing it hands the new caller a stream the server is about to end, with no `sub` ever sent. See ADR-0011.
A subscribe arriving while unsubscribeAll ran awaited a stale request whose subscription was gone, and was answered with nothing. Also names the holder rule positively as hasOtherHolders, patches one subscription state record instead of rebuilding it, and drops the annotating comments.
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.
Proposed changes
Closes #372.
Socket.subscribe(name, params)with no explicit id always minted a new DDP subscription id, even when an identical stream was already recorded.reopenNow()does not close, so records survive a reconnect, and any caller that re-subscribes in its own post-login flow landed a second record for the same stream — one more per reconnect, each costing asubframe and a callback invocation per incoming event.subscribenow hands back the DDP subscription a stream already has, whether recorded or still in flight. The explicit-id resume path (subscribeAll,resubscribeWhenRecorded) is unchanged.Stream identity is exact, since the reuse turns on it.
findSubscriptionsmatches params by prefix, which would collapse distinctstream-notify-loggedstreams into one, and compares elements by reference, which never matches the options object everyDriver.subscriberebuilds per call — the streams the issue measured growing. Identity is the serialized name and params in full, keys sorted.Two callers sharing one DDP subscription means the first
unsubscribewould end the stream for both, so holders are counted per id and theunsubwaits for the last of them. A count is taken only when a caller is handed a subscription, so the entry an abandonedsubwrites — which resolvesundefined— has no holder.unsubscribeAlldrops the counts, since a teardown of everything ends a stream whoever else holds it. An entry whoseunsubis already on the wire is not reusable, or the new caller receives a stream the server is about to end with nosubever sent.ADR-0011 records the decision.
Steps to reproduce
subscribe(name, params)with no explicit id.reopenNow()runs.socket.subscriptions— before this change, two records for one stream; after it, one.Tests