Merge pull request #292 from kkoz/table-lock-bug

Fixed race condition bug in tables

src/omero/hdfstorageV2.py

@@ -156,12 +156,15 @@ def addOrThrow(self, hdfpath, hdfstorage, read_only=False):
         return hdffile
 
     @locked
-    def getOrCreate(self, hdfpath, read_only=False):
+    def getOrCreate(self, hdfpath, table, read_only=False):
+        storage = None
         try:
-            return self.__paths[hdfpath]
+            storage = self.__paths[hdfpath]
         except KeyError:
             # Adds itself to the global list
-            return HdfStorage(hdfpath, self._lock, read_only=read_only)
+            storage = HdfStorage(hdfpath, self._lock, read_only=read_only)
+        storage.incr(table)
+        return storage
 
     @locked
     def remove(self, hdfpath, hdffile):

src/omero/tables.py

@@ -50,23 +50,21 @@ class TableI(omero.grid.Table, omero.util.SimpleServant):
     Spreadsheet implementation based on pytables.
     """
 
-    def __init__(self, ctx, file_obj, factory, storage, uuid="unknown",
+    def __init__(self, ctx, file_obj, file_path, factory, storage_factory, read_only=False, uuid="unknown",
                  call_context=None, adapter=None):
         self.id = Ice.Identity()
         self.id.name = uuid
         self.uuid = uuid
         self.file_obj = file_obj
         self.factory = factory
-        self.storage = storage
+        self.storage = storage_factory.getOrCreate(file_path, self, read_only)
         self.call_context = call_context
         self.adapter = adapter
         self.can_write = factory.getAdminService().canUpdate(
             file_obj, call_context)
         omero.util.SimpleServant.__init__(self, ctx)
 
         self.stamp = time.time()
-        self.storage.incr(self)
-
         self._closed = False
 
         if (not self.file_obj.isLoaded() or
@@ -531,8 +529,10 @@ def getTable(self, file_obj, factory, current=None):
         if not p.exists():
             p.makedirs()
 
-        storage = self._storage_factory.getOrCreate(file_path, self.read_only)
-        table = TableI(self.ctx, file_obj, factory, storage,
+        table = TableI(self.ctx, file_obj,file_path,
+                       factory,
+                       self._storage_factory,
+                       read_only=self.read_only,
                        uuid=Ice.generateUUID(),
                        call_context=current.ctx,
                        adapter=current.adapter)

test/unit/tablestest/test_servants.py

@@ -192,6 +192,15 @@ def decr(self, *args):
     def size(self):
         return 0
 
+class mock_storage_factory():
+    def __init__(self):
+        self.storages = []
+
+    def add_storage(self, storage):
+        self.storages.append(storage)
+
+    def getOrCreate(self, hdfpath, table, read_only=False):
+        return self.storages.pop()
 
 class TestTables(TestCase):
 
@@ -307,27 +316,21 @@ def testTableOriginalFileLoaded(self):
         f2.details.group = omero.model.ExperimenterGroupI(1, False)
         self.sf.return_values.append(f2)
         storage = mock_storage()
+        storage_factory = mock_storage_factory()
+        storage_factory.add_storage(storage)
         self.ctx.newSession()
-        table = omero.tables.TableI(self.ctx, f1, self.sf, storage)
+        table = omero.tables.TableI(self.ctx, f1, 'dummy/path', self.sf, storage_factory)
         assert table.file_obj.details.group
 
-    def testTableIncrDecr(self):
-        f = omero.model.OriginalFileI(1, True)
-        f.details.group = omero.model.ExperimenterGroupI(1, False)
-        storage = mock_storage()
-        table = omero.tables.TableI(self.ctx, f, self.sf, storage)
-        assert storage.up
-        table.cleanup()
-        assert storage.down
-
     def testTablePreInitialized(self):
         f = omero.model.OriginalFileI(1, True)
         f.details.group = omero.model.ExperimenterGroupI(1, False)
         mocktable = self.testTables()
         table1 = mocktable.table
         storage = table1.storage
         storage.initialize([LongColumnI("a", None, [])])
-        table2 = omero.tables.TableI(self.ctx, f, self.sf, storage)
+        storage_factory = self._TestTables__tables[0]._storage_factory
+        table2 = omero.tables.TableI(self.ctx, f, str(storage._HdfStorage__hdf_path), self.sf, storage_factory)
         table2.cleanup()
         table1.cleanup()
 
@@ -372,12 +375,12 @@ def testErrorInStorage(self):
         self.repofile(self.sf.db_uuid)
         of = omero.model.OriginalFileI(1, False)
         self.sf.return_values.append(of)
+        self.sf.return_values.append(of)
 
         internal_repo = mock_internal_repo(self.tmp)
         f = open(internal_repo.path, "w")
         f.write("this file is not HDF")
         f.close()
-
         tables = self.tablesI(internal_repo)
         pytest.raises(omero.ValidationException, tables.getTable, of, self.sf,
                       self.current)