From 9b54a8f29e934af1adcd5cde389f80abc0f72217 Mon Sep 17 00:00:00 2001
From: Tom Morris <tfmorris@gmail.com>
Date: Thu, 30 Aug 2012 16:22:28 +0000
Subject: [PATCH] FIXED - task 559: Deadlock between autosave thread and
 history code http://code.google.com/p/google-refine/issues/detail?id=559

git-svn-id: http://google-refine.googlecode.com/svn/trunk@2538 7d457c2a-affb-35e4-300a-418c747d4874
---
 .../com/google/refine/history/History.java    | 39 ++++++++++++-------
 .../google/refine/history/HistoryEntry.java   |  6 +++
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/main/src/com/google/refine/history/History.java b/main/src/com/google/refine/history/History.java
index d00796887..8719eb2ee 100644
--- a/main/src/com/google/refine/history/History.java
+++ b/main/src/com/google/refine/history/History.java
@@ -127,22 +127,29 @@ public class History implements Jsonizable {
      * Adding a new entry clears all currently held future histories
      * @param entry
      */
-    synchronized public void addEntry(HistoryEntry entry) {
-        entry.apply(ProjectManager.singleton.getProject(_projectID));
-        _pastEntries.add(entry);
+    public void addEntry(HistoryEntry entry) {
+        Project project = ProjectManager.singleton.getProject(_projectID);
+        synchronized (project) {
+            // NOTE: project lock must be acquired *first* to prevent deadlocks, so we use a 
+            // synchronized block instead of synchronizing the entire method.
+            synchronized (this) {
+                entry.apply(project);
+                _pastEntries.add(entry);
 
-        setModified();
+                setModified();
 
-        // Any new change will clear all future entries.
-        List<HistoryEntry> futureEntries = _futureEntries;
-        _futureEntries = new ArrayList<HistoryEntry>();
+                // Any new change will clear all future entries.
+                List<HistoryEntry> futureEntries = _futureEntries;
+                _futureEntries = new ArrayList<HistoryEntry>();
 
-        for (HistoryEntry entry2 : futureEntries) {
-            try {
-                // remove residual data on disk
-                entry2.delete();
-            } catch (Exception e) {
-                e.printStackTrace();
+                for (HistoryEntry entry2 : futureEntries) {
+                    try {
+                        // remove residual data on disk
+                        entry2.delete();
+                    } catch (Exception e) {
+                        e.printStackTrace();
+                    }
+                }
             }
         }
     }
@@ -273,6 +280,12 @@ public class History implements Jsonizable {
         writer.endObject();
     }
 
+    /*
+     * NOTE: this method is called from the autosave thread with the Project
+     * lock already held, so no other synchronized method here can aquire that
+     * Project lock or a deadlock will result.be careful of thread synchronization to avoid
+     * deadlocks.
+     */
     synchronized public void save(Writer writer, Properties options) throws IOException {
         writer.write("pastEntryCount="); writer.write(Integer.toString(_pastEntries.size())); writer.write('\n');
         for (HistoryEntry entry : _pastEntries) {
diff --git a/main/src/com/google/refine/history/HistoryEntry.java b/main/src/com/google/refine/history/HistoryEntry.java
index f14bc967a..178371154 100644
--- a/main/src/com/google/refine/history/HistoryEntry.java
+++ b/main/src/com/google/refine/history/HistoryEntry.java
@@ -119,6 +119,12 @@ public class HistoryEntry implements Jsonizable {
         _manager.save(this, writer, options);
     }
 
+    /**
+     * Apply a change to a project.  In most cases you should already hold the Project lock
+     * before calling this method to prevent deadlocks.
+     * 
+     * @param project the project the change should be applied to
+     */
     public void apply(Project project) {
         if (getChange() == null) {
             ProjectManager.singleton.getHistoryEntryManager().loadChange(this);