-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Brutal connection cleanup in teardown of django_db_setup #429
Comments
It's the first time for me to hear about this requirement. |
It works well as we have it now. I know more projects that have the same issue, will find some references for it. For example celery workers that have connection to the db. @ionelmc |
This is an example of exceptions we get if we don't brute force terminate the connections that might be active...
|
@peterlauri, thanks for leaving this reference here. I've been running into, and trying to diagnose this problem in my work codebase for the last couple of days. The issue is intermittent. It fails with the same operational error you saw above in about 40-60% of the test runs. Other times it works just fine. From digging in and printing entries in SELECT "django_migrations"."app", "django_migrations"."name" FROM "django_migrations" The failure persists, whether I use Have you made any progress with this? (N.B. I am using Django 2.0.3 and Python 3) |
For information: I have isolated the problem to an issue with transaction management. It's not clear at the moment whether the problem lies in Django or in pytest_django, so I'm digging further. Here's what I know now: When the # django.test.testcases.TestCase
@classmethod
def _enter_atomics(cls):
"""Open atomic blocks for multiple databases."""
atomics = {}
for db_name in cls._databases_names():
atomics[db_name] = transaction.atomic(using=db_name)
atomics[db_name].__enter__()
return atomics When def __enter__(self):
connection = get_connection(self.using)
if not connection.in_atomic_block:
# Reset state when entering an outermost atomic block.
connection.commit_on_exit = True
connection.needs_rollback = False
if not connection.get_autocommit(): It is here that things go awry. During successful test passes, the connection at that point is in an atomic block, and so we do not enter that block of code. When test passes fail, the connection is not in an atomic block, and so we end up invoking When we skip doing this, the connection to the database that is used for each test executed is the same connection as the one used for setting up the database. When we don't skip it, then a new connection is used for running tests, and the connection from database setup is left open When test execution is completed, Django closes a database connection, but because there are two connections open, the other is left open. When I'm not clear if this is a problem that can be cleanly solved. I could use a bit of help in trying to figure that out. In any case, the open connection is no longer doing any useful work, so it is likely safe to use the hammer approach suggested in this issue. |
Just for clarification: is this with 3.1.2? Have you tried master? (soon to be released as 3.2.0) |
@blueyed, yes it is 3.1.2. I haven’t tried master, but will on Monday. I’ll post the outcome here, and dig a bit further to root cause this if it persists. |
@blueyed: I can verify that the same problems occur on master as in 3.1.2. Working on a root cause. |
I've found the root cause of our problem. It's related to the fact that the settings we use for our tests and our local development include two configured databases, one is our primary and the other a replica. Looking at the documentation for pytest-django, it's clear that multiple databases are not supported, so I don't believe that the right fix for this issue lies here. But I'm going to write out what happened anyway, in case it helps someone else in the future. When the In our case, this was not initially detected because the configuration for each in our test settings are identical. But the connection to each is a unique object. So if creating the database picked the replica, then the final action performed on that connection left the connection open, and prevented the database from being torn down at the end. The appropriate solution to our problem is in fact documented here. By adding information to the database configuration indicating that the replica is to be treated as a mirror of the primary during tests, the problem disappears. Thanks for the patience. |
I am running into the same issue with only using a single database ("default") and am using this fix: def teardown_database():
with django_db_blocker.unblock():
# close unused connections
for connection, old_name, destroy in db_cfg:
if not destroy:
connection.close()
# delete test databases
for connection, old_name, destroy in db_cfg:
if not destroy:
continue
connection.creation.destroy_test_db(
old_name, request.config.option.verbose, False
)
connection.close() The problem was that the connections were not being closed properly in the main function. Full context: @pytest.fixture(scope="session")
def django_db_setup(request, django_test_environment, django_db_blocker):
"""Top level fixture to ensure test databases are available"""
from pytest_django.compat import setup_databases
with django_db_blocker.unblock():
db_cfg = setup_databases(
verbosity=request.config.option.verbose,
interactive=False,
)
def teardown_database():
with django_db_blocker.unblock():
# close unused connections
for connection, old_name, destroy in db_cfg:
if not destroy:
connection.close()
# delete test databases
for connection, old_name, destroy in db_cfg:
if not destroy:
continue
connection.creation.destroy_test_db(
old_name, request.config.option.verbose, False
)
connection.close()
request.addfinalizer(teardown_database) |
@snowman2, do you have an updated fixture for pytest-django 4.5.2? I tried your fixture, but this line causes an error since there's no compat module in the latest version: |
That is a few years old. I don't think that I am using that anymore, but I don't remember what project I was using that for to verify. Current projects don't use that block of code. |
We are having a few projects that suffer from hanging connections to the DB, can be from external processes or threads that holds an connection to the database, and causes tests to fail from time to time.
Below is the fixture we have in all our conftests.py to make the cleanup more brutal, to assure that there are no hanging connections around that can cause the tests to fail...
Are we alone having this problems? It is not a big deal having this specific extension of django_db_setup, but if there are plenty of people doing similar hacks, it should be considered :)
The text was updated successfully, but these errors were encountered: