dhis2-devs team mailing list archive

[noreply@xxxxxxxxxxxxx]

------------------------------------------------------------
revno: 21767
committer: Halvdan Hoem Grelland <halvdanhg@xxxxxxxxx>
branch nick: dhis2
timestamp: Fri 2016-01-15 18:50:00 +0100
message:
  Re-implement temporary fix for fileResource Hibernate vs callback race condition
modified:
  dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java


--
lp:dhis2
https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk

Your team DHIS 2 developers is subscribed to branch lp:dhis2.
To unsubscribe from this branch go to https://code.launchpad.net/~dhis2-devs-core/dhis2/trunk/+edit-subscription

=== modified file 'dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java'
--- dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java	2016-01-04 02:27:49 +0000
+++ dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/fileresource/DefaultFileResourceService.java	2016-01-15 17:50:00 +0000
@@ -29,14 +29,15 @@
  */
 
 import com.google.common.io.ByteSource;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.hisp.dhis.common.GenericIdentifiableObjectStore;
 import org.hisp.dhis.system.scheduling.Scheduler;
 import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.joda.time.Hours;
+import org.joda.time.Seconds;
 import org.springframework.transaction.annotation.Transactional;
-import org.springframework.transaction.support.TransactionSynchronizationAdapter;
-import org.springframework.transaction.support.TransactionSynchronizationManager;
 import org.springframework.util.concurrent.ListenableFuture;
 
 import javax.annotation.PostConstruct;
@@ -52,6 +53,8 @@
 public class DefaultFileResourceService
     implements FileResourceService
 {
+    private static final Log log = LogFactory.getLog(DefaultFileResourceService.class);
+
     private static final String KEY_FILE_CLEANUP_TASK = "fileResourceCleanupTask";
 
     private static final Duration IS_ORPHAN_TIME_DELTA = Hours.TWO.toStandardDuration();
@@ -119,7 +122,8 @@
     @Override
     public FileResource getFileResource( String uid )
     {
-        return fileResourceStore.getByUid( uid );
+        // TODO ensureStorageStatus(..) is a temp fix. Should be removed.
+        return ensureStorageStatus( fileResourceStore.getByUid( uid ) );
     }
 
     @Override
@@ -147,24 +151,7 @@
 
         final String uid = fileResource.getUid();
 
-        // Ensures callback is registered after this transaction is committed.
-        // Works as a safeguard against the unlikely race condition which
-        // could occur when the callback is executed before the FileResource
-        // object has been written to the db. We should consider exposing the
-        // locking mechanisms of Hibernate which would offer a cleaner solution
-        // to this very issue.
-
-        TransactionSynchronizationManager.registerSynchronization(
-            new TransactionSynchronizationAdapter()
-            {
-                @Override
-                public void afterCommit()
-                {
-                    super.afterCommit();
-                    saveContentTask.addCallback( uploadCallback.newInstance( uid ) );
-                }
-            }
-        );
+        saveContentTask.addCallback( uploadCallback.newInstance( uid ) );
 
         return uid;
     }
@@ -220,4 +207,48 @@
 
         return fileResourceContentStore.getSignedGetContentUri( fileResource.getStorageKey() );
     }
+
+    // -------------------------------------------------------------------------
+    // Supportive methods
+    // -------------------------------------------------------------------------
+
+    /**
+     * TODO Temporary fix. Remove at some point.
+     *
+     * Ensures correctness of the storageStatus of this FileResource.
+     *
+     * If the status has been 'PENDING' for more than 1 second we check to see if the content may actually have been stored.
+     * If this is the case the status is corrected to STORED.
+     *
+     * This method is a TEMPORARY fix for the for now unsolved issue with a race occurring between the Hibernate object cache
+     * and the upload callback attempting to modify the FileResource object upon completion.
+     *
+     * Resolving that issue (likely by breaking the StorageStatus into a separate table) should make this method redundant.
+     */
+    private FileResource ensureStorageStatus( FileResource fileResource )
+    {
+        if (FileResourceStorageStatus.PENDING == fileResource.getStorageStatus() )
+        {
+            Duration pendingDuration = new Duration( new DateTime( fileResource.getLastUpdated() ), DateTime.now() );
+
+            if ( pendingDuration.isLongerThan( Seconds.seconds( 1 ).toStandardDuration() ) )
+            {
+                // Upload has been finished for 5+ seconds and is still PENDING.
+                // Check if content has actually been stored and correct to STORED if this is the case.
+
+                boolean contentIsStored = fileResourceContentStore.fileResourceContentExists( fileResource.getStorageKey() );
+
+                if ( contentIsStored )
+                {
+                    // We fix
+                    fileResource.setStorageStatus( FileResourceStorageStatus.STORED );
+                    fileResourceStore.update( fileResource );
+                    log.warn( "Corrected issue: FileResource '" + fileResource.getUid() +
+                        "' had storageStatus PENDING but content was actually stored." );
+                }
+            }
+        }
+
+        return fileResource;
+    }
 }