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:1 by Chris Jerdonek, 4 years ago

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

Last edited 4 years ago by Chris Jerdonek (previous) (diff)

comment:2 by Chris Jerdonek, 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 Simon Charette, 4 years ago

Component: UncategorizedTesting framework
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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

comment:4 by Chris Jerdonek, 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 Chris Jerdonek, 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 Chris Jerdonek, 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 Chris Jerdonek, 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.)

Last edited 4 years ago by Chris Jerdonek (previous) (diff)

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 91c243f8:

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

comment:9 by Chris Jerdonek, 4 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:10 by Chris Jerdonek, 4 years ago

comment:11 by Chris Jerdonek, 4 years ago

Has patch: set

comment:12 by Mariusz Felisiak, 4 years ago

Cc: Florian Apolloner added

comment:13 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 71a936f:

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

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

Resolution: fixed
Status: assignedclosed

In 823a9e6b:

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

ThreadedWSGIServer is used by LiveServerTestCase.

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