Error signals are not reliable, especially when dealing with database errors
|Reported by:||tal@…||Owned by:||schacki|
|Severity:||Normal||Keywords:||signals errors databaseError|
|Cc:||tal@…, preston@…, schacki||Triage Stage:||Accepted|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||yes||Patch needs improvement:||yes|
In core.handlers.base, unhandled exceptions are processed as such:
except: # Handle everything else, including SuspiciousOperation, etc. # Get the exception info now, in case another exception is thrown later. signals.got_request_exception.send(sender=self.__class__, request=request) response = self.handle_uncaught_exception(request, resolver, sys.exc_info())
Which delivers the error signal to the various handlers (database transaction resets, things like Sentry etc).
However, the code that dispatches signals aborts the dispatch if any of the handlers fail (to try..except block):
for receiver in self._live_receivers(_make_id(sender)): response = receiver(signal=self, sender=sender, **named) responses.append((receiver, response)) return responses
This is perfectly reasonable in most cases, but is problematic when dealing with top-level error handling, as this prevents the unhandled error from reaching handlers that just want to report it.
This is very noticeable in cases where "something bad" happens to the database connection, as the first handler is always the database rollback handler, which only catches DatabaseError, which excludes a lot of errors that can and do crop up in production
def _rollback_on_exception(**kwargs): from django.db import transaction for conn in connections: try: transaction.rollback_unless_managed(using=conn) except DatabaseError: pass
I think that it will be better to have the error signal dispatcher defer except raising until after all the handlers were called (or just swallow them all).
Alternatively, a different signal for error reporting is possible, but I think that's a little confusing.
Change History (19)
comment:1 Changed 2 years ago by anonymous
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
Changed 2 years ago by tal@…
comment:7 Changed 2 years ago by schacki
- Owner changed from nobody to schacki
- Status changed from new to assigned
Changed 2 years ago by schacki
comment:10 Changed 2 years ago by ptone
- Component changed from Core (Other) to HTTP handling
- Easy pickings unset
- Triage Stage changed from Unreviewed to Accepted
- Version changed from 1.5 to master