Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register custom node before package load reset #10591

Merged
merged 9 commits into from
Apr 29, 2020
Merged

Register custom node before package load reset #10591

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
db7a772
Place VM reset in Runtime Core mutex context
mmisol Apr 17, 2020
8a9d04e
Mark UpdateCmdLineInterpreter as obsolete
mmisol Apr 17, 2020
1998100
Remove DisableRun flag
mmisol Apr 17, 2020
ca6ed8e
Register custom node before package load reset
mmisol Apr 23, 2020
3533097
Merge remote-tracking branch 'origin' into remove_disable_run
mmisol Apr 23, 2020
49a8a74
Add test
mmisol Apr 23, 2020
19d956c
Change test to not use geometry
mmisol Apr 23, 2020
1ad4ce4
Assert node status before package load
mmisol Apr 23, 2020
8ea3c63
Queue CNs on HW library loaded
mmisol Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/DynamoCore/Engine/EngineController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -518,8 +513,18 @@ private void OnLibraryLoaded()
/// </summary>
private void LibraryLoaded(object sender, LibraryServices.LibraryLoadedEventArgs e)
{
if(e.LibraryPaths.Any())
if (e.LibraryPaths.Any())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so libraryPaths will include paths to custom nodes or only binaries?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used for binaries only

{
// Need to make compiled custom nodes available before resetting the VM
OnRequestCustomNodeRegistration();
OnLibraryLoaded();
Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now it looks like the custom node definitions are scheduled to be added to the VM before it is reset in OnLibraryLoaded and therefore could be added before it is reset, which could again wipe out all of the definitions potentially. Wouldn't it be safer to call OnRequestCustomNodeRegistration after OnLibraryLoaded?

Copy link
Collaborator Author

@mmisol mmisol Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the method names might be misleading. I named this OnRequestCustomNodeRegistration mainly because it calls RegisterCustomNodeDefinitionWithEngine downstream. However, that function does not register anything with the RuntimeCore, that's the misleading part. What this does is fill a queue that gets processed afterwards, when calling OnLibraryLoaded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. The pendingCustomNodeSyncData queue is processed in EngineController.ProcessPendingCustomNodeSyncData, which is called in HomeWorkspaceModel.Run. Run is not called from OnLibraryLoaded. My question is, what is the harm in calling OnRequestCustomNodeRegistration after the VM is reset in OnLibraryLoaded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run is not called from OnLibraryLoaded

You are right, I had overlooked this. Looking at the sequence of calls, the most appropriate place for this code should be in HomeWorkspaceModel.LibraryLoaded, which is the event handler triggering the run. That way we avoid doing extra work if no home workspace is open. Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments below.

}
}

internal event EventHandler RequestCustomNodeRegistration;
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
private void OnRequestCustomNodeRegistration()
{
RequestCustomNodeRegistration?.Invoke(null, EventArgs.Empty);
}

