[Django] #32416: Apparent regression of #22414 from switching to ThreadedWSGIServer in LiveServerTestCase (#20238)
Django
LiveServerTestCase (#20238)
------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.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 |
------------------------------------------+------------------------
In Django 2.2.17, I'm seeing the reappearance of #22414 after it was fixed
in 1.11. #22414 is the issue where the following error will occur at the
conclusion of a test run when `destroy_test_db()` is called:
> OperationalError: database "test_myapp" is being accessed by other users
This error happens when not all of the database connections are closed. In
my case today, I'm seeing this when running a single test that is a
`LiveServerTestCase`. I see it in approximately half of my test runs, so
it's not wholly deterministic (it's a race condition).
There weren't a whole lot of changes in the `LiveServerTestCase`-related
code between 1.11 and 2.2, so I looked at them individually.
Issue #20238 added threading support to `LiveServerTestCase`. One of the
changes it made
[https://github.com/django/django/commit/bece837829eafbc22f2598dadf82c9a8364b085a
#diff-
7833da5b45a68d00834388d97dd5c4413e3796497c7bc5e0cc2621b08a2d0df1R1251 was
changing] `LiveServerThread` to use `ThreadedWSGIServer` instead of
`WSGIServer`. `LiveServerThread` is used by `LiveServerTestCase`.
When I tried modifying `LiveServerThread` to use the old `WSGIServer`, I
could no longer reproduce the above error. My changes were as follows:
{{{#!python
class NonThreadedLiveServerThread(LiveServerThread):
def _create_server(self):
return WSGIServer((self.host, self.port), QuietWSGIRequestHandler,
allow_reuse_address=False)
class MyTest(LiveServerTestCase):
server_thread_class = NonThreadedLiveServerThread
}}}
The CPython docs [https://docs.python.org/3/library/socketserver.html
describe] `ThreadingMixIn` as defining an attribute "which indicates
whether or not the server should wait for thread termination."
Consistent with what I described above, Aymeric
[https://code.djangoproject.com/ticket/20238#comment:5 said the following]
on ticket #20238, seeming to foreshadow issues like this one:
> more threading will certainly create more race conditions on shutdown,
especially when it comes to the database connections — it's taken months
to eliminate most from LiveServerTestCase, and I'm sure there are still
some left,
--
Ticket URL: <https://code.djangoproject.com/ticket/32416>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
LiveServerTestCase (#20238)
Reporter: Chris Jerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
I wonder if this issue is because `ThreadingMixIn` creates a new thread
for each request, but those threads don't close their database connections
at their conclusion, e.g. like `LiveServerThread` does.
Here is the code in CPython for `ThreadingMixIn`'s `process_request()` and
`server_close()`:
https://github.com/python/cpython/blob/v3.9.1/Lib/socketserver.py#L656-L674
And here is the code for Django's `LiveServerThread.run()`, which does
close its connections:
https://github.com/django/django/blob/3f8979e37b6c498101d09a583ccc521d7f2879e5/django/test/testcases.py#L1460-L1484
(I wonder if it's also a problem that `ThreadingMixIn` doesn't implement
the same thread-sharing logic that `LiveServerThread` does, which is
needed for SQLite.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:1>
Django
LiveServerTestCase (#20238)
Reporter: Chris Jerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Here is some more info I've collected. For each request, Django's
`ThreadedWSGIServer` calls its `process_request()` method, which lives in
CPython's `ThreadingMixIn`. In this method, `ThreadingMixIn` creates a new
thread with `target=self.process_request_thread`. The
`ThreadingMixIn.process_request_thread()` method looks like this:
{{{#!python
def process_request_thread(self, request, client_address):
"""Same as in BaseServer but as a thread.
In addition, exception handling is done here.
"""
try:
self.finish_request(request, client_address)
except Exception:
self.handle_error(request, client_address)
finally:
self.shutdown_request(request)
}}}
The `shutdown_request()` method has its implementation inside CPython's
`socketserver.TCPServer`. That method looks like this (`close_request()`
is also included here):
{{{#!python
def shutdown_request(self, request):
"""Called to shutdown and close an individual request."""
try:
#explicitly shutdown. socket.close() merely releases
#the socket and waits for GC to perform the actual close.
request.shutdown(socket.SHUT_WR)
except OSError:
pass #some platforms may raise ENOTCONN here
self.close_request(request)
def close_request(self, request):
"""Called to clean up an individual request."""
request.close()
}}}
Thus, if we wanted, database connections could perhaps be closed inside
`close_request()`. This could be done by adding a suitable implementation
to `ThreadedWSGIServer`, thus overriding
`socketserver.TCPServer.close_request()` .
The thread-sharing needed for SQLite could probably also be handled by
adding suitable method overrides to `ThreadedWSGIServer`.
By the way, since `LiveServerThread` currently creates its server with a
hard-coded class like this:
{{{#!python
def _create_server(self):
return ThreadedWSGIServer((self.host, self.port),
QuietWSGIRequestHandler, allow_reuse_address=False)
}}}
it would be easier for users if it instead got the class from a class
attribute, e.g.
{{{#!python
def _create_server(self):
return self.http_server_class((self.host, self.port),
QuietWSGIRequestHandler, allow_reuse_address=False)
}}}
That would let people patch `ThreadedWSGIServer` more easily, if needed.
This is similar to how `LiveServerTestCase` has a `server_thread_class`
class attribute currently set to `LiveServerThread` (with the attribute
added in #26976).
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:2>
Django
LiveServerTestCase (#20238)
Reporter: Chris Jerdonek | Owner: nobody
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
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 Simon Charette):
* type: Uncategorized => Bug
* component: Uncategorized => Testing framework
* stage: Unreviewed => Accepted
Comment:
I haven't reproduced myself but accepting on the basis on the very
detailed analysis.
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:3>
Django
LiveServerTestCase (#20238)
Reporter: Chris Jerdonek | Owner: nobody
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
FYI, I see that the lack of the SQLite thread-sharing logic I mentioned
above has previously been reported here: #29062 (but without a root cause
analysis / proposed fix). I will let that issue know my findings here.
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:4>
Django
after each thread
-----------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:5>
Django
after each thread
-----------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
It looks like this issue's fix might be as simple as adding the following
method to Django's `ThreadedWSGIServer` (code:
https://github.com/django/django/blob/50a5f8840fa564dcefdb1fa5c58f06fcd472ee70/django/core/servers/basehttp.py#L80-L82)
(it seems to be fixing it for me):
{{{#!python
from django.db import connections
def close_request(self, request):
"""Called to clean up an individual request."""
connections.close_all()
super().close_request(request)
}}}
CPython's `socketserver` module documentation confirms the method is meant
to be overridden:
https://github.com/python/cpython/blob/v3.9.1/Lib/socketserver.py#L165-L175
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:6>
Django
after each thread
-----------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
I just filed [https://github.com/django/django/pull/14002 PR #14002] to
let people customize the `ThreadedWSGIServer` used by `LiveServerThread`
more easily. This is what I suggested above in
[https://code.djangoproject.com/ticket/32416#comment:2 this comment].
This will make it easier for people e.g. to implement workarounds whenever
issues like the current one are found with the server used by
`LiveServerThread`. (This is not the first time that the server used
needed to be altered.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:7>
Django
after each thread
-----------------------------------+------------------------------------
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"91c243f80f36b6aa4ae20461b6914b63db81df17" 91c243f8]:
{{{
#!CommitTicketReference repository=""
revision="91c243f80f36b6aa4ae20461b6914b63db81df17"
Refs #32416 -- Added LiveServerThread.server_class to ease subclassing.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:8>
Django
after each thread
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:9>
Django
after each thread
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
I posted a PR for this: https://github.com/django/django/pull/14011
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:10>
Django
after each thread
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:11>
Django
after each thread
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Florian Apolloner (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:12>
Django
after each thread
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
| 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/32416#comment:13>
Django
after each thread
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: Testing framework | Version: 2.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"71a936f9d84864a1420ce3ecc3de4a4908dff1e8" 71a936f]:
{{{
#!CommitTicketReference repository=""
revision="71a936f9d84864a1420ce3ecc3de4a4908dff1e8"
Refs #32416 -- Added LiveServerTestCase._make_connections_override() hook.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:14>
Django
after each thread
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Component: Testing framework | Version: 2.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:"823a9e6bac38d38f7b0347497b833eec732bd384" 823a9e6b]:
{{{
#!CommitTicketReference repository=""
revision="823a9e6bac38d38f7b0347497b833eec732bd384"
Fixed #32416 -- Made ThreadedWSGIServer close connections after each
thread.
ThreadedWSGIServer is used by LiveServerTestCase.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32416#comment:15>