[Django] #33277: SimpleTestCase does not block database connections in threads
Django
---------------------------------------------+------------------------
Reporter: Daniel Hahler | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
Due to {{{ConnectionHandler}}}'s connections being thread-local [1] new
connections will be used in new threads, which then do not have been
patched for disallowed methods [2].
Given {{{test_simpletestcase.py}}}:
{{{
import threading
from django.db import connection
from django.test import SimpleTestCase
class MySimpleTestCase(SimpleTestCase):
def test_this(self):
try:
with connection.cursor() as cursor:
cursor.execute("SELECT 1")
raise Exception("should have failed")
except AssertionError:
pass
res = []
def thread_func():
res.append(1)
try:
with connection.cursor() as cursor:
cursor.execute("SELECT 1")
raise Exception("should have failed")
except AssertionError:
pass
res.append(2)
t = threading.Thread(target=thread_func)
t.start()
t.join()
assert res == [1, 2], res
}}}
{{{./manage.py test test_simpletestcase.py}}} fails like this:
{{{
Exception in thread Thread-1:
Traceback (most recent call last):
File "/usr/lib/python3.9/threading.py", line 973, in _bootstrap_inner
self.run()
File "/usr/lib/python3.9/threading.py", line 910, in run
self._target(*self._args, **self._kwargs)
File "…/test_simpletestcase.py", line 23, in thread_func
raise Exception("should have failed")
Exception: should have failed
F
======================================================================
FAIL: test_this (test_simpletestcase.MySimpleTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "…/test_simpletestcase.py", line 31, in test_this
assert res == [1, 2], res
AssertionError: [1]
----------------------------------------------------------------------
Ran 1 test in 0.006s
FAILED (failures=1)
}}}
(Note that there is some handling of {{{connection.settings_dict}}} for
workers of the test runner, which is only slightly related:
https://github.com/django/django/blob/dfa1145a22042dcf9e504a5a7edd5557e3e0d07c/django/test/runner.py#L327-L335)
A possible solution might be to use the existing
[https://docs.djangoproject.com/en/3.2/ref/signals/#connection-created
connection_created] signal to raise an exception when a connections was
created (although that would happen only after the fact - a new pre-
connect signal could be used/added for this).
Given that the test DB names are not prefixed with {{{test_}}} with
{{{SimpleTestCase}}} you might accidentally change the production DB from
within your tests when something like a {{{ThreadPoolExecutor}}} is being
used when mixing sync with async etc.
1:
https://github.com/django/django/blob/dfa1145a22042dcf9e504a5a7edd5557e3e0d07c/django/utils/connection.py#L41
2:
https://github.com/django/django/blob/dfa1145a22042dcf9e504a5a7edd5557e3e0d07c/django/test/testcases.py#L183
--
Ticket URL: <https://code.djangoproject.com/ticket/33277>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Daniel Hahler | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Daniel Hahler:
Old description:
New description:
Given {{{test_simpletestcase.py}}}:
{{{
import threading
res = []
FAILED (failures=1)
}}}
Note: pytest-django monkeypatches
{{{django.db.backends.base.base.BaseDatabaseWrapper.ensure_connection}}}
to block DB access, which appears to work better in this regard (across
threads).
1:
https://github.com/django/django/blob/dfa1145a22042dcf9e504a5a7edd5557e3e0d07c/django/utils/connection.py#L41
2:
https://github.com/django/django/blob/dfa1145a22042dcf9e504a5a7edd5557e3e0d07c/django/test/testcases.py#L183
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:1>
Django
Reporter: Daniel Hahler | Owner: nobody
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
Comment:
Hi Daniel, thanks for this — Nice report. (Was momentarily confused by
the footnotes linking to the initial commit, e.g. [1] :)
Yes, I think SimpleTestCase likely should do the right thing here.
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:2>
Django
Reporter: Daniel Hahler | Owner: wangxiaolei
Type: New feature | Status: assigned
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wangxiaolei):
* owner: nobody => wangxiaolei
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:3>
Django
Reporter: Daniel Hahler | Owner: Sulabh
| Katila
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sulabh Katila):
* owner: nobody => Sulabh Katila
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:3>
Django
Reporter: Daniel Hahler | Owner: Sulabh
| Katila
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* cc: David Wobrock (added)
Comment:
Hey Sulabh,
I see you assigned yourself the ticket, that's great!
I've started a [https://github.com/django/django/pull/17075 PR] some
months ago, feel free to check it out if it can give you some inspiration
:)
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:4>
Django
Reporter: Daniel Hahler | Owner: Sulabh
| Katila
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sulabh Katila):
Hello David,
Thank you for getting the ball rolling on this feature! I've looked at the
PR, and it's impressive. I'll definitely use it as inspiration to continue
the work. If you have any specific insights or tips based on your progress
so far, please feel free to share.
I'm looking forward to working on this together!
Replying to [comment:4 David Wobrock]:
> Hey Sulabh,
>
> I see you assigned yourself the ticket, that's great!
> I've started a [https://github.com/django/django/pull/17075 PR] some
months ago, feel free to check it out if it can give you some inspiration
:)
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:5>
Django
Reporter: Daniel Hahler | Owner: Sulabh
| Katila
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sulabh Katila):
Hi David,
I've been grappling with this issue for a while now, and I haven't made
much progress. Given your expertise in this area, I'm reaching out for
some much-needed guidance.
To address this, I've been contemplating the idea of implementing a "pre-
signal" solution. However, I must admit that I'm currently stuck, and I'm
unsure about how to move forward.
I had initially considered intervening at the
BaseDataBaseWrapper.connect() level, but I haven't been able to find a
clear and effective path for implementation.
Since you have experience in related areas of this issue, I was hoping you
could provide some insights. What should I be looking for, or are there
alternative approaches that might be more suitable?
Thank you!
Replying to [comment:4 David Wobrock]:
> Hey Sulabh,
>
> I see you assigned yourself the ticket, that's great!
> I've started a [https://github.com/django/django/pull/17075 PR] some
months ago, feel free to check it out if it can give you some inspiration
:)
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:6>
Django
Reporter: Daniel Hahler | Owner: Sulabh
| Katila
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Wobrock):
I do not think I have that much experience.
How did you implement the "pre-signal" solution? Do you have an open
PR/branch to check out the code :)
And did you have a look on the
[https://github.com/django/django/pull/17075 patching method] approach,
and play around with it?
I feel like it could be a working path - it mainly requires some test
fixing.
Replying to [comment:6 Sulabh Katila]:
> Hi David,
>
> I've been grappling with this issue for a while now, and I haven't made
much progress. Given your expertise in this area, I'm reaching out for
some much-needed guidance.
>
> To address this, I've been contemplating the idea of implementing a
"pre-signal" solution. However, I must admit that I'm currently stuck, and
I'm unsure about how to move forward.
>
> I had initially considered intervening at the
BaseDataBaseWrapper.connect() level, but I haven't been able to find a
clear and effective path for implementation.
>
> Since you have experience in related areas of this issue, I was hoping
you could provide some insights. What should I be looking for, or are
there alternative approaches that might be more suitable?
>
> Thank you!
>
> Replying to [comment:4 David Wobrock]:
> > Hey Sulabh,
> >
> > I see you assigned yourself the ticket, that's great!
> > I've started a [https://github.com/django/django/pull/17075 PR] some
months ago, feel free to check it out if it can give you some inspiration
:)
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:7>
Django
Reporter: Daniel Hahler | Owner: Sulabh
| Katila
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
Hey Sulabh,
I took a few more minutes to dig into the issue and I think we are close
to a fix :)
djangoci.com is down at the time of writing, so I'm missing the complete
test suite, but it looks quite promising for now! We just need to fix a
few tests.
=> [https://github.com/django/django/pull/17075 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:8>
Django
Reporter: Daniel Hahler | Owner: David
| Wobrock
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* owner: Sulabh Katila => David Wobrock
* needs_better_patch: 1 => 0
Comment:
I picked up the existing patch and opened a new
[https://github.com/django/django/pull/17639 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:9>
Django
Reporter: Daniel Hahler | Owner: David
| Wobrock
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:10>
Django
Reporter: Daniel Hahler | Owner: David
| Wobrock
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:11>
Django
Reporter: Daniel Hahler | Owner: David
| Wobrock
Type: New feature | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:12>
Django
Reporter: Daniel Hahler | Owner: David
| Wobrock
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8fb0be3500cc7519a56985b1b6f415d75ac6fedb" 8fb0be35]:
{{{
#!CommitTicketReference repository=""
revision="8fb0be3500cc7519a56985b1b6f415d75ac6fedb"
Fixed #33277 -- Disallowed database connections in threads in
SimpleTestCase.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33277#comment:13>