GNOME Bugzilla – Bug 684060
global async_free_list is not not protected against multi-thread access
Last modified: 2013-02-20 20:42:06 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.
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?
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?
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.
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.
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()!
I assume that all calls to _pygi_make_native_closure() are GIL protected.
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?
I don't have a test case or anything, this was just from reading the code while tracking down some other crash.
(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.
With GStreamer, we can definitely get python running in another thread while the main thread is doing something else.
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.