feat: add SetOnInvalidations to DedicatedClient#967
Conversation
Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
|
|
||
| // ClientTrackingOffCmd is predefined CLIENT TRACKING OFF | ||
| ClientTrackingOffCmd = Completed{ | ||
| cs: newCommandSlice([]string{"CLIENT", "TRACKING", "OFF"}), |
There was a problem hiding this comment.
Hi @jinbum-kim,
I think using this is probably wrong because we will never turn client tracking on again, right?
There was a problem hiding this comment.
Yes. I've removed CLIENT TRACKING OFF from CleanSubscriptions and dropped ClientTrackingOffCmd from this PR.
Should we instead send CLIENT TRACKING OFF when the invalidation hook is explicitly cleared, or leave that entirely to the caller?
There was a problem hiding this comment.
Hmm, this is a bit tough.
After a second thought, doing CLIENT TRACKING OFF automatically when the connection is returned to the pool might actually be a correct move because, for the dedicated pool, neither users nor we rely on client tracking in the pool. Client tracking is essentially off in the pool in the first place.
There was a problem hiding this comment.
Yeah, I think this is a bit tricky too 👀
Let me think about it a bit more and get back to you.
There was a problem hiding this comment.
Hi, It seems more appropriate to clean it up when the dedicated connection is returned to the pool, so that no tracking state is retained across reuse. I'll revise the PR in this way.
…ntTrackingOffCmd Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
…on close Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
…ns via wire getter Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
4d6bdf6 to
8da5d92
Compare
…o pool Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
| func (m *mux) Store(w wire) { | ||
| w.SetPubSubHooks(PubSubHooks{}) | ||
| w.CleanSubscriptions() | ||
| if w.Version() >= 6 { |
There was a problem hiding this comment.
Let's do this only when onInvalidations was set on the hooks.
There was a problem hiding this comment.
Should I also keep the w.Version() >= 6 guard, or is checking onInvalidations != nil alone sufficient?
hasOnInvalidations := w.GetPubSubHooks().onInvalidations != nil
...
if hasOnInvalidations (``&& w.Version() >= `6`) {
w.Do(context.Background(), cmds.ClientTrackingOffCmd)
}There was a problem hiding this comment.
I've updated it so that it only applies when onInvalidations is set. If you have any further suggestions, please let me know.
Signed-off-by: jinbum9958 <jinbum9958@gmail.com>
|
|
||
| type pshks struct { | ||
| hooks PubSubHooks | ||
| orig PubSubHooks |
There was a problem hiding this comment.
What will happen if we don't save this orig? Can we remove it?
There was a problem hiding this comment.
Hi @jinbum-kim, is it possible to remove this orig?
There was a problem hiding this comment.
Hi @rueian, Apologies for the delayed response. I recently started a new job, so my available time has been a bit limited 😅
I think orig is still needed.
SetPubSubHooks canonicalizes nil OnMessage / OnSubscription into internal no-op handlers, and SetOnInvalidations now does a read-modify-write through GetPubSubHooks():
hooks := c.wire.GetPubSubHooks()
hooks.onInvalidations = fn
return c.SetPubSubHooks(hooks)(in this commit)
SetPubSubHooks checks isZero() before canonicalization, so this works only if GetPubSubHooks() returns the original user-intent shape.
If GetPubSubHooks() returned the canonicalized form, then clearing fn would no longer reset back to zero when invalidations was the only originally-installed hook, because OnMessage / OnSubscription would already be non-nil.
That would break the documented behavior of SetOnInvalidations, which returns nil when clearing fn leaves no hooks installed.
So unless we preserve the original nil-ness some other way, I think we should keep orig.
There was a problem hiding this comment.
Thanks, I just removed the canonicalization.
There was a problem hiding this comment.
Thanks! Are there any additional points I should look into?
Signed-off-by: Rueian <rueiancsie@gmail.com>
|
Thanks for the contribution! @jinbum-kim |
|
My pleasure 😀 @rueian |

Closes #927
pipe.handlePushcurrently intercepts RESP3invalidatepush messages, soDedicatedClientusers cannot receive client-side caching invalidations on the same dedicated connection they use forCLIENT TRACKING.Instead, they currently need a separate client configured with
ClientOption.OnInvalidations.This PR adds
SetOnInvalidationstoDedicatedClientso invalidation messages can be handled on that same dedicated connection.Changes:
SetOnInvalidationstoDedicatedClientPubSubHooksand replace only the invalidation callbackCLIENT TRACKING OFFwhen a dedicated connection is returned to the pool, so no tracking state is retained across reuseExample:
Note
Medium Risk
Adds a new callback path for RESP3
invalidatepush messages and changes dedicated-connection pooling to issueCLIENT TRACKING OFFon reuse, which could affect client-side caching behavior if misused. Scope is contained to DedicatedClient/pubsub hook plumbing and has test coverage.Overview
Adds
DedicatedClient.SetOnInvalidations(fn)to let dedicated standalone and cluster clients receive RESP3invalidatepush messages on the same connection used forCLIENT TRACKING, without overwriting existingPubSubHooks.Plumbs invalidation delivery through
PubSubHooks(newonInvalidationsfield), addswire.GetPubSubHooks(), and updatespipe.handlePushto fan outinvalidateevents to both the existingClientOptioncallback and the dedicated hook path.Hardens pooling semantics: when a dedicated
wireis returned to the pool,mux.Storeclears hooks/subscriptions and, if invalidation hooks were installed, sendsCLIENT TRACKING OFFto avoid leaking tracking state across reuse. Updates mocks/wrappers and adds targeted tests for hook preservation, reset behavior, and tracking-off on store.Reviewed by Cursor Bugbot for commit e870346. Bugbot is set up for automated code reviews on this repo. Configure here.