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

Featurerequest: Get a list of all channels in a group #174

Closed
ostcar opened this issue May 29, 2016 · 17 comments
Closed

Featurerequest: Get a list of all channels in a group #174

ostcar opened this issue May 29, 2016 · 17 comments

Comments

@ostcar
Copy link
Contributor

ostcar commented May 29, 2016

I am new to channels, so maybe I oversea something and what I am trying to do get be done in another way. In this case please tell me how.

I am trying to send an object to all clients that are connected via websocket, but for each client the object has to be manipulated individually. Take the example from the documentation:

http://channels.readthedocs.io/en/latest/concepts.html#groups
If in this liveblog the users have different permissions, then some client should not see the instance and other should see only some attributes from it. This would looks like this:

redis_conn = redis.Redis("localhost", 6379)

@receiver(post_save, sender=BlogUpdate)
def send_update(sender, instance, **kwargs):
    # Loop through all response channels and send the update
    for reply_channel in redis_conn.smembers("readers"):
        if reply_channel.user.has_perm('can_see_instance'):
            Channel(reply_channel).send(
                id=instance.id,
                content=instance.content,
            )
        else:
            Channel(reply_channel).send({'text': 'Something changed, but you can not see it'})

To do this with groups there needs to be a possibility to get all current channels out of them. This could look like this:

@receiver(post_save, sender=BlogUpdate)
def send_update(sender, instance, **kwargs):
    for channel in Group("liveblog").get_channels():
        if channel.user.has_perm('can_see_instance'):
            channel.send(
                id=instance.id,
                content=instance.content,
            )
        else:
              Channel(reply_channel).send({'text': 'Something changed, but you can not see it'})

The implementation is quite easy:
channels.channel.Group gets a new method:

    def get_channels(self):
        return [Channel(channel) for channel in self.channel_layer.group_channels(self.name)]

All Channel layers get a new method. For example the inmeory layer:

    def group_channels(self, group):
        assert isinstance(group, six.text_type), "%s is not text" % group
        for channel in self._groups.get(group, set()):
            yield channel

If you like this idea, then I create pull requests for it.

@andrewgodwin
Copy link
Member

