Log looping call exceptions #4008
Conversation
If a looping call function errors, then it kills the loop entirely. Currently it throws away the exception logs, so we should make it actually log them. Fixes #3929
|
Should we not restart the looping call, vs logging and then breaking? |
|
(I guess, though, we do catch it lower, so this actually shouldn't ever happen. Hmmmmm.) |
synapse/util/__init__.py
Outdated
| @functools.wraps(f) | ||
| def wrap(*args, **kwargs): | ||
| try: | ||
| logger.info("Running looping call") |
There was a problem hiding this comment.
This doesn't seem like a useful thing to log at INFO.
There was a problem hiding this comment.
Woops. That's me failing to remove my debugging.....
|
Yeah, opinions welcome if we just want to restart the loop. I'm a bit cautious about changing the behaviour vs. the in built twisted one. |
synapse/util/__init__.py
Outdated
| @@ -66,7 +67,7 @@ def looping_call(self, f, msec): | |||
| f(function): The function to call repeatedly. | |||
| msec(float): How long to wait between calls in milliseconds. | |||
| """ | |||
| call = task.LoopingCall(f) | |||
| call = task.LoopingCall(_log_exception_wrapper(f)) | |||
There was a problem hiding this comment.
is there a reason for doing this rather than adding an errback to the deferred returned by call.start?
synapse/util/__init__.py
Outdated
| def wrap(*args, **kwargs): | ||
| try: | ||
| logger.info("Running looping call") | ||
| return f(*args, **kwargs) |
There was a problem hiding this comment.
this won't actually catch any asynchronous failures.
|
given this is intended as a last resort to catch utter brokenness, I'd advocate not restarting the broken task, and rather requiring that looping calls not to return failures if they want to keep running. |
erikjohnston
force-pushed
the
erikj/log_looping_exceptions
branch
from
572eab2 to
f5777ce
Compare
erikjohnston
force-pushed
the
erikj/log_looping_exceptions
branch
from
f5777ce to
8a1817f
Compare
synapse/util/__init__.py
Outdated
| @@ -112,17 +114,30 @@ def batch_iter(iterable, size): | |||
| return iter(lambda: tuple(islice(sourceiter, size)), ()) | |||
|
|
|||
|
|
|||
| def _log_exception_wrapper(f): | |||
| """Used to wrap looping calls to log loudly if they get killed | |||
| def make_log_failure_errback(msg, consumeErrors=True): | |||
There was a problem hiding this comment.
could you not have msg and consumeErrors as params to log_failure and then just do addErrback(log_failure, msg) ?
There was a problem hiding this comment.
... yes.
This PR is going terribly.
Yup, and this time I actually tried an async failure! |
**Warning**: This release removes the example email notification templates from `res/templates` (they are now internal to the python package). This should only affect you if you (a) deploy your Synapse instance from a git checkout or a github snapshot URL, and (b) have email notifications enabled. If you have email notifications enabled, you should ensure that `email.template_dir` is either configured to point at a directory where you have installed customised templates, or leave it unset to use the default templates. The configuration parser will try to detect the situation where `email.template_dir` is incorrectly set to `res/templates` and do the right thing, but will warn about this. Features -------- - Ship the example email templates as part of the package ([\#4052](#4052)) - Add support for end-to-end key backup (MSC1687) ([\#4019](#4019)) Bugfixes -------- - Fix bug which made get_missing_events return too few events ([\#4045](#4045)) - Fix bug in event persistence logic which caused 'NoneType is not iterable' ([\#3995](#3995)) - Fix exception in background metrics collection ([\#3996](#3996)) - Fix exception handling in fetching remote profiles ([\#3997](#3997)) - Fix handling of rejected threepid invites ([\#3999](#3999)) - Workers now start on Python 3. ([\#4027](#4027)) - Synapse now starts on Python 3.7. ([\#4033](#4033)) Internal Changes ---------------- - Log exceptions in looping calls ([\#4008](#4008)) - Optimisation for serving federation requests ([\#4017](#4017)) - Add metric to count number of non-empty sync responses ([\#4022](#4022))
If a looping call function errors, then it kills the loop entirely.
Currently it throws away the exception logs, so we should make it
actually log them.
Fixes #3929