Add option to use the PyGILState API in gil_scoped_acquire (#1276)

[KyleFromKitware]

No description provided.

[jagerman]

This patch seems rather complicated (going via the global option). It also has the problem that the global setting could get you into trouble: for example, { gil_scoped_acquire gil; f(); } where f() invokes some Python code that loads another pybind module that changes the global option. Or, similarly, the order in which you load modules ends up dictating whether a feature in one module (relying on pybind's advanced gil abilities) works or not. Load A, or B followed by A and A works; load A followed by B and A stops working.

I think a simpler approach would be to entirely isolate the basic Python API approach into its own RAII class. Call them, perhaps, basic_gil_acquire and basic_gil_release, which use the vanilla API.

[KyleFromKitware]

One of the things I found during testing of this patch is that each shared library or executable gets its own copy of the global options singleton, because it's declared in a header and not exported - different modules have no way of knowing that the others are using pybind11. That said, perhaps you're right that adding new classes would be the better approach, though this would also involve making new PYBIND11_OVERLOAD() macros that use the new classes too.

[jagerman]

Oh, right, I forgot about the macros. The number of different macros there is already getting unwieldy; doubling them up doesn't sound nice at all. (I'd like to somehow replace all those macros, but I don't yet have an idea of how to go about eliminating them--mainly because, at least right now, the macro needs to be able to return from the calling code).

That said, there's another advantage of having a different class: it ought to be possible to detect a deadlock when mixing implementations (like the one you ran into originally), at least in some cases, and raise an exception instead.

[KyleFromKitware]

What about just leaving it up to the caller to do the actual return? Example:

voidmy_class::some_func(int arg) {
returnpybind11::overload(this, "some_func", arg);
}

That's just an example, it's nowhere near complete, and it probably wouldn't actually look like that. It seems to me the issue is more with templated functions being more unwieldy than macros.

[KyleFromKitware]

We found another case for making a global configuration option: error_already_set. It calls gil_scoped_acquire in the destructor. We were able to redefine our own versions of the PYBIND11_OVERLOAD() macros to use the basic PyGILState_* API, but error_already_set is used extensively throughout pybind, so making a new version of it isn't feasible without simply forking pybind altogether.

[KyleFromKitware]

Here is a simple example that illustrates the problem with error_already_set:

#include<pybind11/pybind11.h>
#include<pybind11/embed.h>
#include<iostream>classbasic_gil_scoped_acquire
{
public:basic_gil_scoped_acquire()
{
state = PyGILState_Ensure();
}
~basic_gil_scoped_acquire()
{
PyGILState_Release(state);
}
private:
PyGILState_STATE state;
};
classbasic_gil_scoped_release
{
public:basic_gil_scoped_release()
{
state = PyEval_SaveThread();
}
~basic_gil_scoped_release()
{
PyEval_RestoreThread(state);
}
private:
PyThreadState *state;
};
staticconstchar pycode[] =
"raise Exception('This is a test')";
intmain(int argc, char **argv)
{
pybind11::scoped_interpreter interp;
basic_gil_scoped_release release;
try
{
basic_gil_scoped_acquire acquire;
pybind11::exec(pycode);
}
catch (pybind11::error_already_set &e)
{
std::cout << e.what() << std::endl;
}
return0;
}

This will segfault when the error_already_set is destructed because of the above issue.

[scdub]

@KyleFromKitware is there anything that I can do to help move this PR forward? In embedded Python deployments this continues to prevent working with libraries that use the gil_scoped functions.

[rwgk]

Just to log: I looked, and I have the same worries as @jagerman (Feb 17, 2018).

Threading is a can of worms in general. Mixing in the pitfalls of pybind11::options is likely to make it worse.

Another thought, which may be naive: could the proposed basic_gil_acquire become the default (under the old name). And the old implementation moved to a separate RAII type?

It doesn't solve the issue with the OVERRIDE macros.