Fix/presence user syncing nonblocking#19885
Conversation
…sync user_syncing() was awaiting set_state() before returning the context manager, causing every /sync request to block on presence I/O (DB read/write on the presence writer, or an HTTP RPC from workers). Move the counter increment and mark_as_coming_online ahead of the call, then fire set_state via run_in_background consistent with how _end() is already handled.
…sync user_syncing() was awaiting set_state() before returning the context manager, causing every /sync request to block on presence I/O (DB read/write on the presence writer, or an HTTP RPC from workers). Move the counter increment and mark_as_coming_online ahead of the call, then fire set_state via run_in_background consistent with how _end() is already handled.
…sync user_syncing() was awaiting set_state() before returning the context manager, causing every /sync request to block on presence I/O (DB read/write on the presence writer, or an HTTP RPC from workers). Move the counter increment and mark_as_coming_online ahead of the call, then fire set_state via run_in_background consistent with how _end() is already handled.
There was a problem hiding this comment.
Pull request overview
This PR prevents /sync from blocking on presence state updates by moving the set_state call in user_syncing() to a background task, reducing sync-start latency when presence I/O is slow (DB/RPC).
Changes:
- Stop awaiting
set_state()inuser_syncing()and instead schedule it viarun_in_background()to avoid blocking sync start. - Reorder sync-tracking updates so the “currently syncing” counters and “coming online” bookkeeping happen before the presence update is dispatched.
- Add a Towncrier newsfragment documenting the
/syncunblocking behavior change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| synapse/handlers/presence.py | Makes user_syncing() fire-and-forget the presence set_state() update to avoid blocking /sync on presence I/O. |
| changelog.d/19885.bugfix | Adds a changelog entry noting /sync is no longer blocked by presence updates when enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Note that this causes last_active_ts to be incremented which is not | ||
| # what the spec wants. Run in background to avoid blocking sync start. | ||
| run_in_background( | ||
| self.set_state, | ||
| UserID.from_string(user_id), | ||
| device_id, | ||
| {"presence": presence_state}, | ||
| is_sync=True, | ||
| ) |
| # Note that this causes last_active_ts to be incremented which is not | ||
| # what the spec wants. | ||
| await self.set_state( | ||
| # what the spec wants. Run in background to avoid blocking sync start. | ||
| run_in_background( | ||
| self.set_state, | ||
| UserID.from_string(user_id), |
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
@shcherbak have you observed a noticeable difference in real-world usage with this change? I ask, as this is the first time I'm hearing of /sync being slow due to setting presence. I would expect /sync to take much more resources than updating presence state.
It also appears that presence tests are failing with this change.
|
enabling presence on my instance caused massive key synchronization problems because of blocking. Yes, I saw the failing tests. This mr is a draft , I need advice to move forward with it. @anoadragon453 do You see any posiible sollutions? |
|
Thanks for your initial work on this @shcherbak. Heads up that #19946 might end up superseding this PR. |
presence: run set_state in background during user_syncing to unblock sync
user_syncing() was awaiting set_state() before returning the context
manager, causing every /sync request to block on presence I/O (DB
read/write on the presence writer, or an HTTP RPC from workers). Move
the counter increment and mark_as_coming_online ahead of the call, then
fire set_state via run_in_background consistent with how _end() is
already handled.