Resolve threading issue with gil_scoped_acquire

[davidhewitt]

It was possible for python threads to execute c++ code which would subsequently call gil_scoped_acquire, incorrectly conclude that the gil was not held, and then deadlock.

The most basic example I can build of the failing code is:

#include <pybind11/pybind11.h>
#include <pybind11/functional.h>

PYBIND11_MODULE(pybind11test, m)
{
  m.def("deadlock", [] (std::function<void ()> f) { f(); });
}

and then invoked in python as so:

import pybind11test

import threading

thread1 = threading.Thread(target=pybind11test.deadlock, args=(lambda: None,))
thread1.start()

thread2 = threading.Thread(target=pybind11test.deadlock, args=(lambda: None,))
thread2.start() 

thread1.join()
thread2.join()

I believe that this patch will resolve both #1525 and #1308.

To me this is a critical problem, as I often work with python threads. I would be very grateful if this can be reviewed and merged in time for the pybind11 2.3 release.

I am happy to make any changes you feel necessary, including writing a different solution altogether if you have a better one in mind.

Thanks in advance,
David

[davidhewitt]

I wanted to write a test case, but I am unsure if adding code than can potentially deadlock the test suite is wise. Other solutions warranted, can remove the test case altogether if preferred.

[davidhewitt]

Looks like at least on CI the deadlock is handled cleanly!

[davidhewitt]

Looks like this has only worked for Python 3, not 2 (my dev box is Python 3).

I'd like to give this another shot that works for both platforms. Would be great if anyone familar with this part of the codebase can weigh in on whether I'm heading on the right path before I spend too much time on this.

[awaizman1]

Hi David,

This PR is critical to me too, I found out that running my C-extensions which acquire gil from non-main python thread dead-lock.

Do you know when this PR will be accepted?

Thanks a lot!

[davidhewitt]

Hi @awaizman1, at the moment I'm avoiding threads as much as possible and other workarounds, but I'm sure this is going to really bite me soon.

I'd love to push forward with further development and get it fixed but I'm waiting to see if we can get any interest from the pybind11 maintainers first...

[uentity]

@davidhewitt I'm sure that in the first place we should consider helping each other (and giving ideas) to resolve issues and walk around corner cases we face as users of this great library.
And also I believe in rule that maintainer's interest has strong positive correlation with users' interest 😃

So can't wait for this PR to be finished.

[davidhewitt]

I've just seen that the master branch has recieved a merge from #1211 which solves exactly what I do here. As such, I'm going to close this PR. Wish I'd managed to discover #1211 previously!