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

Reduce our reliance on an up-to-date joined rooms cache #16

Merged
merged 6 commits into from
Jun 5, 2023

Conversation

tadzik
Copy link

@tadzik tadzik commented May 5, 2023

Opening this mostly to start a discussion and see how best to proceed with this.

The current joined rooms API is not very safe by default. It caches joined rooms to allegedly to speed up ensureRoomJoined() (comment), but also utilizes it in other ways, like ignoring some room events in crypto handling if it doesn't think the user is in a certain room – making the invalid cache a potential source of bugs.

Different API calls like Intent.getJoinedRooms() (here) warn that joining rooms out of band may cause the Intent API to return incorrect results. To help with that, it exposes a public API to refresh its own cache (Intent.refreshJoinedRooms() here). The necessity to call this at the right times comes with its own problems, like Appservice membership event handling possibly failing due to triggering Matrix API calls when it doesn't need to. There's a few places where we refresh the entire joined room list rather than applying the membership changes that we know have just happened, which I fixed in this PR.

I don't think the worry about externally induced joins not being noticed by the bot-sdk are entirely warranted (we're bound to get the room joined AS event first after all, I don't think there's any way we can miss it even if we didn't trigger it), but this whole mechanism seems like an optimization both premature (joining an already joined room excessively is not an error, and I imagine is pretty cheap) and happening at the wrong layer. The user of the SDK can make a more informed decision if, in their scope, it makes sense to cache such things, or if it's safe to use at all.


This PR slightly reduces our reliance of the joined rooms cache – never making decisions like "should we drop this event" based on its contents, and making public APIs like getJoinedRooms() return freshest possible information anyway. refreshJoinedRooms() should probably be deprecated here, but I do wonder if a proper solution would be just dropping this whole caching from Intent and leaving it up to the SDK user to decide whether they need something like that or not. It is, after all, trivial to implement yourself – I've done just that in my projects, not knowing that ensureRoomJoined() does the caching by itself.

@AndrewFerr AndrewFerr marked this pull request as ready for review May 26, 2023 12:28
src/appservice/Intent.ts Show resolved Hide resolved
src/appservice/Intent.ts Show resolved Hide resolved
src/appservice/Intent.ts Show resolved Hide resolved
@AndrewFerr
Copy link
Member

Also, can this be merged against element-main instead of main? The former is the true default branch of this fork, and the latter is upstream's.

With that said, having two default branches in this repo is admittedly confusing. How about we remove this repo's main, and leave element-main named as-is to disambiguate it from upstream's main?

@tadzik tadzik changed the base branch from main to element-main June 5, 2023 09:01
@tadzik tadzik requested a review from a team as a code owner June 5, 2023 09:01
@AndrewFerr AndrewFerr merged commit 9df413e into element-main Jun 5, 2023
@AndrewFerr AndrewFerr deleted the tadzik/safer-joined-rooms-caching branch June 29, 2023 14:01
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

Successfully merging this pull request may close these issues.

2 participants