Created attachment 110243 [details] [review]
patch

Fixes the reported bugs and 1) repairs indentation; 2) more importantly, fix g_object_thaw_notify() not being called after an error.

Note that the fix is untested except for that it does compile fine.  Please attach test if you have one.

Created attachment 110244 [details] [review]
release the pil on get_property() too

You patch works well, I'm attaching a second patch to release it on get_property() calls too.

FWIW, did you test the speed impact of your patch?  I dunno, but locks do sound slow and properties are got often.

I didn't test it.. I'm not certain how to do that exactly
But slowness is better than deadlocks. And a gobject property getter could very well do something that causes it to go back into python code.

I admit I don't fully understand Python thread/GIL, but it seems deadlocks can occur if something uses PyEval_AcquireLock().  What is the difference with PyGILState_Ensure()?  The latter doesn't deadlock AFAIK.

The problem in my app is that in one thread I have my C GObject hold a lock while it emits a signal (handled by a python handler), but in a second thread, I get a property in the same object (which takes the lock)...
So Thread 1 has the C lock and wants python lock, while thread 2 has the python lock but wants the C lock... instant deadlock.

From what I understand of the GIL/GObject interaction, it should be released when any foreign code is called (because you don't know the locking rules of the foreign code).

Johan, would be useful if you suggested which exactly work it needs.  Should allow threads be right around g_object_set_property?  (Because at least PyDict_Next is moved inside allow_threads in the patch and that is likely wrong.)

Paul: I don't actually know. I haven't touched that part in a long while.
Maybe Gustavo or Edward Hervey would know better.

(In reply to comment #7)
> Johan, would be useful if you suggested which exactly work it needs.  Should
> allow threads be right around g_object_set_property?  (Because at least
> PyDict_Next is moved inside allow_threads in the patch and that is likely
> wrong.)

Yes, calling PyDict_Next or nearly any python api call when the GIL is not held is illegal.  You might also worry about the dictionary being modified when the thread doesn't hold the GIL -- though this could actually happen without the GIL release if there's a callback into python code.

Maybe it then makes sense to have two loops --- one collecting and checking, with Python C API calls, the second with GIL release, notifications frozen and containing _only_ set_property on a precalculated data?  This way we will not release GIL more than once and also escape the potential problem you outlined.  (OTOH I don't know what Python uses for kwargs calls --- maybe it is a copy which nothing can modify as nothing has a binding to this dict?)

I'm not overly concerned about performance; optimization is probably premature.  One possibility is to do the equivalent to kw.items() and iterate over the resulting list.  There are probably more clever ways of doing this, but using items() is probably the simplest -- another possibility is using iteritems() and being prepared for the exception when the dict changes.

Created attachment 110339 [details] [review]
patch for set_properties with more code reusal

How about this approach of just calling set_property_from_pspec() then?  It does GIL release, does additional error checking so we are more consistent with set_property, etc.  Also, this reduces code duplication.

It seems that we needn't worry about dictionary mutating:
>>> x = { 'a': 1, 'b': 2 }
>>> def foo (**kwargs):
...     print kwargs is x
...
>>> foo (**x)
False

(In reply to comment #12)
> Created an attachment (id=110339) [edit]
> patch for set_properties with more code reusal
> 
> How about this approach of just calling set_property_from_pspec() then?  It
> does GIL release, does additional error checking so we are more consistent with
> set_property, etc.  Also, this reduces code duplication.

This looks better -- my personal stylistic preference would be to drop the common exception or no exception block, but that may just be me.

> It seems that we needn't worry about dictionary mutating:
> >>> x = { 'a': 1, 'b': 2 }
> >>> def foo (**kwargs):
> ...     print kwargs is x
> ...
> >>> foo (**x)
> False

OK, it is a corner case that this would be a problem in any case.

Can we can this merged already ? Deadlocking is really bad...

Created attachment 118390 [details] [review]
patch for set_properties with more code reusal (rediff)

Previously patch no longer applies.  I applied it by hands and rediffed.  New patch is committed now.  I don't want to commit the other patch for getting property --- I'm not convinced it is cheap enough and don't want potentially problematic changes now.

Sending        ChangeLog
Sending        gobject/pygobject.c
Transmitting file data ..
Committed revision 970.

2008-09-10  Paul Pogonyshev  <pogonyshev@gmx.net>

	Bug 530935 – pygobject_set_properties doesnt release the GIL

	* gobject/pygobject.c (pygobject_set_properties): Reuse
	set_property_from_pspec() which release GIL for us.  Also make
	sure that g_object_thaw_notify() is called even after error.

Leaving open since the problem with get_property() is not solved.

Performance should not be used an an excuse to create a deadlock...

AIUI, the GIL is taken and released an awful lot when calling any C code from Python, so performance should not be a concern. I agree that correctness is a far greater concern.

Is there any concern over the correctness of Olivier's patch?

I am curious though why we're only running into this bug now. Lots of Python code must set GObject properties, even in different threads -- any GStreamer code, for instance, is likely to do this.

I think the main difference is that the GstObject lock is not recursive, so its always released before calling back into python code (the Farsight2 code uses a recursive lock, so its sometimes held during some signals calls like FsStream::src-pad-added).

(In reply to comment #18)
> AIUI, the GIL is taken and released an awful lot when calling any C code from
> Python, so performance should not be a concern.

I have to correct you on this one.  Almost all C function wrappers do _not_ take/release the GIL.  Releasing GIL around a function call is the exception and not the rule.  Only when C code calls into Python then the GIL is acquired and released, always.

But I must say that the overhead of releasing/acquiring the GIL is probably small compared to the C code being executed, in this particular case.  Anything that manipulates GValues tends to be slower than simple C function calls.  So +1 from me.

Gustavo: thanks for the clarification.

The problem here is not that the C code calls into Python, but that there is no consistent locking order otherwise.

Thread 1 calls function F from Python while holding the GIL. Function F takes lock L.
Thread 2 has lock L and calls function G which causes Python code to be called. Function G takes the GIL.

In this case, F = g_object_get_property, G = g_signal_emit, L = the FsRtpSession lock.

I don't see any other solution than releasing the GIL before calling F.

Any progress on this bug? I'm hitting this issue quite hard in some piece of software i'd like to release and telling people to patch their pygobject is a bit suboptimal :)

The same issue also arises when calling _unref with the GIL locked. I'll attach an updated patch (against current svn), which also fixes the issue on _unref

Created attachment 123482 [details] [review]
Release the GIL when calling into various C function that can have non-trivial interactions

In the same category, pygbject_clear() should also release the lock when releasing the toggle ref... Actually, you should release the lock whenever you unref an object (since it will call into foreign code that may very well have its own locking).

I have the following deadlock. thread a does a gst state change, which causes a message to be emitted on the GstBus, which triggers the synchronous  handler, which calls back into python (so I have.. first the state lock -> GIL). In another thread, I try to stop the same part of the pipeline from python code, so I have GIL -> state lock.. Result: deadlock.

Based on the comments from John and Gustavo I'm pushing the patch from Sjoerd.

If this particular fix actually affects performance for some particular app, the author should provide profiling data in a different ticket.