After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 684060 - global async_free_list is not not protected against multi-thread access
global async_free_list is not not protected against multi-thread access
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal major
: GNOME 3.8
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2012-09-15 01:43 UTC by Olivier Crête
Modified: 2013-02-20 20:42 UTC
See Also:
GNOME target: ---
GNOME version: 3.5/3.6


Attachments
pygi-closure: Protect the GSList with the GIL (1.08 KB, patch)
2012-09-15 01:43 UTC, Olivier Crête
committed Details | Review

Description Olivier Crête 2012-09-15 01:43:17 UTC
Created attachment 224378 [details] [review]
pygi-closure: Protect the GSList with the GIL

The global GList isn't protected against multi-thread access. Just putting it one line up inside the GIL should guarantee thread safety. I guess if you're unlucky, this could cause a crash.
Comment 1 Paolo Borelli 2012-09-15 10:18:27 UTC
Review of attachment 224378 [details] [review]:

If we protect adding to the list, I guess we would also need to protect actual freeing of the list, no?
Comment 2 Martin Pitt 2012-09-17 09:17:11 UTC
PyGILState_{Ensure,Release} are only documented to provide locking for calling Python code from C (http://docs.python.org/c-api/init.html), which is what the current code does. It protects calling the callbacks with the closures (in _pygi_closure_handle) as well as freeing the closures (_pygi_invoke_closure_free) with those.

For thread-safe handling of the async_free_list GSList itself, do PyGILState_{Ensure,Release} really provide thread safety at the C level, or shouldn't these rather use a GMutex?
Comment 3 Olivier Crête 2012-09-17 14:19:49 UTC
After calling  PyEval_InitThreads(), Release/Restore does two things: take a lock and set the Current pointer to point to the current threadstate inside the python interpreter. So I don't think we need an extra lock. Btw, it may make sense to call GLib.thread_init() in the main initialisation function now as GLib is always thread-enabled these days.
Comment 4 Paolo Borelli 2012-09-17 14:27:21 UTC
glib_thread_init is a noop these days and should not be called and at a quick look GLib.thread_init is not an override which does something extra, so I do not think we should call it.


It is still not clear to me if multithread access to that list can actually happen, but if it does, it would probably be more elegant to use a GAsyncQueue to hold the closures to free.
Comment 5 Olivier Crête 2012-09-17 14:52:29 UTC
That is also what I though, but I discovered that it is secretely an override which does something extra. It defined at http://git.gnome.org/browse/pygobject/tree/gi/_glib/glibmodule.c#n92 and it calls pyglib_enable_threads() which calls PyEval_InitThreads()!
Comment 6 Olivier Crête 2012-09-17 14:58:07 UTC
I assume that all calls to _pygi_make_native_closure() are GIL protected.
Comment 7 Martin Pitt 2013-01-14 08:59:17 UTC
Comment on attachment 224378 [details] [review]
pygi-closure: Protect the GSList with the GIL

I still have some doubts about this patch as to whether it actually does something. Is there evidence that it actually fixes some race condition with threads?
Comment 8 Olivier Crête 2013-01-15 00:57:43 UTC
I don't have a test case or anything, this was just from reading the code while tracking down some other crash.
Comment 9 Simon Feltman 2013-01-15 23:48:15 UTC
(In reply to comment #6)
> I assume that all calls to _pygi_make_native_closure() are GIL protected.

They should be. The code paths reaching it are either a "from py" marshaler or vfunc hookup. Both of which are only called from Python. So the clearing of "async_free_list" is protected in that sense.

(In reply to comment #8)
> I don't have a test case or anything, this was just from reading the code while
> tracking down some other crash.

The proposed patch looks like it could block a case where an async callback is finishing and adding itself to the async_free_list and a new callback is simultaneously being created by _pygi_make_native_closure. This would require the async callback/glib to be running in one thread while python is running in another.
Comment 10 Olivier Crête 2013-01-16 00:03:13 UTC
With GStreamer, we can definitely get python running in another thread while the main thread is doing something else.
Comment 11 Simon Feltman 2013-02-20 20:42:04 UTC
Committed as this seems right to me. The async_free_list is only accessed within the GIL
in other places which is why this works.