Register custom node before package load reset (#10591)

This fixes a problem where existing custom nodes in the home workspace became unresolved after a package that contained binaries was loaded. The cause of the problem was that the compiled function was not available at the time of the execution after a VM reset. Now the data is registered on package load, by queueing it in pendingCustomNodeSyncData. This results in a CompileCustomNodeAsyncTask being scheduled before the update of the home workspace graph takes place.
DynamoDS · Apr 29, 2020 · 31cb44e · 31cb44e
1 parent 415fa68
commit 31cb44e
Show file tree

Hide file tree

Showing 7 changed files with 280 additions and 48 deletions.
diff --git a/src/DynamoCore/Engine/EngineController.cs b/src/DynamoCore/Engine/EngineController.cs
@@ -43,11 +43,6 @@ public class EngineController : LogSourceBase, IAstNodeContainer, IDisposable
         /// </summary>
         internal static event Action VMLibrariesReset;
 
-        /// <summary>
-        /// This flag is used to check if any packages are currently being loaded, and to disable any executions that are triggered before the package loading is completed. See DYN-2101 for more info.
-        /// </summary>
-        internal static Boolean DisableRun = false;
-
         /// <summary>
         /// This event is fired when <see cref="UpdateGraphAsyncTask"/> is completed.
         /// </summary>
@@ -518,8 +513,16 @@ private void OnLibraryLoaded()
         /// </summary>
         private void LibraryLoaded(object sender, LibraryServices.LibraryLoadedEventArgs e)
         {
-            if(e.LibraryPaths.Any())
+            if (e.LibraryPaths.Any())
+            {
                 OnLibraryLoaded();
+            }
+        }
+
+        internal event EventHandler RequestCustomNodeRegistration;
+        internal void OnRequestCustomNodeRegistration()
+        {
+            RequestCustomNodeRegistration?.Invoke(null, EventArgs.Empty);
         }
 
         #region Implement IAstNodeContainer interface

diff --git a/src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs b/src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs
@@ -324,6 +324,8 @@ protected override void OnNodeRemoved(NodeModel node)
 
         private void LibraryLoaded(object sender, LibraryServices.LibraryLoadedEventArgs e)
         {
+            // Need to make compiled custom nodes available before running the graph.
+            EngineController.OnRequestCustomNodeRegistration();
             // Mark all nodes as dirty so that AST for the whole graph will be
             // regenerated.
             MarkNodesAsModifiedAndRequestRun(Nodes);
@@ -346,15 +348,7 @@ internal override void RequestRun()
                 // Make this RunSettings.RunEnabled private, introduce the new flag and remove the "executingTask" variable. 
                 if (RunSettings.RunEnabled || executingTask)
                 {
-                    // skip the execution if runs have been disabled - currently this flag is only set by the Package Loader
-                    if (!EngineController.DisableRun)
-                    {
-                        Run();
-                    }
-                    else
-                    {
-                        this.Log("Run has been disabled in the Engine Controller");
-                    }
+                    Run();
                 }   
             }
         }

