[452962] Move creation of IWorkspaceRunnable

The creation of a IWorkspaceRunnable in the SavingPolicy is not the
right place. Indeed, this can lead to deadlock problem (see [1] for
detail). The creation of a IWorkspaceRunnable has been moved to the
Saver itself. By doing that, the lock on the Workspace is asked before
the lock on Saver.isSaving (same order as in case of a session.save()
called from a WorkspaceModifyOperation).

[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=452962#c21

Bug: 452962
Change-Id: I88e54d8c7b535f16350daca7a61c0c7316ce43c4
Signed-off-by: Laurent Redor <laurent.redor@obeo.fr>

3 files changed, 45 insertions, 46 deletions

diff --git a/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/AbstractSavingPolicy.java b/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/AbstractSavingPolicy.java
index 2e2f28d10d..05c04e3178 100644
--- a/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/AbstractSavingPolicy.java
+++ b/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/AbstractSavingPolicy.java
@@ -16,13 +16,7 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 
-import org.eclipse.core.resources.IWorkspaceRoot;
-import org.eclipse.core.resources.IWorkspaceRunnable;
-import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
-import org.eclipse.core.runtime.SubProgressMonitor;
-import org.eclipse.core.runtime.jobs.Job;
-import org.eclipse.emf.ecore.plugin.EcorePlugin;
 import org.eclipse.emf.ecore.resource.Resource;
 import org.eclipse.emf.ecore.xmi.XMLResource;
 import org.eclipse.emf.transaction.TransactionalEditingDomain;
@@ -60,24 +54,15 @@ public abstract class AbstractSavingPolicy implements SavingPolicy {
         try {
             monitor.beginTask("Save Session", IProgressMonitor.UNKNOWN);
             resourcesToSave.addAll(computeResourcesToSave(Sets.newLinkedHashSet(allResources), options, monitor));
-            if (alreadyIsInWorkspaceModificationOperation()) {
-                wrappedSave(resourcesToSave, allResources, options, new SubProgressMonitor(monitor, IProgressMonitor.UNKNOWN));
+            if (options == null) {
+                ResourceSetSync.getOrInstallResourceSetSync(domain).save(resourcesToSave, allResources, getDefaultSaveOptions());
             } else {
-                IWorkspaceRoot workspaceRoot = EcorePlugin.getWorkspaceRoot();
-                if (workspaceRoot != null) {
-                    try {
-                        workspaceRoot.getWorkspace().run(new IWorkspaceRunnable() {
-                            public void run(final IProgressMonitor monitor) throws CoreException {
-                                wrappedSave(resourcesToSave, allResources, options, monitor);
-                            }
-                        }, new SubProgressMonitor(monitor, IProgressMonitor.UNKNOWN));
-                    } catch (final CoreException e) {
-                        SiriusPlugin.getDefault().error("Core exception while saving session", e);
-                    }
-                } else {
-                    wrappedSave(resourcesToSave, allResources, options, monitor);
-                }
+                ResourceSetSync.getOrInstallResourceSetSync(domain).save(resourcesToSave, allResources, options);
             }
+        } catch (final IOException e) {
+            SiriusPlugin.getDefault().error("error while saving session", e);
+        } catch (final InterruptedException e) {
+            SiriusPlugin.getDefault().error("error while saving session", e);
         } finally {
             monitor.done();
         }
@@ -112,26 +97,4 @@ public abstract class AbstractSavingPolicy implements SavingPolicy {
      *         on disk.
      */
     protected abstract Collection<Resource> computeResourcesToSave(Set<Resource> scope, Map<?, ?> options, IProgressMonitor monitor);
-
-    private void wrappedSave(final Iterable<Resource> resourcesToSave, final Iterable<Resource> allResources, final Map<?, ?> options, IProgressMonitor monitor) {
-        try {
-            monitor.beginTask("Session Saving", IProgressMonitor.UNKNOWN);
-            if (options == null) {
-                ResourceSetSync.getOrInstallResourceSetSync(domain).save(resourcesToSave, allResources, getDefaultSaveOptions());
-            } else {
-                ResourceSetSync.getOrInstallResourceSetSync(domain).save(resourcesToSave, allResources, options);
-            }
-        } catch (final IOException e) {
-            SiriusPlugin.getDefault().error("error while saving session", e);
-        } catch (final InterruptedException e) {
-            SiriusPlugin.getDefault().error("error while saving session", e);
-        } finally {
-            monitor.done();
-        }
-    }
-
-    private boolean alreadyIsInWorkspaceModificationOperation() {
-        final Job currentJob = Job.getJobManager().currentJob();
-        return currentJob != null && currentJob.getRule() != null;
-    }
 }
diff --git a/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/SavingPolicy.java b/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/SavingPolicy.java
index e5f1f830af..be4c763f5f 100644
--- a/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/SavingPolicy.java
+++ b/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/api/session/SavingPolicy.java
@@ -24,7 +24,10 @@ import org.eclipse.emf.ecore.resource.Resource;
  */
 public interface SavingPolicy {
     /**
-     * Save the given resources with the provided options.
+     * Save the given resources with the provided options. The caller has the
+     * responsibility to call this method in a
+     * {@link org.eclipse.core.resources.IWorkspaceRunnable} if it modifies the
+     * workspace.
      * 
      * @param resourcesToSave
      *            the resources to save
diff --git a/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/internal/session/danalysis/Saver.java b/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/internal/session/danalysis/Saver.java
index 1e27288a9e..b61ffd1c60 100644
--- a/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/internal/session/danalysis/Saver.java
+++ b/plugins/org.eclipse.sirius/src/org/eclipse/sirius/business/internal/session/danalysis/Saver.java
@@ -13,7 +13,13 @@ package org.eclipse.sirius.business.internal.session.danalysis;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import org.eclipse.core.resources.IWorkspaceRoot;
+import org.eclipse.core.resources.IWorkspaceRunnable;
+import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.SubProgressMonitor;
+import org.eclipse.core.runtime.jobs.Job;
+import org.eclipse.emf.ecore.plugin.EcorePlugin;
 import org.eclipse.emf.transaction.TransactionalEditingDomain;
 import org.eclipse.emf.transaction.TransactionalEditingDomain.Lifecycle;
 import org.eclipse.emf.transaction.TransactionalEditingDomainEvent;
@@ -21,6 +27,7 @@ import org.eclipse.emf.transaction.TransactionalEditingDomainListenerImpl;
 import org.eclipse.emf.transaction.impl.InternalTransaction;
 import org.eclipse.emf.transaction.impl.InternalTransactionalEditingDomain;
 import org.eclipse.emf.transaction.util.TransactionUtil;
+import org.eclipse.sirius.viewpoint.SiriusPlugin;
 
 /**
  * Encapsulates the decision of *when* to actually save the session's state when
@@ -101,7 +108,28 @@ final class Saver extends TransactionalEditingDomainListenerImpl {
     /**
      * Save immediately and disarm the trigger.
      */
-    private void saveNow(Map<?, ?> options, IProgressMonitor monitor, boolean runExclusive) {
+    private void saveNow(final Map<?, ?> options, IProgressMonitor monitor, final boolean runExclusive) {
+        if (alreadyIsInWorkspaceModificationOperation()) {
+            wrappedSave(options, monitor, runExclusive);
+        } else {
+            IWorkspaceRoot workspaceRoot = EcorePlugin.getWorkspaceRoot();
+            if (workspaceRoot != null) {
+                try {
+                    workspaceRoot.getWorkspace().run(new IWorkspaceRunnable() {
+                        public void run(final IProgressMonitor progressMonitor) throws CoreException {
+                            wrappedSave(options, progressMonitor, runExclusive);
+                        }
+                    }, new SubProgressMonitor(monitor, IProgressMonitor.UNKNOWN));
+                } catch (final CoreException e) {
+                    SiriusPlugin.getDefault().error("Core exception while saving session", e);
+                }
+            } else {
+                wrappedSave(options, monitor, runExclusive);
+            }
+        }
+    }
+
+    private void wrappedSave(Map<?, ?> options, IProgressMonitor monitor, boolean runExclusive) {
         // This allows to have session saving thread safe, i.e. only one thread
         // can do a save at a time
         synchronized (isSaving) {
@@ -122,6 +150,11 @@ final class Saver extends TransactionalEditingDomainListenerImpl {
         }
     }
 
+    private boolean alreadyIsInWorkspaceModificationOperation() {
+        final Job currentJob = Job.getJobManager().currentJob();
+        return currentJob != null && currentJob.getRule() != null;
+    }
+
     protected void disarm() {
         this.savedOptions = null;
         this.savedMonitor = null;