I've actually deliberately avoided this idea so far, as in order to make a group send system scalable, it's a lot easier to not have to be able to return all groups given the request (for example, the Redis backend is slowly moving towards a place where group send is handled entirely in Lua per-shard; to get a list, you'd have to query all shards, get the result, set-union them, and pass them back, all to result in less efficient code).

With the problem you're describing, could I suggest having one Group per permission level? That would let you send the content differently per client's permission level but still be pretty efficient.

@ostcar
Copy link
Contributor Author

ostcar commented May 29, 2016

One Group per permission level is hard. I have more then 20 permissions. To create groups for all possible permission permutations is not possible. Also there are some requests, where the the output is different on user basis. For example a user has not the permission to see unpublished objects but can see it, if he created the object himself. In this case two users with the same permissions can see different objects. Therefore I need a Group per user.

Is there a way to get all Groups which have at least one channel in them and there name matches a regular expression? Then I could create a group per user (how could have opened more then one tab) and get all groups I have to send the object to. For example:

@channel_session_user_from_http
def ws_add(message):
    # Add them to the right group
    Group("user-%s" % message.user.username[0]).add(message.reply_channel)

for group in Group.search(r"^user-"):
    group.send(...)

I think me usecase is not so unusable. It would be really nice if channels had a solution for it.

@andrewgodwin
Copy link
Member

No, in general Channels tries to avoid making the backends expose iterators over their datastructures because of concerns of performance and incorrect use (for example, the list of channels in a group in the Redis backend often contains old or expired entries, the send code cleans them up as it goes).

That said, I think enough people have asked for this functionality, and there's enough other use cases (e.g. per-connection signatures) that its probably worth adding, with a big caveat that it will never be as fast as Group.send(). I'll look into exposing it in the next couple days.

@andrewgodwin andrewgodwin reopened this May 29, 2016
@andrewgodwin
Copy link
Member

I need to clarify something else though - when you iterate over a group's channels, all you'll get are channel names; no identifying information about what that channel means or who the user is; that's all only available from a message context.

Given that, I don't think they'd be useful for what you want them for? You'd need an external store (in the database) of who is doing what anyway, and at that point, do you need the group list endpoint?

@ostcar
Copy link
Contributor Author

ostcar commented May 29, 2016

I already have a working patch for a project that used tornado before, but should use channels in the future. I modified channels and asgiref like I wrote in the first post of this issue. This works perfectly fine.

https://github.com/OpenSlides/OpenSlides/pull/2170/files#diff-8bfadcb4bd744b1e4b747594f58bd16dR43

In my channel patch I use [Channel(channel) for channel in self.channel_layer.group_channels(self.name) and therefore get Channel objects and not strings. This Channel objects contain the user via the session that was added via @channel_session_user_from_http.

I works perfectly. If you like, I can create pull requests for channels, asgiref, asgi_redis and asgi_ipc. This are only some lines per repo.

EDIT: session_for_reply_channel() only need the channel name. So for that to work it does not matter if channel names or channel objects are returned.

@andrewgodwin
Copy link
Member

Yeah, the Channel() objects do not contain a session or user when you inflate them, you have to use session_for_reply_channel(), but I would be very concerned about running that inside a for loop with potentially hundreds of entries; you're going to absolutely hammer your session backend doing a fetch for every reply channel for every event sent.

I still think doing a group per permission level or per username is the best way to go; the problem you then need to solve is which users are logged in and should get a message sent to their group, but that's the sort of thing you can track with a database table + query in a very efficient way.

@andrewgodwin
Copy link
Member

To elaborate: This is why I've resisted adding the list channels thing in the past. There's nothing you can do with that list of names efficiently that isn't send to them or check the size of the set, and if the only value is in the second one, it might be better to add an endpoint that returns group size instead.

@ostcar
Copy link
Contributor Author

ostcar commented May 30, 2016

I found a way around this current limitation(?) of channels by getting the needed information out of the database.

I used

@channel_session_user_from_http
def ws_add(message):
    Group('user-{}'.format(message.user.id)).add(message.reply_channel)

To add each user to its own group. If a user has opened two tabs, than he is two times in the same group. There is only one Group for the anonymous user with the name 'user-None'.

Then I can get a list for all active users.

def get_active_users():
    uid_set = set()
    for session in Session.objects.filter(expire_date__gte=timezone.now()):
        data = session.get_decoded()
        uid_set.add(data.get('_auth_user_id', None))

    users = get_user_model().objects.filter(id__in=uid_set)
    if None in uid_set:
        users = itertools.chain(users, [AnonymousUser()])
    return users

And iterate over all active websocket-connections:

for user in get_active_users():
    channel = Group('user-{}'.format(user.id))
    ...

The limitation is, that only the db-session backend can be used. The reason is, that django session api has no function to get a list of all active sessions. Probably for the same reason you don't like to get a list of all channels in a group.

With this limitation this solution could be optimized by creating an individual session backend, that saved the user-id directly into the database, so it can be retrieved without decoding all sessions.

In conclusion I don't need this requested feature anymore. But I still would be happy if you implement it, so my code can be session backend independent.

@andrewgodwin
Copy link
Member

Yeah, the problem is generally that providing a list-all-things API is a different kind of cost to a get-a-single-thing API (see how expensive Redis' KEYS command is compared to GET, for example), and so I don't want to have to make every ASGI backend have to be able to do it unless there's a good reason.

In this case, I think you could go one step further and just have a last_seen column in the User table, and just say that active_users is anyone seen in the last 5 minutes. If there was a channel list API, I don't see how it would result in a better solution in this example.

I guess what I need to convince me is a couple of examples where a channel list API is obviously a massive improvement, rather than the basis for a solution that is actually better done in the database/etc.

@ostcar
Copy link
Contributor Author

ostcar commented May 30, 2016

Your proposal could look like this:

@channel_session_user_from_http
def ws_add(message):
    if message.user.is_authenticated():
        message.user.objects.update(last_seen=timezone.now())
    Group('user-{}'.format(message.user.id)).add(message.reply_channel)

And at the other place

for user in get_user_model().objects.filter(...):
    channel = Group('user-{}'.format(user.id))

The disadvantage of this is, that there as to be a database write request on each connection and a database read request for any object, that has to be sent to the client. So I have to save and receive an redundant information, that is at any rate in the memory of the connected asgi channel_layer.

The performance issue with group_list in the redis_backend could be fixed, if you create a redis key with all current websocket connections.

@andrewgodwin
Copy link
Member

Right, my way would need a single database read per event sent, while your proposed solution of iterating through the reply channels and using channel_session would be N session reads per event sent, which is likely a lot worse.

(The Redis backend already has channels stored as sets, it doesn't have the performance issue I describe; I'm talking about the general case)

@approxit
Copy link

+1 from me.

Right now adding and removing reply_channels in groups are only usable in routing callbacks - only there we have reply_channel. We can only play with channel that we currently process.

Without access to channel list in group there is no way to manage other reply_channels. Why we would take care of other reply_channels? Because we want to add and remove things on the fly, like giving some user access to IRC-like chat room, kicking out that user or "subscribing" someone for updates in newly created Item.

Certainly there are tons of use cases.

@michaelkuty
Copy link
Contributor

My way for doing this #245 (comment)

@andrewgodwin
Copy link
Member

Adding/removing users from chatrooms is an excellent point, and might swing me in the balance of saying this is needed after all (and as a list of channels, not just a number).

Of course, all the operations would be adding/removing a Group from another Group, and this is something I can implement at a higher level on the Channels Group object, but I'm starting to think it's worth having the raw listing endpoint too. Let me think about it today.

@andrewgodwin
Copy link
Member

All ASGI backends I maintain now have a group_channels(group) method that returns an iterable of all channel names at that point in time, which I believe fills this request!

@sebhaase
Copy link

could you please add a reference to the corresponding changeset / commit for
now have a group_channels(group) method
!?
Also I wonder if you could give a vague number on how many channels in a group would start making this kind of iteration slow. Reading the comments I often get the feeling that django-channels is designed to handle many hundreds / thousands (more?) clients --- while others have use cases of (less than) a dozen in group...

@WhitePako
Copy link

Just a note for newcomers, like me: This feature is no longer available (It was removed with 836f6be from the main channels repo, also It was removed from the channels_redis repo with django/channels_redis@ff8c8e6 )
However, In my use-case I wanted to use this feature to avoid unnecessary (group based) computation and DB usage when sending to groups that are possibly empty (hence the group_channels check), but then: the consumer is not called, if the group is empty anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants