----- 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... _______________________________________________ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel