[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