Re: [patch] python locking fixes

[Marko Kreen <marko@l-t.ee>]

On Thu, Oct 02, 2003 at 11:48:50PM +0100, Gustavo J. A. M. Carneiro wrote:
> A Qui, 2003-10-02 ās 22:08, Marko Kreen escreveu:
> > Making python lock recursive was not a good idea because
> > it assumed that next callback will be from same plugin.
> > And anyway, the recursive locks were only needed because
> > xchat and python locks were both held at the same time.
> 
>   Are you sure about this? Consider this example plugin:
> 
> import gtk
> 
> def someXchatHook(...):
>     dialog = gtk.Dialog(...)
>     dialog.run()
> 
>   It may run into a deadlock, like this:
> 
> 1. xchat hook triggers plugin
> 2. acquire python interpreter lock
> 3. create gtk dialog
> 4. dialog.run(): runs a recursive gtk main loop; but notice that gtk
> main loop and xchat main loop are one and the same; meanwhile:
>         4a. some xchat event triggers another hook in this plugin
>         4b. acquire python interpreter lock, but
>             -> lock already acquired (by the same thread) => deadlock!

I checked pygtk 2.0.0 sources: it seems to have similar ideas
about locking as patched python.c - dialog.run() drops python
lock before calling gtk_dialog_run.  So no deadlock.

But lets assume python.c uses recursive locking.  So,
entering step 4b without Python lock, python.c thinks it still
has it and wont bother grabbing it.  So it starts doing Python
stuff without valid thread state -> crash.  The recursion
lets xchat get out of sync with reality.


I did a quick research on pygtk locking and it _seems_ the
important signal multiplexing funtions like main, mainloop,
dialog_run use locking similar to my patch so everything should
work.  But there are also some hints of danger - they use
their own variant of 'recursive Python lock' - lots of smaller
funtions like 'set_text' dont do Python locking but may emit
signals so interpreter can be entered recursively.

That still wont give us problems, sequence

    xchat->python(1)->gtk(*)->python(2)

where (*) wont drop python lock wont give us problems,
python(2) can call both gtk and xchat as much it wants.
Problematic sequence is:

    xchat->python(1)->gtk(*)->xchat			[X]

ok, it seems possible to solve it with recursive lock. But now:

    xchat->python->gtk_dialog_run(drops lock)->xchat	[1]

so with or without recursive lock we are pooped, if [X] can
happen.

pygtk source has files THREADING and TODO which talk
about threading but they seem to be out of date, but looking
at them and the source I would say that pygtk authors intent
is also not to let [X] happen and it should be considered
a bug in pygtk.

Only way to let [X] happen safely is when both xchat and
pygtk use _same_ recursive python lock (currently each has its
own housekeeping variables).  It is possible in the future:
starting from Python 2.3 there are such funstions in Python,
PyGILState_Ensure, PyGILState_Release.  And pytgtk 2.2 _may_
start useing them, so then xchat can be converted too (remember,
_both_ need to be using them).

Hmm.  I wanted to continue with "that messes xchat_lock
handling up", but now I wondering if its correct even now,
in light of [1].  Need to think a bit.  But I still dont
see anything wrong in my handling of Python lock.

> > Following patch fixes locking, so that only one of the
> > locks is held at any time - except in one controlled
> > situation.  That means when calling xchat code, python
> > lock is dropped and when calling python code, xchat lock
> > is dropped.  Little bit of code reorg was needed, to better
> > separate xchat and python code.
> 
>   I see what you're trying to do.  You are thinking that if you release
> the python lock before calling xchat functions, then if the plugin
> reenters due to the xchat call, the lock would be free to be acquired
> again.  But you are forgetting that reentry can be caused by pygtk
> functions too.  I don't think there's any other way to make this work
> without a recursive mutex.

But xchat locking is now "correct" in the sense that whether
this works or not depends on pygtk locking.  And if pygtk
does not play fair, then adding recursion to locking does not
help because recursion variables will show nonsense.

>   This is why I wanted to throw away all threading stuff.  This is
> becoming all too complex for my liking :p

Thats true - adding threading to any code does 2 * compexity.
But this patch should make the threading much more consistent
and easily understandable.

>   I personally just want to run pygtk plugins without fear of possible
> deadlock.  Whether the plugins run in separate threads and separate
> globals or not I don't care.
> 
>   I will conclude by stating that I liked your previous patch better.

Well, it was smaller...  But it was rather quick fix to get some
code working, it did not remove bugs from python.c.

-- 
marko

--
XChat-discuss: mailing list for XChat users
Archive:       http://mail.nl.linux.org/xchat-discuss/