Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve threading issue with gil_scoped_acquire #1556

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Contributor

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

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.


def test_no_deadlock():
# This code would deadlock - see issue 1525
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@davidhewitt
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor

uentity commented Dec 1, 2018

@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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants