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 <nsof...@redhat.com> To: Yaniv Bronheim <ybron...@redhat.com> ----- Forwarded Message -----
From: "Yaniv Bronheim" <ybron...@redhat.com> To: "Nir Soffer" <nsof...@redhat.com> Cc: "Dan Kenigsberg" <dan...@redhat.com>, "VDSM Project Development" <vdsm-devel@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" <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. Yaniv Bronhaim. > > Nir > >
-- Yaniv Bronhaim. _______________________________________________ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
