Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

_count_known_servers delay #10030

Closed
heftig opened this issue May 20, 2021 · 4 comments · Fixed by #10195
Closed

_count_known_servers delay #10030

heftig opened this issue May 20, 2021 · 4 comments · Fixed by #10195
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@heftig
Copy link

heftig commented May 20, 2021

synapse/synapse/storage/databases/main/roommember.py

Lines 88 to 95 in 1c6a190

self.hs.get_clock().looping_call(
self._count_known_servers,
60 * 1000,
)
self.hs.get_clock().call_later(
1000,
self._count_known_servers,
)

Is this really supposed to be 1000 seconds? looping_call uses milliseconds and call_later uses seconds.
@callahad
Copy link
Contributor

FWIW, this was introduced almost three years ago in 55d5b3a as part of #5981

@anoadragon453
Copy link
Member

@heftig You're correct, and that definitely shouldn't be 1,000 seconds 🙂

Would you like to PR a fix? Otherwise we can take care of it.

@anoadragon453 anoadragon453 added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels May 26, 2021
@heftig
Copy link
Author

heftig commented May 26, 2021

Would you like to PR a fix? Otherwise we can take care of it.

I would if I just had to make a commit with the fix, but your guidelines require a lot more work I don't have time for.

@anoadragon453 anoadragon453 mentioned this issue Jun 17, 2021
Merged
@anoadragon453
Copy link
Member

Understandable, thanks for bringing it to our attention regardless 🙂

I've opened a PR to fix this here: #10195

anoadragon453 added a commit that referenced this issue Jun 17, 2021
@anoadragon453
Fix incorrect time magnitude on delayed call (#10195)
6f1a28d 
Fixes #10030.

We were expecting milliseconds where we should have provided a value in seconds.

The impact of this bug isn't too bad. The code is intended to count the number of remote servers that the homeserver can see and report that as a metric. This metric is supposed to run initially 1 second after server startup, and every 60s as well. Instead, it ran 1,000 seconds after server startup, and every 60s after startup.

This fix allows for the correct metrics to be collected immediately, as well as preventing a random collection 1,000s in the future after startup.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix incorrect time magnitude on delayed call matrix-org/synapse
3 participants
@callahad @heftig @anoadragon453