Fixed #30171 -- Made DatabaseWrapper thread sharing logic reentrant.

[jdufresne]

Changed to use a reference counting like scheme to allow nested uses. To increment the reference count, use the method DatabaseWrapper.inc_thread_sharing(). To decrement it, use the method
DatabaseWrapper.dec_thread_sharing(). DatabaseWrapper.allow_thread_sharing is True if the reference count is greater than zero.

This change obviates the need for passing the value of `allow_thread_sharing to the constructor, so it has been removed.

Fixed the following warning during LiveServerTestCase:

    Exception in thread Thread-16:
    Traceback (most recent call last):
      File "/usr/lib64/python3.7/threading.py", line 917, in _bootstrap_inner
        self.run()
      File "django/test/testcases.py", line 1399, in run
        connections.close_all()
      File "django/db/utils.py", line 224, in close_all
        connection.close()
      File "django/db/backends/sqlite3/base.py", line 244, in close
        self.validate_thread_sharing()
      File "django/db/backends/base/base.py", line 531, in validate_thread_sharing
        % (self.alias, self._thread_ident, _thread.get_ident())
    django.db.utils.DatabaseError: DatabaseWrapper objects created in a thread can only be used in that same thread. The object with alias 'default' was created in thread id 140092344866304 and this is thread id 140091929347840.

https://code.djangoproject.com/ticket/30171

[jdufresne]

I'm very confused as to why this change would result in a segfault. I'll keep digging, but have been unable to reproduce locally so far on Fedora 29. If anyone has any ideas, I'm interested to hear them.

[timgraham]

Do you see any test failures on your machine? I can't reproduce the seg fault either but I have a number of test failures on my Ubuntu 18.04 machine (SQLite 3.22).

[jdufresne]

I used an environment with an older sqlite3 version and I have been able to reproduce locally and even got a segfault once, so I have something to work with.

[jdufresne]

I have fixed the test failures by adding the line self._thread_sharing_count = 0 to dec_thread_sharing(). I had an alternative thought of setting the connection as "unusable" if _thread_sharing_count falls below zero as it would clearly indicate resource mismanagement and an unknown state. Thoughts?

[charettes]

I had an alternative thought of setting the connection as "unusable" if _thread_sharing_count falls below zero as it would clearly indicate resource mismanagement and an unknown state. Thoughts?

Hmm isn't it a bit odd that setting it back to 0 fixes the segfault in the first place? Did you figure it out? What about raising the ValueError before decrementing if self._thread_sharing_count == 0.

Also, shouldn't _thread_sharing_count alterations be performed within RLock to implement some form of reverse semaphore?

Also, the commit message mentions To decrement it, use the method DatabaseWrapper.inc_thread_sharing().

[jdufresne]

Hmm isn't it a bit odd that setting it back to 0 fixes the segfault in the first place? Did you figure it out?

The newly added test, tests.backends.ThreadTests.test_thread_sharing_count(), finishes by calling an extra .dec_thread_sharing() which previously put the count in an invalid state. Assigning it back to zero restored a valid state.

I like the suggestion of checking the count before decrementing and will make that update. Thanks!

Also, shouldn't _thread_sharing_count alterations be performed within RLock to implement some form of reverse semaphore?

Can you clarify what you mean. Are you suggesting something like this?

    @property
    def allow_thread_sharing(self):
        with self._thread_sharing_lock:
            return self._thread_sharing_count > 0

    def inc_thread_sharing(self):
        with self._thread_sharing_lock:
            self._thread_sharing_count += 1

    def dec_thread_sharing(self):
        with self._thread_sharing_lock:
            if self._thread_sharing_count <= 0:
                raise RuntimeError(
                    'The DatabaseWrapper thread sharing count was decremented below zero.'
                )
            self._thread_sharing_count -= 1

Or do you have something else in mind? I'm open to this but can you briefly explain what potential scenario this will solve for my benefit?

[charettes]

The newly added test, tests.backends.ThreadTests.test_thread_sharing_count(), finishes by calling an extra .dec_thread_sharing() which previously put the count in an invalid state. Assigning it back to zero restored a valid state.

Makes sense, thanks for the clarifications!

Can you clarify what you mean. Are you suggesting something like this?

yeah, that's what I had in mind.

Or do you have something else in mind? I'm open to this but can you briefly explain what potential scenario this will solve for my benefit?

I might be missing something but I was under the impression a threading lock was required to make sure alterations of the counter were thread safe since the connection was meant to be shared between threads.

[timgraham]

I've seen the warning when running the Django test suite. In particular, it seems that servers tests reliably give that warning after 8c77539. I'd be okay with backporting to 2.2 if it doesn't seem too risky.

[timgraham]

Use present tense:

* The third argument of ``DatabaseWrapper.__init__()``,
  ``allow_thread_sharing``, is removed.

[charettes]

I'd be okay with backporting to 2.2 if it doesn't seem too risky.

It should be pretty safe to backport IMO.

[jdufresne]

Is RLock necessary or will just Lock do? If I understand the docs correctly, Lock is simpler and the lock is never acquired twice in a single thread.

[charettes]

You are right, Lock should be sufficient.

[jdufresne]

Thanks. I've amended Tim's edits to change RLock to Lock.