Eric Blake
2022-Jun-09 21:26 UTC
[Libguestfs] [libnbd PATCH] python: Correctly use PyGILState
The python docs are clear that in a multi-threaded app, we CANNOT call
any python functions from C code (other than a short-list of
exceptions) without first owning the GIL in the calling thread.
Although many users of nbdsh are single-threaded (where the GIL does
not matter), it is indeed possible to write a python script that
creates python threads to manage h.poll(-1) in one thread while using
other threads to trigger aio_pread and aio_pwrite requests, so
callbacks can be reached from a different thread than the
corresponding thread that initiated the nbd I/O request.
Unfortunately, our generated code was not doing it correctly (for
example, debug_wrapper() to handle h.set_debug_callback() calls
Py_BuildValue outside PyGILState_Ensure()). But it appears that we
have so far been lucky that our callback actions generally occur based
on state machine actions triggered by another Python-based action in
the same thread, coupled with the fact that we have not been
explicitly releasing the GIL around calls into libnbd C API, so in
practice all our callbacks have already owned the GIL in spite of
coding it in the wrong place. However, I cannot rule out the risk of
a mysterious crash in a multi-threaded python program using the nbd
module. Worse, holding the GIL across a potentially-blocking C
operation is bad form; no other Python thread can make progress if we
don't release the GIL.
Fix this by widening the scope of PyGILState in all callback wrappers
(all other code calling into Python is assumed to already own the
GIL), and by dropping the GIL around calls into libnbd C code.
Note that the Python docs also state that manipulation of PyGILState
is incompatible with the use of sub-interpreters with
Py_NewInterpreter(); but if someone actually wants to write a complex
program that uses sub-interpreters to interact with libnbd via Python
independently from some other use of python, rather than just directly
interacting with libnbd via C, I'd be surprised.
Fixes: 936488d4 ("python: Implement Callback properly.", v0.1)
---
generator/Python.ml | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index e94f37b..f196e8e 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -158,7 +158,7 @@ let
pr " const struct user_data *data = user_data;\n";
pr " int ret = -1;\n";
pr "\n";
- pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+ pr " PyGILState_STATE py_save = PyGILState_Ensure();\n";
pr " PyObject *py_args, *py_ret;\n";
List.iter (
function
@@ -227,9 +227,7 @@ let
pr ");\n";
pr " if (!py_args) { PyErr_PrintEx (0); goto out; }\n";
pr "\n";
- pr " py_save = PyGILState_Ensure ();\n";
pr " py_ret = PyObject_CallObject (data->fn, py_args);\n";
- pr " PyGILState_Release (py_save);\n";
pr "\n";
pr " Py_DECREF (py_args);\n";
pr "\n";
@@ -268,6 +266,7 @@ let
| CBUInt _ | CBUInt64 _ -> ()
| CBArrayAndLen _ | CBMutable _ -> assert false
) cbargs;
+ pr " PyGILState_Release (py_save);\n";
pr " return ret;\n";
pr "}\n";
pr "\n"
@@ -478,7 +477,8 @@ let
| BytesPersistOut (n, _) ->
pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto
out;\n" n
| _ -> ()
- ) args;
+ ) args;
+ pr " Py_BEGIN_ALLOW_THREADS\n";
pr " ret = nbd_%s (" name;
pr_wrap ',' (fun () ->
pr "h";
@@ -487,6 +487,7 @@ let
| _, _, n -> pr ", %s" n
) params);
pr ");\n";
+ pr " Py_END_ALLOW_THREADS\n";
List.iter (
function
| Closure { cbname } -> pr " %s_user_data = NULL;\n" cbname
@@ -613,8 +614,10 @@ let
pr " struct user_data *data = user_data;\n";
pr "\n";
pr " if (data) {\n";
+ pr " PyGILState_STATE py_save = PyGILState_Ensure();\n";
pr " Py_XDECREF (data->fn);\n";
pr " Py_XDECREF (data->buf);\n";
+ pr " PyGILState_Release (py_save);\n";
pr " free (data);\n";
pr " }\n";
pr "}\n";
--
2.36.1
Richard W.M. Jones
2022-Jun-10 07:31 UTC
[Libguestfs] [libnbd PATCH] python: Correctly use PyGILState
On Thu, Jun 09, 2022 at 04:26:30PM -0500, Eric Blake wrote:> The python docs are clear that in a multi-threaded app, we CANNOT call > any python functions from C code (other than a short-list of > exceptions) without first owning the GIL in the calling thread. > Although many users of nbdsh are single-threaded (where the GIL does > not matter), it is indeed possible to write a python script that > creates python threads to manage h.poll(-1) in one thread while using > other threads to trigger aio_pread and aio_pwrite requests, so > callbacks can be reached from a different thread than the > corresponding thread that initiated the nbd I/O request. > > Unfortunately, our generated code was not doing it correctly (for > example, debug_wrapper() to handle h.set_debug_callback() calls > Py_BuildValue outside PyGILState_Ensure()). But it appears that we > have so far been lucky that our callback actions generally occur based > on state machine actions triggered by another Python-based action in > the same thread, coupled with the fact that we have not been > explicitly releasing the GIL around calls into libnbd C API, so in > practice all our callbacks have already owned the GIL in spite of > coding it in the wrong place. However, I cannot rule out the risk of > a mysterious crash in a multi-threaded python program using the nbd > module. Worse, holding the GIL across a potentially-blocking C > operation is bad form; no other Python thread can make progress if we > don't release the GIL. > > Fix this by widening the scope of PyGILState in all callback wrappers > (all other code calling into Python is assumed to already own the > GIL), and by dropping the GIL around calls into libnbd C code. > > Note that the Python docs also state that manipulation of PyGILState > is incompatible with the use of sub-interpreters with > Py_NewInterpreter(); but if someone actually wants to write a complex > program that uses sub-interpreters to interact with libnbd via Python > independently from some other use of python, rather than just directly > interacting with libnbd via C, I'd be surprised. > > Fixes: 936488d4 ("python: Implement Callback properly.", v0.1) > --- > generator/Python.ml | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/generator/Python.ml b/generator/Python.ml > index e94f37b..f196e8e 100644 > --- a/generator/Python.ml > +++ b/generator/Python.ml > @@ -158,7 +158,7 @@ let > pr " const struct user_data *data = user_data;\n"; > pr " int ret = -1;\n"; > pr "\n"; > - pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; > + pr " PyGILState_STATE py_save = PyGILState_Ensure();\n"; > pr " PyObject *py_args, *py_ret;\n"; > List.iter ( > function > @@ -227,9 +227,7 @@ let > pr ");\n"; > pr " if (!py_args) { PyErr_PrintEx (0); goto out; }\n"; > pr "\n"; > - pr " py_save = PyGILState_Ensure ();\n"; > pr " py_ret = PyObject_CallObject (data->fn, py_args);\n"; > - pr " PyGILState_Release (py_save);\n"; > pr "\n"; > pr " Py_DECREF (py_args);\n"; > pr "\n"; > @@ -268,6 +266,7 @@ let > | CBUInt _ | CBUInt64 _ -> () > | CBArrayAndLen _ | CBMutable _ -> assert false > ) cbargs; > + pr " PyGILState_Release (py_save);\n"; > pr " return ret;\n"; > pr "}\n"; > pr "\n" > @@ -478,7 +477,8 @@ let > | BytesPersistOut (n, _) -> > pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto out;\n" n > | _ -> () > - ) args; > + ) args; > + pr " Py_BEGIN_ALLOW_THREADS\n"; > pr " ret = nbd_%s (" name; > pr_wrap ',' (fun () -> > pr "h"; > @@ -487,6 +487,7 @@ let > | _, _, n -> pr ", %s" n > ) params); > pr ");\n"; > + pr " Py_END_ALLOW_THREADS\n"; > List.iter ( > function > | Closure { cbname } -> pr " %s_user_data = NULL;\n" cbname > @@ -613,8 +614,10 @@ let > pr " struct user_data *data = user_data;\n"; > pr "\n"; > pr " if (data) {\n"; > + pr " PyGILState_STATE py_save = PyGILState_Ensure();\n"; > pr " Py_XDECREF (data->fn);\n"; > pr " Py_XDECREF (data->buf);\n"; > + pr " PyGILState_Release (py_save);\n"; > pr " free (data);\n"; > pr " }\n"; > pr "}\n";Reviewed-by: Richard W.M. Jones <rjones at redhat.com> although maybe ... https://gitlab.com/nbdkit/nbdkit/-/blob/c96f025b39a3581405845004e1fcceb5dfa04583/plugins/python/plugin.h#L45 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW