Skip to content

[Feature] Automatically fetch full members list for rooms when needed #144

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

Closed
jsparber opened this issue Feb 12, 2021 · 16 comments
Closed

Comments

@jsparber
Copy link
Contributor

matrix_sdk_base::Room has methods to get the members of a room, but they only return members stored in the StateStore, but the stored list may be incomplete if lazy-loading is used.
The client developer can request the full list from the matrix server if they like, but I think we should do the request for them automatically.

I would suggest adding an boolean argument to Room::joined_user_ids(), Room::active_members(), Room.get_member() , Room::joined_members() for letting the user decide whether to automatically request the full member list, when not already stored, before return any result or return without making a request to the Matrix server.

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

This requires an Executor abstraction so we can send out requests in parallel.

Basically the following + a way to get a generic output of the task:

pub trait Executor {
     fn spawn(future: F) -> Task;
}

Possibly also a spawn_blocking() method so crypto operations run on the blocking threadpool.

@jsparber
Copy link
Contributor Author

@poljar i guess we will also need the same Executor abstraction for #143

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

Do we? I don't see why, an executor is only needed to run things in parallel, since we support WASM we can't just use tokio::spawn().

I don't think there's parallelism needed in #143, unless I'm misunderstanding something.

@jsparber
Copy link
Contributor Author

@poljar Maybe i wasn't clear in #143 about this part, but I think the timeline should also be automatically loaded if the request can't be fulfilled with the information in the StateStore.

I actually assumed that the Room::joined_members() (and others) already automatically requested the full member list if it wasn't available, therefore i filled this issue now.

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

I understand that, but why do you need to spawn() a task? If the member list isn't available, do a request and then return the list, else just return the list.

The methods are already async, where does an executor come into place?

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

Ah right, I noted this issue to be a general "load the members from the server", not a "load the members when the user requests them" issue. We can add this without the executor, but I would like to load the members after a sync response so the members are available in the general case.

So after a sync, check rooms that need the members to be fetched, and fire out all the requests at once +/- a delay.

Sorry for being confused.

@jsparber
Copy link
Contributor Author

@poljar I was about to write that i'm confused now :)

@jsparber
Copy link
Contributor Author

Ah right, I noted this issue to be a general "load the members from the server", not a "load the members when the user requests them" issue. We can add this without the executor, but I would like to load the members after a sync response so the members are available in the general case.

If we load the the members immediately (or with a delay) after a sync response, i think we reduce the usefulness of lazy-loading the room members and could directly include them in the sync request. To make full use of lazy-loading we should load the members only when they are actually needed or maybe when nothing more important needs to be loaded.

@jsparber
Copy link
Contributor Author

Since matrix_sdk_base can't make IO requests (according to the Readme.md) I'm not sure how we can make the needed requests to the Matrix server.

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

If we load the the members immediately (or with a delay) after a sync response, i think we reduce the usefulness of lazy-loading the room members and could directly include them in the sync request. To make full use of lazy-loading we should load the members only when they are actually needed or maybe when nothing more important needs to be loaded.

The usefulness of lazy loading is to lower the amount of data a client needs to handle on an initial sync (ideally it would do so for all sync but currently synapse doesn't do any lazy loading on subsequent syncs), an initial sync lets you display the room list.

Spawning tasks after the initial sync is done to fetch room members won't block anything else and when the client wants to display a room fully, including the member list, the members will likely already be loaded thus reducing the delay we would otherwise have to display a room.

@jsparber
Copy link
Contributor Author

The usefulness of lazy loading is to lower the amount of data a client needs to handle on an initial sync (ideally it would do so for all sync but currently synapse doesn't do any lazy loading on subsequent syncs), an initial sync lets you display the room list.

Spawning tasks after the initial sync is done to fetch room members won't block anything else and when the client wants to display a room fully, including the member list, the members will likely already be loaded thus reducing the delay we would otherwise have to display a room.

That still leaves the possibility that client receives an incomplete members list (even when unlikely) which I really would like to avoid.

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

The usefulness of lazy loading is to lower the amount of data a client needs to handle on an initial sync (ideally it would do so for all sync but currently synapse doesn't do any lazy loading on subsequent syncs), an initial sync lets you display the room list.

Spawning tasks after the initial sync is done to fetch room members won't block anything else and when the client wants to display a room fully, including the member list, the members will likely already be loaded thus reducing the delay we would otherwise have to display a room.

That still leaves the possibility that client receives an incomplete members list (even when unlikely) which I really would like to avoid.

What do you mean? When would that occur?

@jsparber
Copy link
Contributor Author

jsparber commented Feb 12, 2021

What do you mean? When would that occur?

When the client needs the member list immediately after the "initial" sync. to me it sounds like a race condition.

@poljar
Copy link
Contributor

poljar commented Feb 12, 2021

Well the method would need to be locked per room, so you can have only one of them in flight and the get_members() method would await the lock and completion of the request, the only difference is that the request was sent out before the user called get_members().

@jsparber
Copy link
Contributor Author

Well the method would need to be locked per room, so you can have only one of them in flight and the get_members() method would await the lock and completion of the request, the only difference is that the request was sent out before the user called get_members().

works for me :)

@jsparber
Copy link
Contributor Author

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

2 participants