From 9206ee5a63a65e076342896e3b41bbcbf819af56 Mon Sep 17 00:00:00 2001
From: Stanislaw Pitucha <stanislaw.pitucha@hp.com>
Date: Mon, 6 Aug 2012 23:16:07 +0100
Subject: Solve possible race in semaphor creation

Solve the race condition described in the bug 872558 report which can
result in:

- thread crashing trying to remove semaphore from dict
- two threads getting different semaphores for the same name

First case is solved automatically by weakref dictionary. No explicit
deletion takes place.
The second case is solved by getting existing or new semaphore in one
step. Once a local reference to the semaphore is obtained, it can be
safely assigned to the dictionary if it's missing. If it's present, it
will not be removed anymore because there's at least one strong
reference present (local variable 'sem').

This solution is only valid for greenthreads.

Change-Id: I6bddc3e7abb39fd75e1f03abb2ea0f911b761957
---
 nova/utils.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

(limited to 'nova')

diff --git a/nova/utils.py b/nova/utils.py
index 6b37a544b..a3c657364 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -39,6 +39,7 @@ import tempfile
 import threading
 import time
 import uuid
+import weakref
 from xml.sax import saxutils
 
 from eventlet import corolocal
@@ -629,7 +630,7 @@ class InterProcessLock(object):
                              % self.fname)
 
 
-_semaphores = {}
+_semaphores = weakref.WeakValueDictionary()
 
 
 def synchronized(name, external=False):
@@ -667,9 +668,13 @@ def synchronized(name, external=False):
             # NOTE(soren): If we ever go natively threaded, this will be racy.
             #              See http://stackoverflow.com/questions/5390569/dyn
             #              amically-allocating-and-destroying-mutexes
+            sem = _semaphores.get(name, semaphore.Semaphore())
             if name not in _semaphores:
-                _semaphores[name] = semaphore.Semaphore()
-            sem = _semaphores[name]
+                # this check is not racy - we're already holding ref locally
+                # so GC won't remove the item and there was no IO switch
+                # (only valid in greenthreads)
+                _semaphores[name] = sem
+
             LOG.debug(_('Attempting to grab semaphore "%(lock)s" for method '
                         '"%(method)s"...'), {'lock': name,
                                              'method': f.__name__})
@@ -692,11 +697,6 @@ def synchronized(name, external=False):
                 else:
                     retval = f(*args, **kwargs)
 
-            # If no-one else is waiting for it, delete it.
-            # See note about possible raciness above.
-            if not sem.balance < 1:
-                del _semaphores[name]
-
             return retval
         return inner
     return wrap
-- 
cgit