fix: incorrect local presence #660
Conversation
HolgerHuo
force-pushed
the
main
branch
2 times, most recently
from
e4ebe26 to
fd43c2d
Compare
HolgerHuo
force-pushed
the
main
branch
2 times, most recently
from
d724871 to
c3ad7c3
Compare
src/service/presence/mod.rs
Outdated
| .services | ||
| .users | ||
| .list_local_users() | ||
| .map(UserId::to_owned) |
There was a problem hiding this comment.
I think this can be optimised to avoid needing to call to_owned, but might be wrong. I'll try messing with it and see myself.
There was a problem hiding this comment.
I think this can be optimised to avoid needing to call
to_owned, but might be wrong. I'll try messing with it and see myself.
I think reference should do the job. I just borrowed this logic from another part of the codebase.
There was a problem hiding this comment.
Btw, I just remembered that we should also unset statuses for all users, including those federated. Maybe I could call db for fetching users directly?
There was a problem hiding this comment.
The startup performance impact will be very bad if we include federated users without further significant database optimisations, making this use pure references only instead of copying/to_owned, and optimising this code more.
I think it's best that is left out of this PR for now.
There was a problem hiding this comment.
I think I can optimise enough to allow clearing remote presences on startup for remote users without startup performance taking a toll.
There was a problem hiding this comment.
I just realized that the best solution may just be to store presence state in memory, and not persisting it to the database, since they will be removed on startup. Will federated presence information taking up too much memory?
There was a problem hiding this comment.
Yes it will. Presence is designed to track tens of thousands of remotes users. Presence contents can be large such as status messages. Storing them in memory is definitely not a valid option.
There was a problem hiding this comment.
I think this can be optimised to avoid needing to call
to_owned, but might be wrong. I'll try messing with it and see myself
you have to call to_owned() here before collect()
|
I accidentally force pushed your PR and now I can't edit it to re-open it back it :( If you'd like to re-open it, use this branch: https://github.com/girlbossceo/conduwuit/tree/strawberry/pr-660-presence-fix I think the performance impact of clearing all users presence, even remote/federated, is fine after a bit of optimising. So we'll just do that. Subsequent restarts are faster anyways. |
|
I've committed my branch with your commits to main branch, effectively merging your PR. |
Ok! I was on a trip so I didn't reply in time. Thanks for creating such wonderful project. |
This pr fixed unstable local presence state.
3 possible optimizations:
last_active_ago0 when user is online, this is done on every client side route in this pr. (src/api/client/sync/v3.rsandsrc/api/client/presence.rs). Maybe a wrapper could make the conversion more efficient.For most accurate last online information,I hardcoded this to 1mrefresh_timeoutshould be disabled entirely, making last_online_timestamp everytime the client sends ping.