[vdsm] Fwd: Fwd: suggested patch for python-pthreading

ybronhei ybronhei at redhat.com
Tue Feb 25 06:59:06 UTC 2014


back to the topic
I would like to continue pushing the fix in.

Nir, iirc you suggested a patch to validate that 
pthreading.monkey_patch() is being called before using the native thread 
and pthreading module, right?
any comments about that part?



-------- Original Message --------
Subject: Fwd: [vdsm] suggested patch for python-pthreading
Date: Tue, 18 Feb 2014 09:32:59 -0500 (EST)
From: Nir Soffer <nsoffer at redhat.com>
To: Yaniv Bronheim <ybronhei at redhat.com>



----- Forwarded Message -----
> From: "Yaniv Bronheim" <ybronhei at redhat.com>
> To: "Nir Soffer" <nsoffer at redhat.com>
> Cc: "Dan Kenigsberg" <danken at redhat.com>, "VDSM Project Development" <vdsm-devel at lists.fedorahosted.org>
> Sent: Wednesday, February 5, 2014 6:41:28 PM
> Subject: Re: [vdsm] suggested patch for python-pthreading
>
> ----- Original Message -----
> > From: "Nir Soffer" <nsoffer at redhat.com>
> > To: "Dan Kenigsberg" <danken at redhat.com>
> > Cc: "Yaniv Bronheim" <ybronhei at redhat.com>, "VDSM Project Development"
> > <vdsm-devel at lists.fedorahosted.org>
> > Sent: Tuesday, February 4, 2014 4:39:55 PM
> > Subject: Re: [vdsm] suggested patch for python-pthreading
> >
> > ----- Original Message -----
> > > From: "Dan Kenigsberg" <danken at redhat.com>
> > > To: "Nir Soffer" <nsoffer at redhat.com>
> > > Cc: "Yaniv Bronheim" <ybronhei at redhat.com>, "VDSM Project Development"
> > > <vdsm-devel at lists.fedorahosted.org>
> > > Sent: Tuesday, February 4, 2014 3:51:02 PM
> > > Subject: Re: [vdsm] suggested patch for python-pthreading
> > >
> > > On Tue, Feb 04, 2014 at 05:14:51AM -0500, Nir Soffer wrote:
> > > > ----- Original Message -----
> > > > > From: "Yaniv Bronheim" <ybronhei at redhat.com>
> > > > > To: "Nir Soffer" <nsoffer at redhat.com>
> > > > > Cc: "VDSM Project Development" <vdsm-devel at lists.fedorahosted.org>
> > > > > Sent: Tuesday, February 4, 2014 11:53:52 AM
> > > > > Subject: Re: [vdsm] suggested patch for python-pthreading
> > > > >
> > > > >
> > > > >
> > > > > ----- Original Message -----
> > > > > > From: "Nir Soffer" <nsoffer at redhat.com>
> > > > > > To: "Yaniv Bronheim" <ybronhei at redhat.com>
> > > > > > Cc: "VDSM Project Development" <vdsm-devel at lists.fedorahosted.org>
> > > > > > Sent: Tuesday, February 4, 2014 11:39:56 AM
> > > > > > Subject: Re: [vdsm] suggested patch for python-pthreading
> > > > > >
> > > > > > ----- Original Message -----
> > > > > > > From: "Yaniv Bronheim" <ybronhei at redhat.com>
> > > > > > > To: "VDSM Project Development"
> > > > > > > <vdsm-devel at lists.fedorahosted.org>
> > > > > > > Sent: Tuesday, February 4, 2014 11:04:37 AM
> > > > > > > Subject: [vdsm] suggested patch for python-pthreading
> > > > > > >
> > > > > > > according to coredumps we found in the scope of the bug [1] we
> > > > > > > opened
> > > > > > > [2]
> > > > > > > that suggested to override python's implementation of
> > > > > > > thread.allocate_lock
> > > > > > > in each coredump we saw few threads stuck with the bt:
> > > > > > >
> > > > > > > #16 0x00007fcb69288c93 in PyEval_CallObjectWithKeywords
> > > > > > > (func=0x2527820,
> > > > > > > arg=0x7fcb6972f050, kw=<value optimized out>) at
> > > > > > > Python/ceval.c:3663
> > > > > > > #17 0x00007fcb692ba7ba in t_bootstrap (boot_raw=0x250a820) at
> > > > > > > Modules/threadmodule.c:428
> > > > > > > #18 0x00007fcb68fa3851 in start_thread (arg=0x7fcb1bfff700) at
> > > > > > > pthread_create.c:301
> > > > > > > #19 0x00007fcb6866694d in clone () at
> > > > > > > ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
> > > > > > >
> > > > > > > in pystack the threads were stuck in
> > > > > > > /usr/lib64/python2.6/threading.py
> > > > > > > (513): __bootstrap_inner
> > > > > > >
> > > > > > > in bootstrap_inner we use thread.allocate_lock which
> > > > > > > python-pthreading
> > > > > > > does
> > > > > > > not override.
> > > > > > >
> > > > > > > we suggest the following commit:
> > > > > > >
> > > > > > > From 9d89e9be1a379b3d93b23dd54a381b9ca0973ebc Mon Sep 17 00:00:00
> > > > > > > 2001
> > > > > > > From: Yaniv Bronhaim <ybronhei at redhat.com>
> > > > > > > Date: Mon, 3 Feb 2014 19:24:30 +0200
> > > > > > > Subject: [PATCH] Mocking thread.allocate_lock with Lock imp
> > > > > > >
> > > > > > > Signed-off-by: Yaniv Bronhaim <ybronhei at redhat.com>
> > > > > > > ---
> > > > > > >  pthreading.py | 4 ++++
> > > > > > >  1 file changed, 4 insertions(+)
> > > > > > >
> > > > > > > diff --git a/pthreading.py b/pthreading.py
> > > > > > > index 916ca7f..96df42c 100644
> > > > > > > --- a/pthreading.py
> > > > > > > +++ b/pthreading.py
> > > > > > > @@ -132,6 +132,10 @@ def monkey_patch():
> > > > > > >      Thus, Queue and SocketServer can easily enjoy them.
> > > > > > >      """
> > > > > > >
> > > > > > > +    import thread
> > > > > > > +
> > > > > > > +    thread.allocate_lock = Lock
> > > > > > > +
> > > > > > >      import threading
> > > > > > >
> > > > > > >      threading.Condition = Condition
> > > > > > > --
> > > > > > > 1.8.3.1
> > > > > > >
> > > > > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1022036
> > > > > > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1060749
> > > > > > >
> > > > > >
> > > > > > Replacing allocate_lock in thread is correct. However, since
> > > > > > threading
> > > > > > copies
> > > > > > thread.allocate_lock, and you don't control import order, you have
> > > > > > to
> > > > > > monkeypatch
> > > > > > threading.allocate_lock as well.
> > > > > >
> > > > > > The full should be:
> > > > > >
> > > > > >   import thread
> > > > > >   thread.allocate_lock = Lock
> > > > > >
> > > > > >   import threading
> > > > > >   threading.allocate_lock = Lock
> > > > > >   threading.Lock = Lock
> > > > > >
> > > > > > Nir
> > > > > >
> > > > >
> > > > > thanks Nir for the reply and review
> > > > > I guess you meant threading._allocate_lock, but we monkey patch it in
> > > > > an
> > > > > order so we do control the import order.. not sure its necessary
> > > >
> > > > Correct, threading._allocate_lock. And we also have to patch
> > > > threading._active_limbo_lock.
> > > >
> > > > We don't control the user import order - If I import threading before
> > > > pthreading, both
> > > > threading._allocate_lock and threading._active_limbo_lock are using the
> > > > incorrect lock.
> > > > Since many library modules import threading, it is impossible to
> > > > require
> > > > any import order.
> > > >
> > > > The safest way would be to patch all places that may use the original
> > > > thread.allocate_lock.
> > >
> > > The safest way would be to say loud and clear: the user must call
> > > pthreading.monkey_patch() on the begining of his main module, before
> > > threading is imported and most certainly before it is ever used.
> > >
> > > If we give the impression that doing anything else is fine, and we can
> > > end up replacing _active_limbo_lock while there's an entity (such as a
> > > _MainThread object) depending on its uniqueness.
> >
> > Then we should find a way to assert that the user did not import threading
> > yet,
> > otherwise our requirement is not very useful.
> >
> > If you import threading or another module that import threading, importing
> > pthreading should raise something like:
> >
> >     "You must import pthreading before threading is imported"
>
> that is not enough, you need also to run pthreading.monkeypatch. this is
> written in docs and comments, but you can't force the python's user who
> imports pthreading to use it as replacement for threading..
> it can be a warning though. anyhow, I'm not sure that this comment is under
> the umbrella of this suggested patch\bug fix.
>
> Yaniv Bronhaim.
>
> >
> > Nir
> >
> >
>

-- 
Yaniv Bronhaim.




More information about the vdsm-devel mailing list