jhthorsen / mojo-redis

Non-blocking Redis driver using Mojo::IOLoop
https://metacpan.org/pod/Mojo::Redis
14 stars 11 forks source link

Enhancement: pubsub->json() too global? #72

Closed Yenya closed 2 years ago

Yenya commented 2 years ago

There is a problem with $pubsub->json($channel)->... call chain - it switches the given channel to a JSON en/decapsulation, and does so permanently. But because all requests in a single server process share the same pubsub connection, the ->json() call switches the channel to JSON for all handlers running within the same UNIX process, even though they did not call ->json() themselves. So the results on multi-process servers (prefork or hypnotoad) differ from what can be seen in a single-process development environment (morbo), which is a bit counterintuitive.

Our use case, as requested on IRC: we have a websocket app with two types of users, say teacher(s) and students. The app uses two pubsub channels: "all" for every user, and "admin" for one-way communication of users with the teacher (think answering the teacher's questions). So the teacher's websocket handler subscribes both to all and admin channels, and switches them to JSON mode. The students' handlers only subscribe to all, also with ->json('all') but ocasionally send messages also to admin, even though they do not listen on that channel. The result depends whether the student's handler runs inside a process where also a teacher's handler is currently running. If it does, the handler's ->notify() calls send the hashrefs as encoded JSON. Otherwise, the message gets stringified to something like HASH(0xdeadbeef). So the student's handler have to either call ->json('admin')->notify('admin') every time, or track the channels which were already switched to JSON mode by itself.

I can imagine that for debugging purposes, somebody would even want to write a listener for raw messages on some channel, even though it is running inside the same process as other handlers that expect JSON messages on that channel. So switching a particular pubsub connection to the JSON mode globally for a channel is not correct.

I think there should not be a standalone ->json(), but instead something like $pubsub->notify_json() and $pubsub->listen_json(), and the ->json($channel)->listen/notify($channel) syntax should be deprecated.

What do you think about it? Thanks,

-Yenya

jhthorsen commented 2 years ago

Thanks for creating the issue, though I did misunderstand your request. I thought you wanted to say "turn json decoding on for all messages, instead of a specific channel". I think I misunderstood.

Here are some comments:

...all handlers running within the same UNIX process...

That is not correct. It does so for all topics in the given $pubsub object. It sounds like what you are doing is that you create one $redis object for the application and then you do $redis->pubsub->json("foo") which will try to encode/decode all "foo" messages.

So the results on multi-process servers (prefork or hypnotoad) differ from what can be seen in a single-process development environment (morbo), which is a bit counterintuitive

There really isn't any difference. There's no globals, even though you might use a singletong $redis and $redis->pubsub object in your app. (Note: pubsub() is an attribute, not a method returning a fresh pubsub)

I think there should not be a standalone ...

That's not going to happen, unless you change https://metacpan.org/pod/Mojo::Pg::PubSub#json as well. The idea is that you should be able to swap between different Mojo::Whatever::PubSub libraries without any cognitive overhead.

Suggested solution:

Since $redis->pubsub is an attribute and it sounds like you are using a "global" $redis helper/object, then I suggest something like this:

use Mojolicious::Lite;
use Mojo::Redis;
my $redis = Mojo::Redis->new;
# Make sure you enable json for all json-channels up front, and tell you developer to never call json() inside an action
$redis->pubsub->json($_) for qw(admin all foo bar);
helper redis => sub { $redis };
Yenya commented 2 years ago

@jhthorsen - thanks for the reply.

In fact, my previous idea was indeed to have a method for switching all channels (present and future) to JSON mode. But then I thought that somebody could want to have raw text access e.g. for debugging purposes. You are right that this can be done using a dedicated $redis object.

OTOH, I cannot switch all the required channels up front to JSON, because I don't even know their names when connecting to Redis (think classroom IDs, which are part of the pubsub channel name, sub-classes/working groups, and so on).

So having a $pubsub->json_all() or ->json_all(1) method would help. What do you think about it? Thanks,

-Yenya

jhthorsen commented 2 years ago

I'm not sure if that's a good idea, but I'll consider it...

jhthorsen commented 2 years ago

What do you think about 16c985bdc5a622b31c5edcc64995d7189e1d713d? Is that something that could be useful to you. I "stole" the idea from https://docs.mojolicious.org/Mojo/Transaction/WebSocket#json

Yenya commented 2 years ago

@jhthorsen - nice! A dedicated on(json) is very close to what I want. I am just not comfortable with the event being emitted also for non-json messages with null data. In such case the handler might want to log the raw message or something, but has only undef and no way how to find the message contents. Maybe the event should be emitted only for json data and ignored otherwise? After all, I tend to read on(json) as call my handler when JSON message arrives.

How does the Mojo::Transaction::Websocket json event work for non-json data?

jhthorsen commented 2 years ago

Mojo::Transaction::Websocket works the same way.

It sounds like you have a very messy “API”, when you allow json and non-json data to be transmitted over the same channels. I’m not sure if I want to optimize for that. Giving the “json” event raw data, when it’s invalid JSON is just not right. (I’ll consider not emitting the event at all though)

Yenya commented 2 years ago

No, this is purely theoretical, and maybe conservative thinking - in my case, I expect to have all-json channels only. But it never hurts to be able to see the lower levels if needed (e.g. some non-Perl client sends a malformed JSON data.

jhthorsen commented 2 years ago

I wonder if the latest commit is too experimental. @Grinnz: Do you have an opinion on 711f186?

Grinnz commented 2 years ago

I feel like it should be a separate method call or some other way to set up, since '*' is currently a valid channel name. Also, it shouldn't do decoding twice on channels that are separately enabled.

jhthorsen commented 2 years ago

I was thinking the same (ref "*" being a valid channel name). How about checking if "*" is in $self->{chans} first? I don't really want another method/attribute, since I think it will make it confusing to use the module.

jhthorsen commented 2 years ago

I'm going to close this now, since I don't think there's a good way to do this. I suggest that you add a helper to your application instead. Something like this:

$app->helper(listen_json => sub ($channel, $cb) { $redis->json($name)->listen($name, $cb) });
Yenya commented 2 years ago

Well, this approach does not solve the original issue - my problem appears when sending to a channel I am not subscribed to. The result then depends on whether somebody else (sharing the same $redis object) set up that channel to the JSON mode or not.

jhthorsen commented 2 years ago

You could add a helper to your application, apply a role to the $pubsub object or add some other proxy object that protects your team from each other.