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

Clarify retry notification code #15226

Closed
deepbluev7 opened this issue Mar 8, 2023 · 1 comment · Fixed by #16506
Closed

Clarify retry notification code #15226

deepbluev7 opened this issue Mar 8, 2023 · 1 comment · Fixed by #16506
Assignees
Labels
A-Federation T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. z-WTF Causing the user to exclaim! These issues are high impact and low effort.

Comments

@deepbluev7
Copy link
Contributor

deepbluev7 commented Mar 8, 2023

When looking into tweaking the backoff behaviour on my server, I came across this code:

if self.notifier:
# Inform the relevant places that the remote server is back up.
self.notifier.notify_remote_server_up(self.destination)
if self.replication_client:
# If we're on a worker we try and inform master about this. The
# replication client doesn't hook into the notifier to avoid
# infinite loops where we send a `REMOTE_SERVER_UP` command to
# master, which then echoes it back to us which in turn pokes
# the notifier.
self.replication_client.send_remote_server_up(self.destination)

Specifically the code seems to notify every worker (and master), that a particular destination is up, even when it decided a destination is down and in my opinion should only be writing the new backoff interval to the database. This would be fairly easy to fix by just exiting that function if the retry interval is 0, however I am not familiar enough with Synapse internals to be sure that this doesn't break other stuff. It might also be the case, that this code does something completely different, than I assumed. In any case, I would appreciate if someone could clarify what that code is doing and possibly add a comment, why notifying the other workers is the right thing to do. I would assume this would lead to the backoff incrementing to quickly, but maybe it isn't possible to hit this case.

Originally I asked the question in #synapse and it was suggested to just file an issue to not forget about it: https://matrix.to/#/%23synapse-dev%3Amatrix.org/%24-5RtnWrJbfFjGIH8vKkpgOE7UVt2M9aNK6PThkvMsiI

Additionally we discovered, where that code was added and that someone else asked about this code as well at the time: #12500 (comment)

I hope this issue makes sense. I don't really know what to do about the code, I am just documenting, that multiple people think it is weird. :)

@DMRobertson DMRobertson added A-Federation T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. z-WTF Causing the user to exclaim! These issues are high impact and low effort. labels Mar 13, 2023
@clokep
Copy link
Member

clokep commented Oct 16, 2023

See #16506.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Federation T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. z-WTF Causing the user to exclaim! These issues are high impact and low effort.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants