fix: guarantee cleanup of stale data on re-subscriptions #15929
Conversation
Currently, collection cleanup code is only run when an added event is received from the server. Where that fails is in scenarios where a server-side collection turns empty while an affected users is disconnected - and then reconnects. There's no removed (or updated) event so no cleanup code is run and you have stale data. This commit guarantees a stale data check is run whenever a subscription is established again. The `added` check was also maintained, although I'm not too sure anymore it's is still needed. That may need to be revisited.
|
SonarCloud Quality Gate failed.
|
|
I tagged this as closing #15829 and #15823 although it's worth nothing: the recorded videos show "inconsistencies" while the client is disconnected, and this just tackles state reconciliation after re-connecting. Do not expect this PR to fix the fact that while the client is disconnected from the server that does state management (Meteor), you can't "unshare" or "retry" something. If we want to tackle that, we need to properly surface the lack of connectivity to the signaling server (Meteor) to the end user - and explain why they can't do that.
|
|
|
||
| self.checkForStaleData(); |
There was a problem hiding this comment.
Could you add a throttle for this function? I think thats good have it, but I think it could add performance issues call it while the client is sync bacause it would generates a lot of calls
There was a problem hiding this comment.
Could you add a throttle for this function? I think thats good have it, but I think it could add performance issues call it while the client is sync bacause it would generates a lot of calls
I could add a tail debounce, yes. Although I'd like to clear some things up:
- I don't think it'll "add" performance issues because it's virtually the same as the status quo (v2.5.x/v2.6.x have the same code block as this hooked to the
addedcallback - the only difference here is that I refactored it to a method to be re-usable) - From the PR:
The added check was also maintained, although I'm not too sure it's still needed. That may need to be revisited. I'm under the impression this doesn't need to be in theaddedcallback anymore if it is run once the subscription is set up. If you can confirm that is the case, I'll just remove it.
There was a problem hiding this comment.
^ Even though this was merged, It'd be important to verify whether the added cleanup callback is still needed.
| @@ -164,6 +167,10 @@ export default withTracker(() => { | |||
| }, | |||
| ...subscriptionErrorHandler, | |||
| }); | |||
|
|
|||
| Object.values(localCollectionRegistry).forEach( | |||
There was a problem hiding this comment.
Also add a throttle here, if it runs as is, it'll generate 961 calls for checkForStaleData
There was a problem hiding this comment.
If it generates 961 calls, throttling does not seem like the solution - if anything it should be debounced.
Do you have a different place to suggest where this will be called only when absolutely necessary?
There was a problem hiding this comment.
Also: how did the 961 calls number come up here? Can you provide a reproducible scenario so I can tinker with it?
The way I initially came up with this code is hat this loop would only be run once every subscription handler is ready - so this would amount to the N collection calls to checkForStaleData (which currently is 32).
I just re-validated that adding a call counter stemming from this entrypoint only and it's just that: 32 calls per (re)-connection.
What does this PR do?
addedcheck was also maintained, although I'm not too sure it's still needed. That may need to be revisited.Closes Issue(s)
Part of #15829
Part of #15823
Motivation
See #15823 (comment)