Opened 4 years ago
Closed 4 years ago
#32416 closed Bug (fixed)
LiveServerTestCase's ThreadedWSGIServer doesn't close database connections after each thread
Reported by: | Chris Jerdonek | Owned by: | Chris Jerdonek |
---|---|---|---|
Component: | Testing framework | Version: | 2.2 |
Severity: | Normal | Keywords: | |
Cc: | Florian Apolloner | 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
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 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:
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 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 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,
Change History (15)
comment:2 by , 4 years ago
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:
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):
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:
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.
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).
comment:3 by , 4 years ago
Component: | Uncategorized → Testing framework |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
I haven't reproduced myself but accepting on the basis on the very detailed analysis.
comment:4 by , 4 years ago
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.
comment:5 by , 4 years ago
Summary: | Apparent regression of #22414 from switching to ThreadedWSGIServer in LiveServerTestCase (#20238) → LiveServerTestCase's ThreadedWSGIServer doesn't close database connections after each thread |
---|
comment:6 by , 4 years ago
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):
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
comment:7 by , 4 years ago
I just filed PR #14002 to let people customize the ThreadedWSGIServer
used by LiveServerThread
more easily. This is what I suggested above at the end of 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.)
comment:9 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 4 years ago
Has patch: | set |
---|
comment:12 by , 4 years ago
Cc: | added |
---|
comment:13 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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. likeLiveServerThread
does.Here is the code in CPython for
ThreadingMixIn
'sprocess_request()
andserver_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 (which was the change made for #22414):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 thatLiveServerThread
does, which is needed for SQLite.)