#region Implement IAstNodeContainer interface
Expand Down
10 changes: 1 addition & 9 deletions src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,15 +346,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();
}
}
}
Expand Down
12 changes: 9 additions & 3 deletions src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,7 @@ private void OnAsyncTaskStateChanged(DynamoScheduler sender, TaskStateChangedEve
public void Dispose()
{
EngineController.TraceReconcliationComplete -= EngineController_TraceReconcliationComplete;
EngineController.RequestCustomNodeRegistration -= EngineController_RequestCustomNodeRegistration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for make sure we unsubscribe. Things like this take much more time to identify later and do memory analysis


ExtensionManager.Dispose();
extensionManager.MessageLogged -= LogMessage;
Expand Down Expand Up @@ -1454,6 +1455,7 @@ protected void ResetEngineInternal()
{
if (EngineController != null)
{
EngineController.RequestCustomNodeRegistration -= EngineController_RequestCustomNodeRegistration;
EngineController.TraceReconcliationComplete -= EngineController_TraceReconcliationComplete;
EngineController.MessageLogged -= LogMessage;
EngineController.Dispose();
Expand All @@ -1467,11 +1469,18 @@ protected void ResetEngineInternal()

EngineController.MessageLogged += LogMessage;
EngineController.TraceReconcliationComplete += EngineController_TraceReconcliationComplete;
EngineController.RequestCustomNodeRegistration += EngineController_RequestCustomNodeRegistration;
Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just move all these subscriptions to the constructor for EngineController?

Copy link
Contributor

@aparajit-pratap aparajit-pratap Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Can we move the EngineController.OnLibraryLoaded call out of the EngineController constructor to after this line and
  2. Move the call to OnRequestCustomNodeRegistration within EngineController.OnLibraryLoaded here:

    Dynamo/src/DynamoCore/Engine/EngineController.cs

    Line 514 in c00c80b

    }
  3. This way we can remove the foreach here:

    Dynamo/src/DynamoCore/Models/DynamoModel.cs

    Line 1471 in c00c80b

    foreach (var def in CustomNodeManager.LoadedDefinitions)
    as the same code will already be called within EngineController.OnLibraryLoaded in OnRequestCustomNodeRegistration, thereby leading to more code reuse.

You could make EngineController.OnLibraryLoaded internal.


foreach (var def in CustomNodeManager.LoadedDefinitions)
RegisterCustomNodeDefinitionWithEngine(def);
}

private void EngineController_RequestCustomNodeRegistration(object sender, EventArgs e)
{
foreach (var def in CustomNodeManager.LoadedDefinitions)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the timing of this - as I think @reddyashish mentioned - what if the package to be loaded contains custom nodes, would those exist in the LoadedDefinitions list before this call is made?

Copy link
Collaborator Author

@mmisol mmisol Apr 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems worth investigating. Taking a look at the interaction during the PackgeLoaded event, fired on the first stage of LoadPackages, there are some steps related to custom nodes that I did not fully capture. I'll take a closer look and see if I can find where this happens and add it to the internal documentation.

RegisterCustomNodeDefinitionWithEngine(def);
}

/// <summary>
/// Forces an evaluation of the current workspace by resetting the DesignScript VM.
/// </summary>
Expand Down Expand Up @@ -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);

Expand All @@ -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;
}
}
Expand Down
15 changes: 0 additions & 15 deletions src/DynamoPackages/PackageLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand Down
36 changes: 21 additions & 15 deletions test/Libraries/PackageManagerTests/PackageLoaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -487,34 +488,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>
Expand Down Expand Up @@ -545,7 +533,25 @@ 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"));

// 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 in workspace should not have any warnings on them.
var functionNodes = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Function>();
Assert.AreEqual(1, functionNodes.Count());
var functionNode = functionNodes.First();
Assert.AreEqual(ElementState.Active, functionNode.State);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also assert this custom node returns the correct value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm not exactly sure what the example is doing, as I took it straight from @aparajit-pratap repro. I can probably check that it returns some value at least.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example load the dyf first then followed by the dyn home workspace, at this point, we should have a working custom node and a dummy node. Then we load the package and make sure custom node still function correctly. I would encourage before and after the package load, we assert custom node in Active State and give correct value.

}

[Test]
Expand Down
117 changes: 117 additions & 0 deletions test/core/PackageLoadReset/MissingNode.dyn
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
{
"Uuid": "63001d03-558d-4fae-a32c-767af1871c59",
"IsCustomNode": false,
"Description": null,
"Name": "Home",
"ElementResolver": {
"ResolutionMap": {}
},
"Inputs": [],
"Outputs": [],
"Nodes": [
{
"ConcreteType": "Dynamo.Graph.Nodes.CustomNodes.Function, DynamoCore",
"FunctionSignature": "96dbab16-f1ce-4dfb-8e1c-1ec930eb7507",
"FunctionType": "Graph",
"NodeType": "FunctionNode",
"Id": "1c338137ab98476aaead678d47a583f0",
"Inputs": [],
"Outputs": [
{
"Id": "30682c8b9c3c4cc6963ee6e8d7dca18d",
"Name": "Point",
"Description": "return value",
"UsingDefaultValue": false,
"Level": 2,
"UseLevels": false,
"KeepListStructure": false
}
],
"Replication": "Auto",
"Description": ""
},
{
"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[]..[]"
}
],
"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.8287",
"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": [
{
"Id": "1c338137ab98476aaead678d47a583f0",
"IsSetAsInput": false,
"IsSetAsOutput": false,
"Name": "test",
"ShowGeometry": true,
"Excluded": false,
"X": 110.0,
"Y": 133.0
},
{
"Id": "b3203366f0c048d8b1804f7b2dc3a6ae",
"IsSetAsInput": false,
"IsSetAsOutput": false,
"Name": "Colors.GetAllColors",
"ShowGeometry": true,
"Excluded": false,
"X": 280.5,
"Y": 321.5
}
],
"Annotations": [],
"X": 0.0,
"Y": 0.0,
"Zoom": 1.0
}
}
Loading