matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.79k stars 2.13k forks source link

Synchrotrons always wake up sync streams for presence updates #8955

Open erikjohnston opened 3 years ago

erikjohnston commented 3 years ago

When calculating presence changes we decide whether we should actively wake up sync streams, or if they should just be bundled up the next time we send a response for another reason. We do not appear to propagate whether to wake up sync streams across replication, and we appear to always notify for presence updates:

https://github.com/matrix-org/synapse/blob/6d02eb22dfde9551c515acaf73503e2500e00eaf/synapse/app/generic_worker.py#L381-L390

It's unclear how much this will help.

(Note that we batch up presence updates that we don't notify for and persist/replicate them once a minute).

realtyem commented 1 year ago

I think I found a great spot to poke this to model this behavior. However, I also think I need a little more information for my testing branch before I offer a fix.

(I also need to make sure my assumption that the notifier from presence is called with a on_new_event() and the sync handler picks up with wait_for_events(), if this isn't right please let me know.)

The code for should_notify()... https://github.com/matrix-org/synapse/blob/78cfa55dad911e667b5a9b613e232eb72410382f/synapse/handlers/presence.py#L1502-L1535

Which condition doesn't pass muster? Quick summary of what it looks like: Notify for:

Do nothing for:

Keeping in mind, some unit tests may have to be adjusted based on how aggressively this changes. Complement tests should not, as they should be 'correct' behavior based on spec.

For Monolith deployments: It currently appears that the notifier is poked inside _persist_and_notify() but the decision to actually send that notification is inside should_notify(). Both of these are inside _update_states().

For Worker deployments: It currently appears that the notifier is poked inside notify_from_replication() however before that function is called is a should_notify() inside process_replication_rows().

So, it looks like should_notify() is the best place to group these together for like decisions.

We do not appear to propagate whether to wake up sync streams across replication, and we appear to always notify for presence updates

For this part, I think this may have been done?