- added synchronization to the mapper() construction step, to avoid

thread collections when pre-existing mappers are compiling in a
different thread [ticket:613]

4 files changed, 66 insertions, 47 deletions

diff --git a/CHANGES b/CHANGES
index e60db5cb7..393c6a288 100644
--- a/CHANGES
+++ b/CHANGES
@@ -8,6 +8,9 @@
     - association proxies no longer bind tightly to source collections
       [ticket:597], and are constructed with a thunk instead
 - orm
+    - added synchronization to the mapper() construction step, to avoid
+      thread collections when pre-existing mappers are compiling in a 
+      different thread [ticket:613]
     - fixed very stupid bug when deleting items with many-to-many
       uselist=False relations
     - remember all that stuff about polymorphic_union ?  for 
diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py
index 7a73ecca5..36830e861 100644
--- a/lib/sqlalchemy/orm/__init__.py
+++ b/lib/sqlalchemy/orm/__init__.py
@@ -117,12 +117,16 @@ def clear_mappers():
     classes as their primary mapper.
     """
 
-    for mapper in mapper_registry.values():
-        attribute_manager.reset_class_managed(mapper.class_)
-        if hasattr(mapper.class_, 'c'):
-            del mapper.class_.c
-    mapper_registry.clear()
-    sautil.ArgSingleton.instances.clear()
+    mapperlib._COMPILE_MUTEX.acquire()
+    try:
+        for mapper in mapper_registry.values():
+            attribute_manager.reset_class_managed(mapper.class_)
+            if hasattr(mapper.class_, 'c'):
+                del mapper.class_.c
+        mapper_registry.clear()
+        sautil.ArgSingleton.instances.clear()
+    finally:
+        mapperlib._COMPILE_MUTEX.release()
 
 def clear_mapper(m):
     """Remove the given mapper from the storage of mappers.
@@ -130,13 +134,17 @@ def clear_mapper(m):
     When a new mapper is created for the previous mapper's class, it
     will be used as that classes' new primary mapper.
     """
-
-    del mapper_registry[m.class_key]
-    attribute_manager.reset_class_managed(m.class_)
-    if hasattr(m.class_, 'c'):
-        del m.class_.c
-    m.class_key.dispose()
-
+    
+    mapperlib._COMPILE_MUTEX.acquire()
+    try:
+        del mapper_registry[m.class_key]
+        attribute_manager.reset_class_managed(m.class_)
+        if hasattr(m.class_, 'c'):
+            del m.class_.c
+        m.class_key.dispose()
+    finally:
+        mapperlib._COMPILE_MUTEX.release()
+        
 def extension(ext):
     """Return a ``MapperOption`` that will insert the given
     ``MapperExtension`` to the beginning of the list of extensions
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 7e44d8a42..994264cc7 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -687,7 +687,11 @@ class Mapper(object):
                 # cant set __name__ in py 2.3 !
                 pass
             self.class_.__init__ = init
-        mapper_registry[self.class_key] = self
+        _COMPILE_MUTEX.acquire()
+        try:
+            mapper_registry[self.class_key] = self
+        finally:
+            _COMPILE_MUTEX.release()
         if self.entity_name is None:
             self.class_.c = self.c
 
diff --git a/test/perf/threaded_compile.py b/test/perf/threaded_compile.py
index 3c521f231..4c9723b43 100644
--- a/test/perf/threaded_compile.py
+++ b/test/perf/threaded_compile.py
@@ -1,7 +1,9 @@
-# tests the COMPILE_MUTEX in mapper compilation
+"""test that mapper compilation is threadsafe, including
+when additional mappers are created while the existing 
+collection is being compiled."""
 
 from sqlalchemy import *
-import thread, time, random
+import thread, time
 from sqlalchemy.orm import mapperlib
 
 meta = BoundMetaData('sqlite:///foo.db')
@@ -16,6 +18,10 @@ t2 = Table('t2', meta,
     Column('c2', String(30)),
     Column('t1c1', None, ForeignKey('t1.c1'))
 )
+t3 = Table('t3', meta,
+    Column('c1', Integer, primary_key=True),
+    Column('c2', String(30)),
+)
 meta.create_all()
 
 class T1(object):
@@ -32,39 +38,37 @@ class FakeLock(object):
 # should produce thread collisions    
 #mapperlib._COMPILE_MUTEX = FakeLock()
 
-existing_compile_all = mapperlib.Mapper._compile_all
-state = [False]
-# decorate mapper's _compile_all() method; the mutex in mapper.compile()
-# should insure that this method is only called once by a single thread only
-def monkeypatch_compile_all(self):
-    if state[0]:
-        raise "thread collision"
-    state[0] = True
-    try:
-        print "compile", thread.get_ident()
-        time.sleep(1 + random.random())
-        existing_compile_all(self)
-    finally:
-        state[0] = False
-mapperlib.Mapper._compile_all = monkeypatch_compile_all
-
 def run1():
-    print "T1", thread.get_ident()
-    class_mapper(T1)
+    for i in range(50):
+        print "T1", thread.get_ident()
+        class_mapper(T1)
+        time.sleep(.05)
 
 def run2():
-    print "T2", thread.get_ident()
-    class_mapper(T2)
+    for i in range(50):
+        print "T2", thread.get_ident()
+        class_mapper(T2)
+        time.sleep(.057)
 
-for i in range(0,1):
-    clear_mappers()
-    mapper(T1, t1, properties={'t2':relation(T2, backref="t1")})
-    mapper(T2, t2)
-    #compile_mappers()
-    print "START"
-    for j in range(0, 5):
-        thread.start_new_thread(run1, ())
-        thread.start_new_thread(run2, ())
-    print "WAIT"
-    time.sleep(5)
+def run3():
+    for i in range(50):
+        def foo():
+            print "FOO", thread.get_ident()
+            class Foo(object):pass
+            mapper(Foo, t3)
+            class_mapper(Foo).compile()
+        foo()
+        time.sleep(.05)
+    
+mapper(T1, t1, properties={'t2':relation(T2, backref="t1")})
+mapper(T2, t2)
+print "START"
+for j in range(0, 5):
+    thread.start_new_thread(run1, ())
+    thread.start_new_thread(run2, ())
+    thread.start_new_thread(run3, ())
+    thread.start_new_thread(run3, ())
+    thread.start_new_thread(run3, ())
+print "WAIT"
+time.sleep(5)