Fix crashes by undoing/redoing nodes deletion (#11021)

mmisol · web-flow · commit 4440c2d52aa9 · 2020-08-19T17:21:50.000-04:00
Fixes two crashes whie doing this at a high pace:

One crash happened in HomeWorkspaceViewModel, when postprocessing the
nodes after execution. These changes were done from the scheduler
thread, which when timed just right conflicted with changes done from
the UI thread, by the undo and redo operations.

Another one happened in ChangeSetComputer, when trying to compute the
delta AST. The cause was an optimization that attempted to skip tasks
based on containment comparison but disrespecting causality. The path
taken was to prevent skipping tasks that added or removed nodes.
diff --git a/src/DynamoCore/Scheduler/UpdateGraphAsyncTask.cs b/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;
diff --git a/src/DynamoCoreWpf/ViewModels/Core/HomeWorkspaceViewModel.cs b/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)
diff --git a/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs b/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();
 
