----- Original Message -----
> From: "Nir Soffer" <nsof...@redhat.com>
> To: "Dan Kenigsberg" <dan...@redhat.com>
> Cc: "VDSM Project Development" <vdsm-devel@lists.fedorahosted.org>
> Sent: Wednesday, February 5, 2014 10:05:22 PM
> Subject: Re: [vdsm] suggested patch for python-pthreading
>
> ----- Original Message -----
> > From: "Dan Kenigsberg" <dan...@redhat.com>
> > To: "Yaniv Bronheim" <ybron...@redhat.com>
> > Cc: "Nir Soffer" <nsof...@redhat.com>, "VDSM Project Development"
> > <vdsm-devel@lists.fedorahosted.org>
> > Sent: Wednesday, February 5, 2014 9:23:08 PM
> > Subject: Re: [vdsm] suggested patch for python-pthreading
> >
> > On Wed, Feb 05, 2014 at 11:41:28AM -0500, Yaniv Bronheim wrote:
> > > ----- Original Message -----
> > > > From: "Nir Soffer" <nsof...@redhat.com>
> > > > To: "Dan Kenigsberg" <dan...@redhat.com>
> > > > Cc: "Yaniv Bronheim" <ybron...@redhat.com>, "VDSM Project Development"
> > > > <vdsm-devel@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" <dan...@redhat.com>
> > > > > To: "Nir Soffer" <nsof...@redhat.com>
> > > > > Cc: "Yaniv Bronheim" <ybron...@redhat.com>, "VDSM Project
> > > > > Development"
> > > > > <vdsm-devel@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" <ybron...@redhat.com>
> > > > > > > To: "Nir Soffer" <nsof...@redhat.com>
> > > > > > > Cc: "VDSM Project Development"
> > > > > > > <vdsm-devel@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" <nsof...@redhat.com>
> > > > > > > > To: "Yaniv Bronheim" <ybron...@redhat.com>
> > > > > > > > Cc: "VDSM Project Development"
> > > > > > > > <vdsm-devel@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" <ybron...@redhat.com>
> > > > > > > > > To: "VDSM Project Development"
> > > > > > > > > <vdsm-devel@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 <ybron...@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 <ybron...@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.
>
> You are correct, this should be in another patch.
>
> >
> > Adding Nir's assertion in monkey_patch() makes a lot of sense, but may
> > be a bit cumbersome to implement. On the other hand, disallowing
> > out-of-order import, even if pthreading is never used, seems a bit rude.
>
> You are also correct.
>
> How is this?
>
> import sys
>
> def monkeypatch():
> if 'thread' in sys.modules or 'threading' in sys.modules:
> raise Exception("pthreading must be imported before thread and
> threading modules")
> import thread
> patch it...
> import threading
> patch it...
+1
> _______________________________________________
> vdsm-devel mailing list
> vdsm-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
>
_______________________________________________
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
