xchat-guile and threads

xchat-guile and threads

[Zeeshan Ali (Khattak)]

Hi all!
   I am looking back at the code i wrote[1] some years back and I
don't seem to recall why i did somethings the way i did. One thing
that i can't remember is why I am doing threads and locking in there.
Given some thought, i called reading python plugin code a bit before
writing xchat-guile so i looked at the source and found this comment:

/* Thread support
 * ==============
 *
 * The python interpreter has a global interpreter lock. Any thread
 * executing must acquire it before working with data accessible from
 * python code. Here we must also care about xchat not being
 * thread-safe. We do this by using an xchat lock, which protects
 * xchat instructions from being executed out of time (when this
 * plugin is not "active").
 *
 * When xchat calls python code:
 *   - Change the current_plugin for the executing plugin;
 *   - Release xchat lock
 *   - Acquire the global interpreter lock
 *   - Make the python call
 *   - Release the global interpreter lock
 *   - Acquire xchat lock
 *
 * When python code calls xchat:
 *   - Release the global interpreter lock
 *   - Acquire xchat lock
 *   - Restore context, if necessary
 *   - Make the xchat call
 *   - Release xchat lock
 *   - Acquire the global interpreter lock
 *
 * Inside a timer, so that individual threads have a chance to run:
 *   - Release xchat lock
 *   - Go ahead threads. Have a nice time!
 *   - Acquire xchat lock
 *
 */

  So in those very early days of my flying lessons, I might have just
assumed I need to do the same in guile plugin as well. If someone
knows some better reason I did what I did, please let me know before i
start removing all the ugly thread stuff and learning the importance
of it the hard way.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

[1] http://gitorious.org/projects/xchat-guile/repos/mainline/blobs/master/guile.c
     http://gitorious.org/projects/xchat-guile/repos/mainline/blobs/master/plugin-system.scm

Re: xchat-guile and threads

[Neil Jerram]

2009/1/21 Zeeshan Ali (Khattak) <zeenix@gmail.com>:
> Hi all!
>   I am looking back at the code i wrote[1] some years back and I
> don't seem to recall why i did somethings the way i did. One thing
> that i can't remember is why I am doing threads and locking in there.
> Given some thought, i called reading python plugin code a bit before
> writing xchat-guile so i looked at the source and found this comment:
[...]
>  So in those very early days of my flying lessons, I might have just
> assumed I need to do the same in guile plugin as well.

That sounds believable to me.  Guile definitively doesn't have
anything like a global interpreter lock; in theory it is possible to
call the SCM API concurrently from as many threads as you like, and
libguile will use fine-grained locks to protect anything that needs
protecting.  In practice, however, there are probably some bugs
lurking...

It's better overall if we find and fix those bugs.  However, just in
case they become unmanageable, you might want to keep your global lock
around as a fallback,

Regards,
       Neil

Re: xchat-guile and threads

[Linas Vepstas]

2009/1/22 Neil Jerram <neiljerram@googlemail.com>:
> in theory it is possible to
> call the SCM API concurrently from as many threads as you like, and
> libguile will use fine-grained locks to protect anything that needs
> protecting.  In practice, however, there are probably some bugs
> lurking...

Well, I'm happy to report that I don't think there
are many left.  There are one or two patches
that I submitted that unfortunately didn't make it
into 1.8.6 -- I resubmitted these, but I haven't
heard back from anyone indicating that they've
been applied (or that they've been explicitly
rejected either).

The biggest outstanding bug is the race
condition in the environment hash tables
that are updated when there are multiple
defines happening in parallel -- the symptom
is that some of the defines "don't stick",
as if they didn't happen.

I think Ludovic understands it well, and has
a good proposal on how to fix it. Unfortunately,
he's not describing an easy fix.

I looked at it but the problem seems to be deep,
with no easy quick fix/work-around. I contemplated
adding a brute force lock around scm_define(),
to serialize defines, but decided that this wasn't
enough, or wouldn't work, or wasn't a 100% solution.
(I don't quite remember the details -- I think the
problem is that other threads might still be reading
or even updating the ash table, outside of scm_define())

The other alternative, adding a lock on all hash table
access, would have been punitive in performance.
Or something like that.

Anyway, with the one or two patches, and a fix for
the define problem, would fix all of the bugs I've seen
-- I've carefully analyzed dozens, and haven't seen
any that are not due to one of these bugs.

--linas