[PATCH] Make thread manager operations atomic (#1029898)
Martin Kolman
mkolman at redhat.com
Fri Nov 15 15:03:06 UTC 2013
On Thu, 2013-11-14 at 15:01 +0100, Vratislav Podzimek wrote:
> On Thu, 2013-11-14 at 14:42 +0100, Martin Kolman wrote:
> > The thread manipulation in thread manager needs to
> > be atomic to get rid of nasty race conditions.
> > For example, without the lock, it was possible
> > to get a thread object that was not yet started
> > (as threads are first added to the tracking dict
> > and then started) and try to join it,
> > resulting in an exception.
> >
> > Signed-off-by: Martin Kolman <mkolman at redhat.com>
> > ---
> > pyanaconda/threads.py | 43 ++++++++++++++++++++++++++++++-------------
> > 1 file changed, 30 insertions(+), 13 deletions(-)
> >
> > diff --git a/pyanaconda/threads.py b/pyanaconda/threads.py
> > index 77cc306..1acde6a 100644
> > --- a/pyanaconda/threads.py
> > +++ b/pyanaconda/threads.py
> > @@ -40,6 +40,7 @@ class ThreadManager(object):
> > """
> > def __init__(self):
> > self._objs = {}
> > + self._objs_lock = threading.RLock()
> > self._errors = {}
> > self._main_thread = threading.current_thread()
> >
> > @@ -50,12 +51,16 @@ class ThreadManager(object):
> > """Given a Thread or Process object, add it to the list of known objects
> > and start it. It is assumed that obj.name is unique and descriptive.
> > """
> > - if obj.name in self._objs:
> > - raise KeyError("Cannot add thread '%s', a thread with the same name already running" % obj.name)
> >
> > - self._objs[obj.name] = obj
> > - self._errors[obj.name] = None
> > - obj.start()
> > + # we need to lock the thread dictionary when adding a new thread,
> > + # so that callers can't get & join threads that are not yet started
> > + with self._objs_lock:
> > + if obj.name in self._objs:
> > + raise KeyError("Cannot add thread '%s', a thread with the same name already running" % obj.name)
> > +
> > + self._objs[obj.name] = obj
> > + self._errors[obj.name] = None
> > + obj.start()
> >
> > return obj.name
> >
> > @@ -64,27 +69,37 @@ class ThreadManager(object):
> > be called when a thread exits, or there will be no way to get a
> > handle on it.
> > """
> > - self._objs.pop(name)
> > + with self._objs_lock:
> > + self._objs.pop(name)
> >
> > def exists(self, name):
> > """Determine if a thread or process exists with the given name."""
> > - return name in self._objs
> > +
> > + # thread in the ThreadManager only oficially exists once started
> ^^ officially
> > + with self._objs_lock:
> > + return name in self._objs
> >
> > def get(self, name):
> > """Given an object name, see if it exists and return the object.
> > Return None if no such object exists. Additionally, this method
> > will re-raise any uncaught exception in the thread.
> > """
> > - obj = self._objs.get(name)
> > - if obj:
> > - self.raise_error(name)
> >
> > - return obj
> > + # without the lockm it would be possible to get & join
> ^^^ lock
> > + # a thread that was not yet started
> > + with self._objs_lock:
> > + obj = self._objs.get(name)
> > + if obj:
> > + self.raise_error(name)
> > +
> > + return obj
> >
> > def wait(self, name):
> > """Wait for the thread to exit and if the thread exited with an error
> > re-raise it here.
> > """
> > + # we don't need a lock here,
> > + # because both exists() & get() are using it
> > if self.exists(name):
> > self.get(name).join()
> >
> > @@ -136,7 +151,8 @@ class ThreadManager(object):
> > :returns: number of running threads
> > :rtype: int
> > """
> > - return len(self._objs)
> > + with self._objs_lock:
> > + return len(self._objs)
> >
> > @property
> > def names(self):
> > @@ -145,7 +161,8 @@ class ThreadManager(object):
> > :returns: list of thread names
> > :rtype: list of strings
> > """
> > - return self._objs.keys()
> > + with self._objs_lock:
> > + return self._objs.keys()
> >
> > class AnacondaThread(threading.Thread):
> > """A threading.Thread subclass that exists only for a couple purposes:
> Otherwise this looks good to me.
>
OK, thanks, fixing locally.
More information about the anaconda-patches
mailing list