Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v4: Better support for handling push messages #269

Open
nussjustin opened this issue Apr 5, 2021 · 1 comment
Open

v4: Better support for handling push messages #269

nussjustin opened this issue Apr 5, 2021 · 1 comment

Comments

@nussjustin
Copy link
Contributor

nussjustin commented Apr 5, 2021

Currently users must manually handle push responses from Redis which is not only tedious, but also impractical since push responses can come before any other response so handling them correctly requires checking for push responses on any command!

A case where this may happen is with client tracking. In this case the server can send out push messages at any point since the push can be caused by any connection. Even if an application uses the [REDIRECT client-id] option to redirect the push messages to another client, a push message may still be received when the other client is closed.

To better support handling of push messages, both in general and for use with server-assisted client side caching, we should think about a way to let users handle push messages automatically.

From my perspective there are 2 obvious places where such logic could live:

  1. In the Conn implementation(s).
  2. In the Unmarshalers / the Unmarshal function

There may be other options, but none come to my mind.

Option 1 would probably be the simplest solution. We could add an optional handler function that receives the push responses (or just a reader) and can do with them whatever it wants, with a default / nil handler that discards pushes.

Option 2 would be more complex and every Unmarshaler would need to manually handle push messages and we would need to somehow pass the handler to UnmarshalRESP (e.g. as a new field on resp.Opts).

Given these 2 options I think option 1 would be the best.

This would probably need some extra logic to allow our pubsub logic to continue working since pubSubConn would not be able to receive pushes anymore. It may be possible to handle this as a special case in our Conn implementation, but it may be better to add "first-level" support for handling pushes on the command level. The idea here is to add a new interface in the resp package that would allow unmarshaling pushes instead of leaving this to the Conn.

Something like this

type PushUnmarshaler interface {
    UnmarshalRESPPush(BufferedReader, *Opts) error
}

Using the default Conn implementation (conn) as an example, we could then do something like this

p, err := c.br.Peek(1)
if err != nil {
    // ...
}

if resp3.Prefix(p[0]) == resp3.PushHeaderPrefix {
    if pu, ok := mu.unmarshalInto.(resp.PushUnmarshaler); ok {
        err := pu.UnmashalRESPPush(br, o)
        if err != nil {
            // ...
        }
    } else {
        handler := c.pushHandler // our callback
        if handler == nil {
            handler = discardHandler // handler that just discards the message
        }
        err := handler(c.br, c.rOpts)
        if err != nil {
            // ...
        }
    }
}

err := resp3.Unmarshal(c.br, mu.unmarshalInto, c.rOpts)
if err != nil {
    // ...
}

A bit off topic: Looking at the PushUnmarshaler interface I can't help but think that something like this may be useful for dealing with attributes as well.

@nussjustin
Copy link
Contributor Author

Having thought about this a bit more, I think a PushUnmarshaler interface that can be implemented by any Unmarshaer isn't really as clean as I initially thought. While it should work, having any Unmarshaler possibly also handle pushes, which have nothing do to with the normal response, just seems like the wrong solution. (Though something like an AttributeHandler interface for unmarshalers that can unmarshal their own attributes still sounds like an interesting idea to me, but that's for another issue)

Maybe we could just have a concrete implementation (e.g .a PushReceiver) that unmarshals pushes into a Rcv interface{} field which we then could handle as a special case. Basically if a Conn needs to unmarshal a response and the receiver is of this concrete type we would skip the normal PushHandler.

So instead of this

    if pu, ok := mu.unmarshalInto.(resp.PushUnmarshaler); ok {
        err := pu.UnmashalRESPPush(br, o)
        if err != nil {
            // ...
        }
    } else {
        handler := c.pushHandler // our callback
        if handler == nil {
            handler = discardHandler // handler that just discards the message
        }
        err := handler(c.br, c.rOpts)
        if err != nil {
            // ...
        }
    }

we could have something like this

    if _, isPushUnmarshaler = mu.unmarshalInto.(PushUnmarshaler); !isPushUnmarshaler {
        handler := c.pushHandler // our callback
        if handler == nil {
            handler = discardHandler // handler that just discards the message
        }
        err := handler(c.br, c.rOpts)
        if err != nil {
            // ...
        }
    }

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

No branches or pull requests

1 participant