diff --git a/src/DynamoCore/Models/DynamoModel.cs b/src/DynamoCore/Models/DynamoModel.cs
@@ -1039,6 +1039,7 @@ private void OnAsyncTaskStateChanged(DynamoScheduler sender, TaskStateChangedEve
         public void Dispose()
         {
             EngineController.TraceReconcliationComplete -= EngineController_TraceReconcliationComplete;
+            EngineController.RequestCustomNodeRegistration -= EngineController_RequestCustomNodeRegistration;
 
             ExtensionManager.Dispose();
             extensionManager.MessageLogged -= LogMessage;
@@ -1454,6 +1455,7 @@ protected void ResetEngineInternal()
         {
             if (EngineController != null)
             {
+                EngineController.RequestCustomNodeRegistration -= EngineController_RequestCustomNodeRegistration;
                 EngineController.TraceReconcliationComplete -= EngineController_TraceReconcliationComplete;
                 EngineController.MessageLogged -= LogMessage;
                 EngineController.Dispose();
@@ -1467,11 +1469,18 @@ protected void ResetEngineInternal()
 
             EngineController.MessageLogged += LogMessage;
             EngineController.TraceReconcliationComplete += EngineController_TraceReconcliationComplete;
+            EngineController.RequestCustomNodeRegistration += EngineController_RequestCustomNodeRegistration;
 
             foreach (var def in CustomNodeManager.LoadedDefinitions)
                 RegisterCustomNodeDefinitionWithEngine(def);
         }
 
+        private void EngineController_RequestCustomNodeRegistration(object sender, EventArgs e)
+        {
+            foreach (var def in CustomNodeManager.LoadedDefinitions)
+                RegisterCustomNodeDefinitionWithEngine(def);
+        }
+
         /// <summary>
         ///     Forces an evaluation of the current workspace by resetting the DesignScript VM.
         /// </summary>
@@ -1775,8 +1784,6 @@ private void ReloadDummyNodes()
                     // If the resolved node is also a dummy node, then skip it else replace the dummy node with the resolved version of the node. 
                     if (!(resolvedNode is DummyNode))
                     {
-                        // Disable graph runs temporarily while the dummy node is replaced with the resolved version of that node.
-                        EngineController.DisableRun = true;
                         currentWorkspace.RemoveAndDisposeNode(dn);
                         currentWorkspace.AddAndRegisterNode(resolvedNode, false);
 
@@ -1799,7 +1806,6 @@ private void ReloadDummyNodes()
                             connectorModel.Delete();
                             ConnectorModel.Make(startNode, endNode, connectorModel.Start.Index, connectorModel.End.Index, connectorModel.GUID);
                         }
-                        EngineController.DisableRun = false ;
                         resolvedDummyNode = true;
                     }
                 }

diff --git a/src/DynamoPackages/PackageLoader.cs b/src/DynamoPackages/PackageLoader.cs
@@ -296,18 +296,6 @@ public void LoadPackages(IEnumerable<Package> packages)
         {
             var packageList = packages.ToList();
 
-            // This fix is in reference to the crash reported in task: https://jira.autodesk.com/browse/DYN-2101
-            // TODO: https://jira.autodesk.com/browse/DYN-2120. we will be re-evaluating this workflow, to find the best clean solution.
-
-            // The reason for this crash is, when a new package is being loaded into the dynamo, it will reload 
-            // all the libraries into the VM. Since the graph execution runs are triggered asynchronously, it causes 
-            // an exception as the VM is reinitialized during the execution run. To avoid this, we disable the execution run's that
-            // are triggered while the package is still being loaded. Once the package is completely loaded and the VM is reinitialized,
-            // a final run is triggered that would execute the nodes in the workspace after resolving them.  
-
-            // Disabling the run here since new packages are being loaded. 
-            EngineController.DisableRun = true;
-
             foreach (var pkg in packageList)
             {
                 // If the pkg is null, then don't load that package into the Library.
@@ -317,9 +305,6 @@ public void LoadPackages(IEnumerable<Package> packages)
                 }
             }
 
-            // Setting back the DisableRun property back to false, as the package loading is completed.
-            EngineController.DisableRun = false;
-
             var assemblies = packageList
                 .SelectMany(p => p.LoadedAssemblies.Where(y => y.IsNodeLibrary))
                 .Select(a => a.Assembly)

diff --git a/test/Libraries/PackageManagerTests/PackageLoaderTests.cs b/test/Libraries/PackageManagerTests/PackageLoaderTests.cs
@@ -4,6 +4,7 @@
 using System.Linq;
 using Dynamo.Engine;
 using Dynamo.Extensions;
+using Dynamo.Graph.Nodes;
 using Dynamo.Graph.Nodes.CustomNodes;
 using Dynamo.Graph.Workspaces;
 using Dynamo.Search.SearchElements;
@@ -495,34 +496,21 @@ public void GetOwnerPackageReturnsNullForInvalidFunction()
             Assert.IsNull(foundPkg);
         }
 
-        /// This test is added for this task: https://jira.autodesk.com/browse/DYN-2101. 
-        /// A followup task is added https://jira.autodesk.com/browse/DYN-2120 to refactor the approach to this solution.
-        /// This test needs to be modified in that case. 
         [Test]
-        [Category("TechDebt")]
         public void PackageLoadExceptionTest()
         {
-            Boolean RunDisabledWhilePackageLoading = false;
-
             string openPath = Path.Combine(TestDirectory, @"core\PackageLoadExceptionTest.dyn");
             OpenModel(openPath);
 
             var loader = GetPackageLoader();
-            loader.PackgeLoaded += (package) =>
-            {
-                RunDisabledWhilePackageLoading = EngineController.DisableRun;
-            };
 
             // Load the package when the graph is open in the workspace. 
             string packageDirectory = Path.Combine(PackagesDirectory, "Ampersand");
             var pkg = loader.ScanPackageDirectory(packageDirectory);
             loader.LoadPackages(new List<Package> { pkg });
 
-            // Assert that the Run is disabled temporarily when the package is still loading. 
-            Assert.IsTrue(RunDisabledWhilePackageLoading);
-
-            // Assert that the DisableRun flag is set back to false, once the package loading is completed. 
-            Assert.IsFalse(EngineController.DisableRun);
+            // Dummy nodes are resolved, and more importantly, no exception was thrown.
+            Assert.AreEqual(0, CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<DummyNode>().Count());
         }
 
         /// <summary>
@@ -553,7 +541,32 @@ public void MixedPackageLoadTest()
 
             // Assert value of loaded CN is non-null.
             AssertNonNull("576f11ed5837460d80f2e354d853de68");
