Cleanly shutdown SynapseHomeServer object #18828
Conversation
This reverts commit 61508a6.
Related to #18905 These fixes were split off from #18828 where @devonh was seeing some test failures because `timeout_deferred(...)` is being updated to use `Clock` utilities instead of raw `reactor` methods. This test was failing in that branch/PR until we made this new version that handles the logcontexts properly. While the previous version of this test does pass on `develop`, it was using what appears completely wrong assertions, assumptions, and bad patterns to make it happen (see diff comments below) --- Test originally introduced in matrix-org/synapse#4407
synapse/util/clock.py
Outdated
|
|
||
|
|
||
| @attr.s(slots=True) | ||
| CALL_LATER_DELAY_TRACKING_THRESHOLD_S = 30 |
There was a problem hiding this comment.
Is 30s really fine?
This means in the worst case, there is a minimum of 30s of downtime when deprovisioning and provisioning a homeserver tenant in the context of Synapse Pro for Small Hosts.
Related to discussion about just tracking all calls: #18828 (comment)
There was a problem hiding this comment.
Alright, let's just track them all at this point and see how it goes.
synapse/handlers/delayed_events.py
Outdated
| if self._next_delayed_event_call is None: | ||
| self._next_delayed_event_call = self._clock.call_later( | ||
| delay_sec, | ||
| True if delay_sec > CALL_LATER_DELAY_TRACKING_THRESHOLD_S else False, |
There was a problem hiding this comment.
👍 I think we have the correct solution if we want this optimization ⏩
Still seems like we should track everything or at-least we don't have very good reasoning for why not. See other conversation (#18828 (comment)) about how this forces downtime in the worst case scenarios.
| Args: | ||
| deferred: The Deferred to potentially timeout. | ||
| timeout: Timeout in seconds | ||
| cancel_on_shutdown: Whether this call should be tracked for cleanup during |
There was a problem hiding this comment.
Do we still use this anywhere? Why?
Feels like it should be removed
| callback: Callable, | ||
| *args: Any, | ||
| call_later_cancel_on_shutdown: Optional[bool] = None, | ||
| call_later_cancel_on_shutdown: bool = True, |
There was a problem hiding this comment.
Do we still use this anywhere? Why?
Feels like it should be removed
| self.server_name = ( | ||
| hs.hostname | ||
| ) # nb must be called this for @wrap_as_background_process | ||
| self.hs = hs # nb must be called this for @wrap_as_background_process |
There was a problem hiding this comment.
There is still an instance in synapse/handlers/device.py that needs some adjusting
synapse/synapse/handlers/device.py
Lines 967 to 969 in 84e1d15
Although in this case, we need to keep part of the comment for @measure_func
This PR aims to allow for a clean shutdown of the
SynapseHomeServerobject so that it can be fully deleted and cleaned up by garbage collection without shutting down the entire python process.Fix https://github.com/element-hq/synapse-small-hosts/issues/50
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.