No reviewers
Labels
No labels
Android
CS::needs customer feedback
CS::needs follow up
CS::needs on prem installation
CS::waiting
Chrome
Design:: Ready
Design:: in progress
Design::UX
E2EE
Edge
Firefox
GDPR
Iteration 13 IM
Linux
MacOS
Need::Discussion
Need::Steps to reproduce
Need::Upstream fix
Needs:: Planning
Needs::Dev-Team
Needs::More information
Needs::Priority
Needs::Product
Needs::Refinement
Needs::Severity
Priority::1-Critical
Priority::2-Max
Priority::3-Impending
Priority::4-High
Priority::5-Medium
Priority::6-Low
Priority::7-None
Progress::Backlog
Progress::Review
Progress::Started
Progress::Testing
Progress::Triage
Progress::Waiting
Reporter::Sentry
Safari
Target::Community
Target::Customer
Target::Internal
Target::PoC
Target::Security
Team:Customer-Success
Team:Design
Team:Infrastructure
Team:Instant-Messaging
Team:Product
Team:Workflows
Type::Bug
Type::Design
Type::Documentation
Type::Feature
Type::Improvement
Type::Support
Type::Tests
Windows
blocked
blocked-by-spec
cla-signed
conduit
contribution::advanced
contribution::easy
contribution::help needed
from::review
iOS
p::ti-tenant
performance
product::triage
proposal
refactor
release-blocker
s: dart_openapi_codegen
s::Famedly-Patient
s::Org-Directory
s::Passport-Generator
s::Requeuest
s:CRM
s:Famedly-App
s:Famedly-Web
s:Fhiroxide
s:Fhiroxide-cli
s:Fhiroxide-client
s:Fhirs
s:Hedwig
s:LISA
s:Matrix-Dart-SDK
s:Role-Manager
s:Synapse
s:User-Directory
s:WFS-Matrix
s:Workflow Engine
s:dtls
s:famedly-error
s:fcm-shared-isolate
s:matrix-api-lite
s:multiple-tab-detector
s:native-imaging
severity::1
severity::2
severity::3
severity::4
technical-debt
voip
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Matthias/conduit#901
Loading…
Reference in a new issue
No description provided.
Delete branch "presence"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR adds presence functionality (by default disabled), including configurable intervals and idle/offline timeouts.
The only caveat here is that presence events only get batched together when PDUs are sent, somewhat reducing the reliability of presence over federation. This will however require addition of a generalized functionality for reliably sending EDUs that is not present at the moment (AFAIK).
Closes #57.
assigned to @M0dEx
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 2 commits
Compare with previous version
added 1 commit
Compare with previous version
This might have to be re-done with timers, as it currently takes a relatively long time (150 ms) even for a relatively low-load server (joined in rooms with ~50k people total).
changed this line in version 8 of the diff
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
changed the description
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
The performance looks ok with timers.
I have been running it for a couple of days now and presence seems to be working great, with no noticeable slowdown (or higher CPU/memory consumption).
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
If we clear presence data on startup, it would probably make sense to make this a memory-only map.
With a HashMap, we would lose access to useful methods like
scan_prefix
, though.I do not like this part, but I could not think of anything better.
Ideas on how to prettify this/do this better are welcome.
Document these options in the example config.
I do not like this, but it is much faster than
serde_json
.If we made presence a memory-only map, we would not need this.
I am worried about the length and readability of this method.
Maybe split it up?
This is duplicated across
ping_presence
andset_presence
, maybe put it into a separate function/method/macro?This is duplicated across multiple methods in this file, maybe make it a function/method/macro?
new = new.or(old) should work:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3596441037b0e7234aa440e3e261e2ba
added 1 commit
Compare with previous version
added 1 commit
Compare with previous version
I think the values should be more nuanced:
allow_incoming_presence (for federation)
allow_outgoing_presence (for federation)
allow_local_presence (without federation)
Maybe make sure the time is not in the future? Or do it inside the set_presence fn
Maybe *_secs would be a bit more intuitive?
Please use to_be_bytes
That's a good idea, BTreeMap should work
This is fine.
At some point I want to improve the key value database by introducing another level of abstraction, but for now it's fine
Use bad_database instead of BadDatabase so we get more logs
I think this would be easier to read as
size_of::<u8>() + 1 .. size_of::<u8>() + 1 + size_of::<u64>()
, maybe inline it directly where you use it.We have utils::u64_from_bytes to make this slightly shorter
What's the reason for the first condition?
What if it's None? You could write
if presence.map(|p| p.state) != Some(new_state)
Can you rename this to something like
start_handler
Here's my review so far, I still have to look at the timer code, but the rest is really well done, thanks!
BTreeMap
withrange
would work great, the problem is with returning an iterator inpresence_since
.The
BTreeMap
has to be read-locked and the read-lock does not live long enough.Do you have any ideas on how to be able to return an iterator in
presence_since
?Or am I just trying to prematurely optimize and it would be just fine to
collect
and return aVec
instead?changed this line in version 18 of the diff
changed this line in version 18 of the diff
changed this line in version 18 of the diff
changed this line in version 18 of the diff
changed this line in version 18 of the diff
changed this line in version 18 of the diff
changed this line in version 18 of the diff
changed this line in version 18 of the diff
added 1 commit
Compare with previous version
added 27 commits
next
Compare with previous version
added 50 commits
next
Compare with previous version
changed this line in version 21 of the diff
added 1 commit
Compare with previous version
last_active_ago
will never be in the future, as it is an unsigned integer.If it is
None
, then we do not care about it.added 3 commits
next
Compare with previous version
added 10 commits
next
Compare with previous version
added 89 commits
next
Compare with previous version
changed this line in version 26 of the diff
added 1 commit
Compare with previous version
Does it add any clarity? Other intervals in the config do not even have the
_s
suffix.added 2 commits
Compare with previous version
added 2 commits
Compare with previous version
Fixed according to spec:
The current status of this MR:
AFAIK, all required functionality from the spec is implemented.
Presence data is stored in the database (someone more familiar with Conduit might devise a more efficient way to save the data, regarding the response times), permanently.
I've been running on this branch for a couple of months and have not experienced any performance degradation or significantly increased resource consumption. I'd love for someone to try this with a bigger server, with a lot of local users.
All threads with initial comments have been resolved with the issues either fixed or refactored.
added 9 commits
next
Compare with previous version
requested review from @timokoesters
mentioned in issue #414
added 4 commits
next
Compare with previous version
Nit:
changed this line in version 31 of the diff
added 2 commits
Compare with previous version
added 1 commit
Compare with previous version
Not really familiar with Conduit, but should
ping_presence
errors be propagated to the caller or simply dropped? Does it even make a difference in practice?Besides the unfortunate
clone
s, is this right? It seems to keep the old values unless they'reNone
, soSome("foo")
andSome("bar")
yieldsSome("foo")
.I could replace the
clone()
withtake()
, that should be without any copying or allocations AFAIK.Yeah, but it would have to be
curr_content.status_msg = new_content.status_msg.or_else(|| curr_content.status_msg.take());
(or simplyor
, whatever).You're doing it in the wrong direction right now.
I see, you're right.
changed this line in version 33 of the diff
added 1 commit
Compare with previous version
mentioned in merge request !565
added 8 commits
next
ed42bcce
- feat(presence): implement presence functionality100c8b39
- feat(presence): add granular allow configuration76a90981
- style(presence): use flat_map instead of matching Results in filter8690552b
- fix(sync): correctly update presence propertiesCompare with previous version
mentioned in issue #412
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.