+        }
+
+        [Test]
+        public void LoadingAPackageWithBinariesDoesNotAffectCustomNodesUsedInHomeWorkspace()
+        {
+            // Open a custom node definition and a workspace where this custom node is used.
+            OpenModel(Path.Combine(TestDirectory, @"core\PackageLoadReset\test.dyf"));
+            OpenModel(Path.Combine(TestDirectory, @"core\PackageLoadReset\MissingNode.dyn"));
+
+            // Get the custom node.
+            var functionNodes = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Function>();
+            Assert.AreEqual(1, functionNodes.Count());
+            var functionNode = functionNodes.First();
+
+            // Custom node should be good before loading the package.
+            Assert.AreEqual(ElementState.Active, functionNode.State);
+            AssertPreviewValue(functionNode.AstIdentifierGuid, 7);
+
+            // Load a package which contains binaries, when the graph is open in the workspace. 
+            var loader = GetPackageLoader();
+            var pkg = loader.ScanPackageDirectory(Path.Combine(PackagesDirectory, "Mixed Package"));
+            loader.LoadPackages(new List<Package> { pkg });
 
+            // Custom node should remain good after loading the package.
+            Assert.AreEqual(ElementState.Active, functionNode.State);
+            AssertPreviewValue(functionNode.AstIdentifierGuid, 7);
         }
 
         [Test]

diff --git a/test/core/PackageLoadReset/MissingNode.dyn b/test/core/PackageLoadReset/MissingNode.dyn
@@ -0,0 +1,117 @@
+{
+  "Uuid": "63001d03-558d-4fae-a32c-767af1871c59",
+  "IsCustomNode": false,
+  "Description": null,
+  "Name": "MissingNode",
+  "ElementResolver": {
+    "ResolutionMap": {}
+  },
+  "Inputs": [],
+  "Outputs": [],
+  "Nodes": [
+    {
+      "ConcreteType": "Dynamo.Graph.Nodes.ZeroTouch.DSFunction, DynamoCore",
+      "NodeType": "FunctionNode",
+      "FunctionSignature": "QRImage.Color.Colors.GetAllColors",
+      "Id": "b3203366f0c048d8b1804f7b2dc3a6ae",
+      "Inputs": [],
+      "Outputs": [
+        {
+          "Id": "f23c1ee57e2140bd834b229eb3f7f6a9",
+          "Name": "var[]..[]",
+          "Description": "var[]..[]",
+          "UsingDefaultValue": false,
+          "Level": 2,
+          "UseLevels": false,
+          "KeepListStructure": false
+        }
+      ],
+      "Replication": "Auto",
+      "Description": "Return All Color\n\nColors.GetAllColors ( ): var[]..[]"
+    },
+    {
+      "ConcreteType": "Dynamo.Graph.Nodes.CustomNodes.Function, DynamoCore",
+      "FunctionSignature": "96dbab16-f1ce-4dfb-8e1c-1ec930eb7507",
+      "FunctionType": "Graph",
+      "NodeType": "FunctionNode",
+      "Id": "6f22a3d1c1294846aef586034146e89d",
+      "Inputs": [],
+      "Outputs": [
+        {
+          "Id": "6208c6fc61f04171af7433d1e483acc8",
+          "Name": "LuckyNumber",
+          "Description": "return value",
+          "UsingDefaultValue": false,
+          "Level": 2,
+          "UseLevels": false,
+          "KeepListStructure": false
+        }
+      ],
+      "Replication": "Auto",
+      "Description": ""
+    }
+  ],
+  "Connectors": [],
+  "Dependencies": [
+    "96dbab16-f1ce-4dfb-8e1c-1ec930eb7507"
+  ],
+  "NodeLibraryDependencies": [
+    {
+      "Name": "QRImage",
+      "Version": "0.0.4",
+      "ReferenceType": "Package",
+      "Nodes": [
+        "b3203366f0c048d8b1804f7b2dc3a6ae"
+      ]
+    }
+  ],
+  "Bindings": [],
+  "View": {
+    "Dynamo": {
+      "ScaleFactor": 1.0,
+      "HasRunWithoutCrash": true,
+      "IsVisibleInDynamoLibrary": true,
+      "Version": "2.7.0.8435",
+      "RunType": "Automatic",
+      "RunPeriod": "1000"
+    },
+    "Camera": {
+      "Name": "Background Preview",
+      "EyeX": -17.0,
+      "EyeY": 24.0,
+      "EyeZ": 50.0,
+      "LookX": 12.0,
+      "LookY": -13.0,
+      "LookZ": -58.0,
+      "UpX": 0.0,
+      "UpY": 1.0,
+      "UpZ": 0.0
+    },
+    "NodeViews": [
+      {
+        "ShowGeometry": true,
+        "Name": "Colors.GetAllColors",
+        "Id": "b3203366f0c048d8b1804f7b2dc3a6ae",
+        "IsSetAsInput": false,
+        "IsSetAsOutput": false,
+        "Excluded": false,
+        "X": 280.5,
+        "Y": 321.5
+      },
+      {
+        "ShowGeometry": true,
+        "Name": "test",
+        "Id": "6f22a3d1c1294846aef586034146e89d",
+        "IsSetAsInput": false,
+        "IsSetAsOutput": false,
+        "Excluded": false,
+        "X": 125.80000000000001,
+        "Y": 110.80000000000001
+      }
+    ],
+    "Annotations": [],
+    "X": 0.0,
+    "Y": 0.0,
+    "Zoom": 1.0
+  }
+}