Close old DB connections allow to recover from DB errors on each iteration #147
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #147 +/- ##
========================================
Coverage 97.85% 97.85%
========================================
Files 6 6
Lines 326 327 +1
========================================
+ Hits 319 320 +1
Misses 7 7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| with pytest.raises(db.OperationalError): | ||
| with db.connection.cursor() as cursor: | ||
| cursor.execute("INVALID SYNTAX ERROR") | ||
| assert db.connection.errors_occurred |
There was a problem hiding this comment.
It seems like the only purpose of executing a query here that deliberately raises an error, and then checking errors_occurred, is to determine if django.db.backends.base.BaseDatabaseWrapper.close_if_unusable_or_obsolete() was called.
We could probably achieve the same more simply by mocking that method out and checking if it was called.
| # | ||
| # get_due_jobs() seems like the most appropriate place for this, | ||
| # as it's called at the beginning of every scheduler iteration. | ||
| db.close_old_connections() |
There was a problem hiding this comment.
Pre-emptively closing connections like this is problematic (see #145 (comment)).
The standard Django can get away with it because it does so at the beginning and end of a request-response-cycle, while all other db operations within that cycle still get to re-use the same connection.
| # | ||
| # get_due_jobs() seems like the most appropriate place for this, | ||
| # as it's called at the beginning of every scheduler iteration. | ||
| db.close_old_connections() |
There was a problem hiding this comment.
Yes, but database connections can be closed by the backend database engine literally anytime and while we are processing code anywhere in the project (see the issue log for examples of where this issue has cropped up elsewhere as well).
We need to find a catch-all solution if possible.
|
Thanks for the PR. I think this is a hard problem to solve and I am not entirely sure how best to go about it. Please see #148 for an alternative approach that attempts to cover more cases and avoid some of the many pitfalls and side effects. I would be grateful if you could try it out and report your findings there. |
This is a proposed fix for #145. Please see the code comment for an explanation.
The added call can cause a connection to close, so it is somewhat assumed that
get_due_jobs()is run from the scheduler loop in a context which "owns" a connection (e.g. a management command or dedicated thread) and not a shared/request context.The
close_old_connectionsAPI itself is not documented but has existed for a long time and I doubt it will disappear without notice.The test is a bit hacky but it's what I managed with sqlite.