Fix crashes by undoing/redoing nodes deletion

src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs

@@ -157,7 +157,7 @@ protected override void HandleTaskCompletionCore()
         /// </summary>
         /// <param name="other"></param>
         /// <returns></returns>
-        private bool Contains(UpdateGraphAsyncTask other)
+        private bool CanReplace(UpdateGraphAsyncTask other)
         {
             // Be more conservative here. Not only check ModifiedNodes, but
             // also check *all* nodes in graph sync data. For example, consider
@@ -175,9 +175,18 @@ private bool Contains(UpdateGraphAsyncTask other)
             else if (other.graphSyncData == null)
                 return true;
 
-            return other.graphSyncData.AddedNodeIDs.All(graphSyncData.AddedNodeIDs.Contains) &&
+            // Merging additions and removals can make the changeSetComputer internal state get corrupted.
+            // Imagine a sequence of queued tasks with changes like these: +A | -A | +AB
+            // Imagine a new task with change -A, ending up with the following sequence: -A | +A | -A | +AB
+            // While the merge routine would simplify this sequence to: -A | +A | +AB
+            // That creates an inconsistent sequence of changes where subtree A is added when it already exists!
+            // Modifications are not susceptible to this problem. Imagine ~A incoming with sequence: +A | -A | ~A | +AB
+            // The resulting sequence is: ~A | +A | -A | +AB, which is valid.
+            // Because modifications do not create or remove entries in the changeSetComputer internal state,
+            // if we remove one from anywhere in the sequence we remain consistent.
+            return other.graphSyncData.AddedNodeIDs.Count() == 0 &&
                    other.graphSyncData.ModifiedNodeIDs.All(graphSyncData.ModifiedNodeIDs.Contains) &&
-                   other.graphSyncData.DeletedNodeIDs.All(graphSyncData.DeletedNodeIDs.Contains);
+                   other.graphSyncData.DeletedNodeIDs.Count() == 0;
         }
 
         private bool IsScheduledAfter(UpdateGraphAsyncTask other)
@@ -188,12 +197,13 @@ private bool IsScheduledAfter(UpdateGraphAsyncTask other)
         protected override TaskMergeInstruction CanMergeWithCore(AsyncTask otherTask)
         {
             var theOtherTask = otherTask as UpdateGraphAsyncTask;
+
             if (theOtherTask == null)
                 return base.CanMergeWithCore(otherTask);
 
-            if (theOtherTask.IsScheduledAfter(this) && theOtherTask.Contains(this))
+            if (theOtherTask.IsScheduledAfter(this) && theOtherTask.CanReplace(this))
                 return TaskMergeInstruction.KeepOther;
-            else if (this.IsScheduledAfter(theOtherTask) && this.Contains(theOtherTask))
+            else if (this.IsScheduledAfter(theOtherTask) && this.CanReplace(theOtherTask))
                 return TaskMergeInstruction.KeepThis;
             else
                 return TaskMergeInstruction.KeepBoth;

src/DynamoCoreWpf/ViewModels/Core/HomeWorkspaceViewModel.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.ComponentModel;
 using System.Globalization;
 using System.Linq;
@@ -91,10 +92,22 @@ public override void OnNodeModified(NodeModel obj)
 
         private void hwm_SetNodeDeltaState(object sender, DeltaComputeStateEventArgs e)
         {
-            var nodeGuids = e.NodeGuidList;
+            // Using Nodes here is not thread safe, as nodes can be added/removed by the UI thread midway.
+            // Dispatching this to the UI thread would help to avoid concurrency issues but has caveats.
+            // When Dynamo is shutting down, a deadlock situation can occur, where each thread waits on the other.
+            // Moreover, tight periodic runs can create situations where we cannot safely check if we are shutting down.
+            // In summary, even if locking may be more costly, it is the safer approach.
+            lock (Nodes)
+            {
+                UpdateNodesDeltaState(e.NodeGuidList, e.GraphExecuted);
+            }
+        }
+
+        private void UpdateNodesDeltaState(List<Guid> nodeGuids, bool graphExecuted)
+        {
             // if runsettings is manual, and if the graph is not executed, then turing on showrunpreview 
             //should turn on showexectionpreview on every node.
-            if (nodeGuids.Count == 0 && !e.GraphExecuted)
+            if (nodeGuids.Count == 0 && !graphExecuted)
             {
                 foreach (var nodeModel in Nodes)
                 {
@@ -104,7 +117,7 @@ private void hwm_SetNodeDeltaState(object sender, DeltaComputeStateEventArgs e)
 
             //if the graph is executed then set the node preview to false , provided
             // there is no error on that node.
-            if (nodeGuids.Count == 0 && e.GraphExecuted)
+            if (nodeGuids.Count == 0 && graphExecuted)
             {
                 foreach (var nodeViewModel in Nodes)
                 {
@@ -113,7 +126,7 @@ private void hwm_SetNodeDeltaState(object sender, DeltaComputeStateEventArgs e)
                         nodeViewModel.ShowExecutionPreview = false;
                         nodeViewModel.IsNodeAddedRecently = false;
                     }
-                }                
+                }
             }
 
             foreach (Guid t in nodeGuids)
@@ -132,7 +145,7 @@ private void hwm_SetNodeDeltaState(object sender, DeltaComputeStateEventArgs e)
             {
                 if (nodes.ShowExecutionPreview)
                     nodes.ShowExecutionPreview = nodes.DynamoViewModel.ShowRunPreview;
-            }           
+            }
         }
 
         void hwm_EvaluationCompleted(object sender, EvaluationCompletedEventArgs e)

src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs

@@ -666,12 +666,15 @@ private void Model_AnnotationsCleared()
 
         void Model_NodesCleared()
         {
-            foreach(var nodeViewModel in Nodes)
+            lock (Nodes)
             {
-                this.unsubscribeNodeEvents(nodeViewModel);
-                nodeViewModel.Dispose();
+                foreach (var nodeViewModel in Nodes)
+                {
+                    this.unsubscribeNodeEvents(nodeViewModel);
+                    nodeViewModel.Dispose();
+                }
+                Nodes.Clear();
             }
-            Nodes.Clear();
             Errors.Clear();
 
             PostNodeChangeActions();
@@ -685,9 +688,13 @@ private void unsubscribeNodeEvents(NodeViewModel nodeViewModel)
 
         void Model_NodeRemoved(NodeModel node)
         {
-            NodeViewModel nodeViewModel = Nodes.First(x => x.NodeLogic == node);
-            Errors.Remove(nodeViewModel.ErrorBubble);
-            Nodes.Remove(nodeViewModel);
+            NodeViewModel nodeViewModel;
+            lock (Nodes)
+            {
+                nodeViewModel = Nodes.First(x => x.NodeLogic == node);
+                Errors.Remove(nodeViewModel.ErrorBubble);
+                Nodes.Remove(nodeViewModel);
+            }
             //unsub the events we attached below in NodeAdded.
             this.unsubscribeNodeEvents(nodeViewModel);
             nodeViewModel.Dispose();
@@ -700,7 +707,10 @@ void Model_NodeAdded(NodeModel node)
             var nodeViewModel = new NodeViewModel(this, node);
             nodeViewModel.SnapInputEvent += nodeViewModel_SnapInputEvent;
             nodeViewModel.NodeLogic.Modified += OnNodeModified;
-            Nodes.Add(nodeViewModel);
+            lock (Nodes)
+            {
+                Nodes.Add(nodeViewModel);
+            }
             Errors.Add(nodeViewModel.ErrorBubble);
             nodeViewModel.UpdateBubbleContent();