-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Fixing Presence Conflicts #3320
Conversation
heres the unit test full output:
|
Oh I should also mention don't look too closely at the commit history, this one is a squash and merge (if it gets merged) |
im fixing it, dont review yet I guess |
eh im in over my head and not in a mental state where it makes sense to work on this. bit of a waste of a day but 🤷 i got out of bed |
I've hopefully fixed any potential race conditions, thanks DiamondBurned for the assistance |
I could be seeing things, but this seems to have made presence with maunium bridges work correctly. |
one interesting limitation ive discovered is this means presence set through sync doesnt override explicitly set presence through the api. the only time this matters is for some reason element ios explicitly sets presence to offline when you swipe out of the app to the homescreen. im marking this down as a client bug more than anything because why the hell would it do that |
I'm not a Dendrite user myself, but I'm very interested in seeing some progress on this, as my homeserver is getting presence spam from a few Dendrite instances from time to time. I did not review your code (I don't usually write or even read go), but thank you for trying and keeping this up to date with the main branch. Have you been running this since February ? Is it going well ? If a maintainer reads this : how can I help with making this move forward ? PS : Disabling presence on my side does not seem to prevent receiving a deluge of presence updates. |
Yes I have been running this patch since I made it, and till has recently
for a couple weeks been running it too. I’m not sure what we can do to move
it forward.
…On Mon, Jul 15, 2024 at 10:06 PM Pierre Couy ***@***.***> wrote:
I'm not a Dendrite user myself, but I'm very interested in seeing some
progress on this, as my homeserver is getting presence spam from a few
Dendrite instances from time to time.
I did not review your code (I don't usually write or even read go), but
thank you for trying and keeping this up to date with the main branch. Have
you been running this since February ? Is it going well ?
If a maintainer reads this : how can I help with making this move forward ?
PS : Disabling presence on my side does not seem to prevent receiving a
deluge of presence updates.
—
Reply to this email directly, view it on GitHub
<#3320 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWNJYMG6IKFGRIINF56QLW3ZMTIAXAVCNFSM6AAAAABC3LURX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZQGI3TQMZUGE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3320 +/- ##
=======================================
Coverage 68.24% 68.24%
=======================================
Files 513 513
Lines 46865 46888 +23
=======================================
+ Hits 31984 32000 +16
- Misses 10892 10899 +7
Partials 3989 3989
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This sounds useful for me, hopefully the maintainers can spend some of their time on reviewing this soon |
I'm basically happy with it, I just want CI to be green before merging, this needs a few other PRs to be merged before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3397 is going to fix the media tests, so approving this one.
Thanks a lot! And sorry for the delay.
This is meant to cache client presence for a moment so that it doesn't oscillate.
Currently Dendrite just federates out whatever presence it gets from the sync loop, which means if theres any clients attempting to sync without setting the user online, and there is an online client, it will just flip back and forth each time one of the clients polls /sync.
This pull request essentially stores in a map when the client last set online ideally to allow the online client to sync again and set an online presence before setting idle or offline.
I am not great at programming nor am I familiar with this codebase so if this pr is just shitwater feel free to discard, just trying to fix an issue that severely bothers me. If it is easier you can also steal the code and write it in yourself. I ran the linter, not sure that it did anything, the vscode go extension seems to format and lint anyways.
I tried to run unit tests but I have no idea any of this thing. it errors on
TestRequestPool_updatePresence/same_presence_is_not_published_dummy2 (10m0s)
which I think making this change broke. I am unsure how to comply, if y'all point me in the right direction ill try to fix it. I have tested it with all the situations I can think of on my personal instance pain.agency, and this seems to stand up under all the previously bugged situations.My go also decided to update a bunch of the dependencies, I hate git and github and have no idea how to fix that, it was not intentional.i just overwrote them with the ones from the main repo and committed it, seems to have done what was needed.Pull Request Checklist
Signed-off-by:
Joseph Winkie <[email protected]>