Since an exception may be thrown within getWriter, add a finally bloc…

…k that releases the lock - Add test for throwing an exception from DOTranslator's deserialize method - Add finally block as suggested by @barmintor
fcrepo3 · Mar 4, 2015 · 486c789 · 486c789
1 parent 38d66b9
commit 486c789
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 8 deletions.
diff --git a/fcrepo-server/src/main/java/org/fcrepo/server/storage/DefaultDOManager.java b/fcrepo-server/src/main/java/org/fcrepo/server/storage/DefaultDOManager.java
@@ -784,14 +784,22 @@ public DOWriter getWriter(boolean cachedObjectRequired, Context context,
         } else {
             BasicDigitalObject obj = new BasicDigitalObject();
             getWriteLock(pid);
-            m_translator.deserialize(m_permanentStore.retrieveObject(pid), obj,
-                    m_defaultStorageFormat, m_storageCharacterEncoding,
-                    DOTranslationUtility.DESERIALIZE_INSTANCE);
-            DOWriter w =
-                    new SimpleDOWriter(context, this, m_translator,
-                            m_defaultStorageFormat, m_storageCharacterEncoding,
-                            obj);
-            return w;
+            boolean clean = false;
+            try {
+                m_translator.deserialize(m_permanentStore.retrieveObject(pid), obj,
+                        m_defaultStorageFormat, m_storageCharacterEncoding,
+                        DOTranslationUtility.DESERIALIZE_INSTANCE);
+                DOWriter w =
+                        new SimpleDOWriter(context, this, m_translator,
+                                m_defaultStorageFormat, m_storageCharacterEncoding,
+                                obj);
+                clean = true;
+                return w;
+            } finally {
+                if (!clean) {
+                    releaseWriteLock(pid);
+                }
+            }
         }
     }
 

diff --git a/fcrepo-server/src/test/java/org/fcrepo/server/storage/DefaultDOManagerTest.java b/fcrepo-server/src/test/java/org/fcrepo/server/storage/DefaultDOManagerTest.java
@@ -36,6 +36,8 @@
 import org.fcrepo.server.Server;
 import org.fcrepo.server.errors.ObjectExistsException;
 import org.fcrepo.server.errors.ObjectLockedException;
+import org.fcrepo.server.errors.ServerException;
+import org.fcrepo.server.errors.GeneralException;
 import org.fcrepo.server.management.BasicPIDGenerator;
 import org.fcrepo.server.management.ManagementModule;
 import org.fcrepo.server.resourceIndex.ResourceIndexModule;
@@ -444,9 +446,65 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
         assertEquals(0, unexpectedFailures);
     }
 
+    @Test
+    public void testGetWriterUnlocksForException() throws Throwable {
+
+        doAnswer(
+            new Answer<Void>() {
+                private boolean thrown = false;
+
+                @Override
+                public Void answer(InvocationOnMock invocation) throws Throwable {
+                    // Throw an exception on the first try, succeed on the second.
+                    if (thrown == false) {
+                        thrown = true;
+                        throw new GeneralException("Expected server exception");
+                    } else {
+                        BasicDigitalObject obj = (BasicDigitalObject) invocation.getArguments()[1];
+                        obj.setPid(DUMMY_PID);
+                    }
+
+                    return null;
+                }
+            }
+        ).when(mockTranslatorModule).deserialize(
+            any(InputStream.class), any(BasicDigitalObject.class), eq(FORMAT), eq(ENCODING),
+                eq(DOTranslationUtility.DESERIALIZE_INSTANCE));
+
+        // Our first try should result in an exception, which should unlock the object.
+
+        GetWriterRunnable r1 = new GetWriterRunnable(testObj);
+        Thread t1 = new Thread(r1);
+
+        t1.start();
+        try {
+            t1.join(100);
+        } catch (InterruptedException e) {}
+
+        assertEquals(0, r1.successes.get());
+        assertEquals(1, r1.expectedFailures.get());
+        assertEquals(0, r1.unexpectedFailures.get());
+
+        // Our second try should succeed, because the object should have been unlocked.
+
+        GetWriterRunnable r2 = new GetWriterRunnable(testObj);
+        Thread t2 = new Thread(r2);
+
+        t2.start();
+        try {
+            t2.join(100);
+        } catch (InterruptedException e) {}
+
+        assertEquals(1, r2.successes.get());
+        assertEquals(0, r2.expectedFailures.get());
+        assertEquals(0, r2.unexpectedFailures.get());
+
+    }
+
     class GetWriterRunnable implements Runnable {
           DefaultDOManager manager;
           AtomicInteger successes = new AtomicInteger();
+          AtomicInteger expectedFailures = new AtomicInteger();
           AtomicInteger unexpectedFailures = new AtomicInteger();
 
           GetWriterRunnable(DefaultDOManager manager) {
@@ -458,6 +516,9 @@ public void run() {
                   // Get a writer and keep it forever
                   DOWriter writer = manager.getWriter(false, mockContext, DUMMY_PID);
                   successes.incrementAndGet();
+              } catch (ServerException ex) {
+                  LOGGER.info( "{} - thread caught expected exception: {}", Thread.currentThread().getName(), ex );
+                  expectedFailures.incrementAndGet();
               } catch (Exception ex) {
                   LOGGER.error(Thread.currentThread().getName() + " - Exception", ex);
                   unexpectedFailures.incrementAndGet();