LiveServerTestCase's ThreadedWSGIServer doesn't close database connections after each thread

[Chris Jerdonek]

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,

[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 (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 that LiveServerThread does, which is needed for SQLite.)

[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:

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).

[Simon Charette]

I haven't reproduced myself but accepting on the basis on the very detailed analysis.

[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.

[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):

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

[Chris Jerdonek]

I just filed ​PR #14002 to let people customize the ThreadedWSGIServer used by LiveServerThread more easily. This is what I suggested above in 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.)

[Mariusz Felisiak <felisiak.mariusz@…>]

In 91c243f8:

Refs #32416 -- Added LiveServerThread.server_class to ease subclassing.

[Chris Jerdonek]

I posted a PR for this: ​https://github.com/django/django/pull/14011

[Mariusz Felisiak <felisiak.mariusz@…>]

In 71a936f:

Refs #32416 -- Added LiveServerTestCase._make_connections_override() hook.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 823a9e6b:

Fixed #32416 -- Made ThreadedWSGIServer close connections after each thread.

ThreadedWSGIServer is used by LiveServerTestCase.