fix: Cap WorkerLock timeout intervals to 60 seconds#19394
Conversation
…es, continue logging at durations greater than 10 minutes
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
|
After the issue occured again in our prod: My hypothesis would be: the issue is not primarily about the dimensions of the growing timeout but, about the timeout being ignored at all? At least the logged timeout is not reflected in the timestamp deltas of the log lines?! |
Yes there is more than one thing going on here. This fix(switch |
…only increase it when a timeout occurs
|
Added some additional work onto this:
This allows that a normal notification of another lock being released should not increment a timeout, when a time out has not actually occurred. It should cut down on what may end up otherwise being excessive log spam about locks having a long timeout duration when such is not true. The jitter value is maintained for the timeout, to help avoid a "thundering herd" situation when all locks may time out at the same time. |
WorkerLock retry intervals to 15 minutesWorkerLock timeout intervals to 15 minutes
| # Recall that pump() will advance time of the given amount 100 times, this | ||
| # amounts to about 10 seconds passing | ||
| self.pump(0.1) |
There was a problem hiding this comment.
To be clear, nothing for you to do here yet. Just noting this discussion so we can think about what to do for these scenarios.
Since #19602, pump() is deprecated and we can be more precise here by using self.reactor.advance(...)
But actually trying this out here and there some internal details leaking through and we would end up with this which isn't great.
| # Recall that pump() will advance time of the given amount 100 times, this | |
| # amounts to about 10 seconds passing | |
| self.pump(0.1) | |
| # Wait for database queries to run so we get past the | |
| # `try_acquire_lock(...)` in `WaitingLock` and the timeout actually gets | |
| # scheduled | |
| self.reactor.advance(0) | |
| # Wait some time which should trigger waiting loop to timeout and retry | |
| # acquiring the lock a few times | |
| self.reactor.advance(10) |
Related comments around how the database queries are ran (added in matrix-org/synapse#8497):
Lines 660 to 669 in 76b4fdc
cc @reivilibre (someone who also has been in the loop around pump() vs advance())
I can maybe see how pump(...) became a thing 🤔 but it's still imprecise.
WorkerLock timeout intervals to 15 minutesWorkerLock timeout intervals to 60 seconds
| while self.reactor.seconds() < end_time_s: | ||
| self.reactor.advance(by.as_secs()) |
There was a problem hiding this comment.
Just to note for people who might come back to look at this PR, the implementation I recommended here isn't quite precise as it may advance time by one by increment beyond the amount. But it's fine for our use case ⏩
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
reivilibre
left a comment
There was a problem hiding this comment.
Generally fine other than a little footgun with our Duration class!
| start_ts_ms: int = 0 | ||
| deferred: "defer.Deferred[None]" = attr.Factory(defer.Deferred) | ||
| _inner_lock: Lock | None = None | ||
| _retry_interval: float = 0.1 |
There was a problem hiding this comment.
Fixes the symptoms of #19315 / #19588 but not the underlying reason causing the number to grow so large in the first place.
I think this fixes the actual problems, not just the symptoms.
The root cause is that _retry_interval can grow past Duration(timedelta)'s max value and raise an OverflowError. When this occurs, there is a tight-loop where the OverflowError propagates up from Duration() → clock.call_later() → timeout_deferred() → await timeout_deferred(...), where it is caught by the bare except Exception: pass at line 254-255. The loop immediately continues to the next iteration with no waiting at all.
Even the initially proposed change here fixed the problem of the _retry_interval/timeout growing too large. Additionally, this PR has since evolved to only increase the timeout when we actually encounter a defer.TimeoutError which is another fix that would solve this.
It's possible that this code could tight-loop again if we encounter some other exception over and over but I guess that's expected and our "critical" section is already as small as possible around timeout_deferred(...). We could have some tight-loop detection but feels overkill as at some point, we have to trust timeout_deferred(...) to do the right thing. Overall, the situation is better here as this PR adds some logging for the general exception case instead of swallowing the error and moving on so you can actually notice any tight-loop root cause now.
Random dev notes
Reproduce Duration(timedelta) raising OverflowError
$ poetry run python
>>> Duration(seconds=10 ** 14)
Traceback (most recent call last):
File "<python-input-5>", line 1, in <module>
Duration(seconds=10 ** 14)
~~~~~~~~^^^^^^^^^^^^^^^^^^
OverflowError: days=1157407407; must have magnitude <= 999999999
Fixes the symptoms of #19315 / #19588 but not the underlying reason causing the number to grow so large in the first place. ``` ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit ``` Copied from the original pull request on [Famedly's Synapse repo](famedly/synapse#221) (with some edits): Basing the time interval around a 5 seconds leaves a big window of waiting especially as this window is doubled each retry, when another worker could be making progress but can not. Right now, the retry interval in seconds looks like `[0.2, 5, 10, 20, 40, 80, 160, 320, (continues to double)]` after which logging should start about excessive times and (relatively quickly) end up with an extremely large retry interval with an unrealistic expectation past the heat death of the universe. 1 year in seconds = 31,536,000. With this change, retry intervals in seconds should look more like: ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 60, < never goes higher than this ] ``` Logging about excessive wait times will start at 10 minutes. <details> <summary>Previous breakdown when we were using 15 minutes</summary> ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 102.4, # 1.7 minutes 204.8, # 3.41 minutes 409.6, # 6.83 minutes 819.2, # 13.65 minutes < logging about excessive times will start here, 13th iteration 900, # 15 minutes < never goes higher than this ] ``` </details> Further suggested work in this area could be to define the cap, the retry interval starting point and the multiplier depending on how frequently this lock should be checked. See data below for reasons why. Increasing the jitter range may also be a good idea --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com> (cherry picked from commit 3f58bc5)
Better to retry more quickly than have workers wait around. 5 seconds is still a reasonable gap in time to not overwhelm anything. This matters most in cross-worker scenarios. When locks are on the same worker, when the lock holder releases, we signal to other locks (with the same name/key) that they should try reacquiring the lock immediately. But locks on other workers only re-check based on their retry `_timeout_interval`. Updating to 5 seconds to match the previous intentions based on the [flawed code](https://github.com/element-hq/synapse/blob/6100f6e4f7fb0c72f1ae2802683ebc811c0e3a77/synapse/handlers/worker_lock.py#L278). We can assume they were trying to have 5 seconds as the max value to retry. Spawning from #19394 (comment)
So people have to specify which time unit they want to use. Spawning from #19394 (comment)
…_INTERVAL` (#19772) There is no behavioral change, only a change to the tests. See #19772 (comment) for an explanation of why the tests needed changing (and diff comments). Follow-up to #19394. The test discussion originally happened in #19394 (comment) This is spawning from thinking about the problem again.
Fixes the symptoms of element-hq#19315 / element-hq#19588 but not the underlying reason causing the number to grow so large in the first place. ``` ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit ``` Copied from the original pull request on [Famedly's Synapse repo](famedly/synapse#221) (with some edits): Basing the time interval around a 5 seconds leaves a big window of waiting especially as this window is doubled each retry, when another worker could be making progress but can not. Right now, the retry interval in seconds looks like `[0.2, 5, 10, 20, 40, 80, 160, 320, (continues to double)]` after which logging should start about excessive times and (relatively quickly) end up with an extremely large retry interval with an unrealistic expectation past the heat death of the universe. 1 year in seconds = 31,536,000. With this change, retry intervals in seconds should look more like: ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 60, < never goes higher than this ] ``` Logging about excessive wait times will start at 10 minutes. <details> <summary>Previous breakdown when we were using 15 minutes</summary> ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 102.4, # 1.7 minutes 204.8, # 3.41 minutes 409.6, # 6.83 minutes 819.2, # 13.65 minutes < logging about excessive times will start here, 13th iteration 900, # 15 minutes < never goes higher than this ] ``` </details> Further suggested work in this area could be to define the cap, the retry interval starting point and the multiplier depending on how frequently this lock should be checked. See data below for reasons why. Increasing the jitter range may also be a good idea --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
Better to retry more quickly than have workers wait around. 5 seconds is still a reasonable gap in time to not overwhelm anything. This matters most in cross-worker scenarios. When locks are on the same worker, when the lock holder releases, we signal to other locks (with the same name/key) that they should try reacquiring the lock immediately. But locks on other workers only re-check based on their retry `_timeout_interval`. Updating to 5 seconds to match the previous intentions based on the [flawed code](https://github.com/element-hq/synapse/blob/6100f6e4f7fb0c72f1ae2802683ebc811c0e3a77/synapse/handlers/worker_lock.py#L278). We can assume they were trying to have 5 seconds as the max value to retry. Spawning from element-hq#19394 (comment)
…19756) So people have to specify which time unit they want to use. Spawning from element-hq#19394 (comment)
…_INTERVAL` (element-hq#19772) There is no behavioral change, only a change to the tests. See element-hq#19772 (comment) for an explanation of why the tests needed changing (and diff comments). Follow-up to element-hq#19394. The test discussion originally happened in element-hq#19394 (comment) This is spawning from thinking about the problem again.
Fixes the symptoms of #19315 / #19588 but not the underlying reason causing the number to grow so large in the first place.
Copied from the original pull request on Famedly's Synapse repo (with some edits):
Basing the time interval around a 5 seconds leaves a big window of waiting especially as this window is doubled each retry, when another worker could be making progress but can not.
Right now, the retry interval in seconds looks like
[0.2, 5, 10, 20, 40, 80, 160, 320, (continues to double)]after which logging should start about excessive times and (relatively quickly) end up with an extremely large retry interval with an unrealistic expectation past the heat death of the universe. 1 year in seconds = 31,536,000.With this change, retry intervals in seconds should look more like:
Logging about excessive wait times will start at 10 minutes.
Previous breakdown when we were using 15 minutes
Further suggested work in this area could be to define the cap, the retry interval starting point and the multiplier depending on how frequently this lock should be checked. See data below for reasons why. Increasing the jitter range may also be a good idea
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.