Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#34825 closed Bug (fixed)

SQLite database files are not destroyed after tests

Reported by: Jacob Walls Owned by: David Sanders
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

After running the Django unit tests, I'm left with 8 .sqlite3 files like so.

Bisected to 3b3f38b3b09b0f2373e51406ecb8c9c45d36aebc.

tests % python3.11 runtests.py admin_inlines
Testing against Django installed in '/Users/jwalls/django/django' with up to 8 processes
Found 82 test(s).
Creating test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
System check identified no issues (0 silenced).
.ssssssssssss.....................................................................
----------------------------------------------------------------------
Ran 82 tests in 1.419s

OK (skipped=12)
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
tests % git status
On branch main
Your branch is ahead of 'origin/main' by 86 commits.
  (use "git push" to publish your local commits)

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        other_1.sqlite3
        other_2.sqlite3
        other_3.sqlite3
        other_4.sqlite3
        other_5.sqlite3
        other_6.sqlite3
        other_7.sqlite3
        other_8.sqlite3

nothing added to commit but untracked files present (use "git add" to track)

Interestingly, I set a breakpoint at _destroy_test_db() and found this:

(Pdb) test_database_name
'file:memorydb_default?mode=memory&cache=shared'

Change History (19)

comment:1 by David Sanders, 2 years ago

What's interesting is if you do --parallel=1 then they aren't left around 🤔

comment:2 by David Sanders, 2 years ago

I'm new to the test framework but here's what I found when doing some investigating:

Looks like the issue is with the new setup_worker_connection() method for the sqlite3 backend in that commit you bisected.

It attempts to copy from other_<worker-id>.sqlite3 into memory … but it doesn't exist because the test runner doesn't create databases for the "other" alias. The default behaviour of sqlite3.connect() is to create a database because the default mode is mode=rwc, c being for create.

We can apparently change the mode to mode=rw in which case an exception is raised. Or we can adjust the logic to use the same settings as "fork" if the file doesn't exist. 🤔

(I just quickly tested this theory by appending mode=rw and it no longer creates the other_n.sqlite3 files)

Last edited 2 years ago by David Sanders (previous) (diff)

comment:3 by David Sanders, 2 years ago

Has patch: set
Triage Stage: Unreviewed → Accepted

PR demonstrating my comment: ​https://github.com/django/django/pull/17242/files

comment:4 by Jacob Walls, 2 years ago

Owner: changed from nobody to David Sanders
Status: new → assigned

comment:5 by Natalia Bidart, 2 years ago

I'm not being able to reproduce, I'm using python3.11 with latest main on a Manjaro system:

$ python3.11 runtests.py admin_inlines
Testing against Django installed in '/home/nessita/fellowship/django/django' with up to 4 processes
Found 82 test(s).
Creating test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
System check identified no issues (0 silenced).
..............ssssssssssss........................................................
----------------------------------------------------------------------
Ran 82 tests in 1.928s

OK (skipped=12)
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...

$ git st
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean

$ ls *sqlite*
test_sqlite.py

comment:6 by Mariusz Felisiak, 2 years ago

I'm not being able to reproduce, I'm using python3.11 with latest main on a Manjaro system:

It's an issue with "spawn" method, so it's reproducible on Windows and MacOS.

comment:7 by David Sanders, 2 years ago

I haven't tried this but you might also be able to set the start method in runtests.py and change to spawn: ​https://docs.python.org/3/library/multiprocessing.html#multiprocessing.set_start_method

comment:8 by Natalia Bidart, 2 years ago

Thank you everyone, I was able to reproduce with this diff:

  • tests/runtests.py 

    diff --git a/tests/runtests.py b/tests/runtests.py
index 2eb7490170..ad63e6a286 100755
    a b if __name__ == "__main__":  
    744744        os.environ.setdefault("DJANGO_SETTINGS_MODULE", "test_sqlite")
    745745        options.settings = os.environ["DJANGO_SETTINGS_MODULE"]
    746746
     747    multiprocessing.set_start_method("spawn")
    747748    if options.selenium:
    748749        if multiprocessing.get_start_method() == "spawn" and options.parallel != 1:
    749750            parser.error(

comment:9 by Natalia Bidart, 2 years ago

Triage Stage: Accepted → Ready for checkin

comment:10 by faizan2700, 2 years ago

Please assign me this, I find it interesting.

comment:11 by David Sanders, 2 years ago

Hi faizan2700, unfortunately it's already assigned & fixed. Your best bet is to search through tickets that are unassigned if you'd like to contribute.

comment:12 by Natalia Bidart, 2 years ago

Triage Stage: Ready for checkin → Accepted

Setting as Accepted until the conversation in the PR are resolved.

comment:13 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:14 by Mariusz Felisiak, 2 years ago

Patch needs improvement: unset
Triage Stage: Accepted → Ready for checkin

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: → fixed
Status: assigned → closed

In a5905b16:

Fixed #34825 -- Avoided setting unused connections when initializing parallel workers.

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 2128a737:

Refs #34825 -- Made SQLite backend open source database in readonly mode when using spawn.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In bc6d71d4:

[5.0.x] Fixed #34825 -- Avoided setting unused connections when initializing parallel workers.

Backport of a5905b164dbf52e59fa646af9c3d523c0804d86a from main

comment:18 by GitHub <noreply@…>, 2 years ago

In 68d0159b:

Fixed #34903, Refs #34825 -- Made workers initialization respect empty set of used connections.

Thanks to David Smith for the investigation & patch.

Regression in 2128a73713735fb794ca6565fd5d7792293f5cfa.
Follow up to a5905b164dbf52e59fa646af9c3d523c0804d86a.

Co-authored-by: David Sanders <shang.xiao.sanders@…>

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 08aa336a:

[5.0.x] Fixed #34903, Refs #34825 -- Made workers initialization respect empty set of used connections.

Thanks to David Smith for the investigation & patch.

Regression in 2128a73713735fb794ca6565fd5d7792293f5cfa.
Follow up to a5905b164dbf52e59fa646af9c3d523c0804d86a.

Co-authored-by: David Sanders <shang.xiao.sanders@…>
Backport of 68d0159b6dfce07f144045d56639c52066e8b90e from main

Note: See TracTickets for help on using tickets.
Back to Top