DYN-7691: Add assembly load contexts to packages #15424

pinzart90 · 2024-08-12T16:05:27Z

Purpose

https://jira.autodesk.com/browse/DYN-7691

Add assemblyLoadCOntexts for all packages loaded in Dynamo
Use feature flags
Did not do dynamic unload - still not clear how to make it work for packages

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

Enable assembly isolation for packages.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

pinzart90 · 2024-08-12T16:06:31Z

Unload is still not working as expected (after the unloading event kicks in, the package assemblies still cannot be deleted...)
Still looking into a couple of things

pinzart90 · 2024-08-12T16:07:03Z

Will probably need to filter out packages that implemented isolation on their own

pinzart90 · 2024-08-12T16:08:50Z

src/DynamoPackages/PackageLoader.cs

+                AssemblyLoadContext pkgLoadContext = AssemblyLoadContext.Default;
+
+                bool isolateAnyPackage = DynamoModel.FeatureFlags?.CheckFeatureFlag("IsolateAnyPackage", false) ?? false;
+                bool isolateThisPackage = DynamoModel.FeatureFlags?.CheckFeatureFlag("IsolatePackage_" + package.Name, false) ?? false;


Should check per version ?
probably would be helpful for IronPython which already has isolation. The current version should not be isolated by Dynamo probably...but all other could be...

github-actions · 2024-08-12T16:29:36Z

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

src/DynamoPackages/PackageLoader.cs

src/DynamoPackages/Package.cs

src/DynamoPackages/PackageAssemblyLoadContext.cs

mjkkirschner · 2024-08-13T14:03:48Z

src/DynamoPackages/PackageLoader.cs

+                    }
+                    else if (package.Header.node_libraries.Any())
+                    {
+                        string entryPoint = new AssemblyName(package.Header.node_libraries.First()).Name + ".dll";


there could be more than one node_library right?

I'll try the root package location first...then I'll circle back to this if needed

src/DynamoPackages/Package.cs

mjkkirschner · 2024-08-13T14:06:26Z

I think if we were going to just turn this using feature flags, there would be value in somehow collecting some analytics though I'm not sure how we can prove a package "works" after being loaded this way without asking people since our testing system for package is non existent.

pinzart90 · 2024-08-13T19:58:22Z

I think if we were going to just turn this using feature flags, there would be value in somehow collecting some analytics though I'm not sure how we can prove a package "works" after being loaded this way without asking people since our testing system for package is non existent.

I could write a test, that downloads each package at a time and loads it in isolation. It would take a long time...so should be a nightly thing...
I thing I have some of that code already from the email notification work

pinzart90 · 2024-10-28T19:25:20Z

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs

@@ -2218,7 +2219,7 @@ private void AddDllFile(string filename)
            {
                // we're not sure if this is a managed assembly or not
                // we try to load it, if it fails - we add it as an additional file
-                var result = PackageLoader.TryLoadFrom(filename, out Assembly assem);
+                var result = PackageLoader.TryLoadFrom(AssemblyLoadContext.Default, filename, out Assembly assem);


Not sure if this needs to be isolated or not.

umm, certainly seems so? I guess this should use an MLC to determine what type of binary it is before loading it into the real ALC for this package?

oh - this is just for publishing, I mean, I still think what I stated is the way to go, but maybe less important.

I might add a new task for this

github-actions

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7691

aparajit-pratap · 2024-11-05T02:16:02Z

src/DynamoPackages/PackageLoader.cs

+                    string pkgs = DynamoModel.FeatureFlags?.CheckFeatureFlag("IsolatePackages", string.Empty);
+                    if (!string.IsNullOrEmpty(pkgs))
+                    {
+                        foreach (var x in pkgs.Split(","))
+                        {
+                            packagesToIsolate.Add(x);
+                        }
+                    }
+
+
+                    pkgs = DynamoModel.FeatureFlags?.CheckFeatureFlag("DoNotIsolatePackages", string.Empty);


Who specifies which packages should be isolated and which shouldn't? If we do, how do we decide that? Is this meant for only the packages that are owned by us? If not, how would this be scalable? Am I right in assuming that in the long run, we'll be getting rid of these feature flags to load all packages in isolation by default (except for the ones that already use ALCs, i.e. if there is a way for us to know that ahead of time)?

I foresee that some packages might have issues with being loaded in isolation (maybe the interaction between the isolated types vs the ones in the main load context).
So the main reason for adding feature flags is to slowly roll it out for all packages and have the option to disable it for a select few(like the ones that are already isolated on the package side )

As for “how do we decide which ones to isolate”:
I was thinking to take a couple at a time from the list I used to notify package maintainers about the dotnet8 migration.

pinzart90 · 2024-11-06T03:54:44Z

src/DynamoUtilities/DynamoFeatureFlagsManager.cs

+        }
+
+        // Useful for mocking in tests
+        internal IFFlags flags { get; set; } = new FFlags();


I may have tunnel vision, but I could not think of anything else to easily enable mocking.
I seem to need mocking because I am using the CheckFeatureFlag function in the PackageLoader

I tried to avoid refactoring the entire DynamoFeatureFlagsManager to be mockable

@mjkkirschner any other options that you see?

The feature flags cli already returns a set of static test flags in test mode - is that sufficient?

pinzart90 · 2024-11-06T03:55:04Z

src/DynamoUtilities/DynamoFeatureFlagsManager.cs

+        {
+            // with testmode = true, the call goes through an interface so that we can intercept it with Mock
+            // with testmode = false, the call simply goes to the CheckFeatureFlagInternal
+            return testmode ? flags.CheckFeatureFlag(this, featureFlagKey, defaultval) : CheckFeatureFlagInternal(featureFlagKey, defaultval);


just a performance thing I guess

mjkkirschner · 2024-11-07T16:48:28Z

I still think that control should be in the hands of users for opting out of isolated loading to avoid issues when something breaks for their local setup.

Feature flags do not seem sufficient for this purpose.

A simple config flag in prefs or config file could work.

pinzart90 · 2024-11-07T16:55:31Z

I still think that control should be in the hands of users for opting out of isolated loading to avoid issues when something breaks for their local setup.

Feature flags do not seem sufficient for this purpose.

A simple config flag in prefs or config file could work.

Preferences might be better know to dynamo users..

aparajit-pratap · 2024-11-08T14:03:31Z

test/Libraries/PackageManagerTests/PackageLoaderTests.cs

@@ -652,13 +652,21 @@ public void PlacingCustomNodeInstanceFromPackageRetainsCorrectPackageInfoState()
        [Test]
        public void LoadPackagesInAssemblyIsolation()
        {
+            var ff = new Mock<DynamoUtilities.IFFlags>();
+            DynamoModel.FeatureFlags.flags = ff.Object;


Can you make flags uppercase as per coding standards?

aparajit-pratap · 2024-11-08T14:16:50Z

src/DynamoPackages/PackageAssemblyLoadContext.cs

+            return null;
+        }
+
+        protected override IntPtr LoadUnmanagedDll(string unmanagedDllName)


Does the fact that you can override this function mean that you can also load unmanaged assemblies in isolation?

Sadly no, you can only override the search algorithm

pinzart90 added 3 commits August 12, 2024 11:53

add alc for packages

f314596

Update PackageLoader.cs

754a48b

update

ff6a99a

pinzart90 changed the title ~~add assembly load contexts to packages~~ [WIP] add assembly load contexts to packages Aug 12, 2024

pinzart90 requested review from mjkkirschner, aparajit-pratap and sm6srw August 12, 2024 16:05

pinzart90 commented Aug 12, 2024

View reviewed changes

src/DynamoPackages/PackageLoader.cs Outdated Show resolved Hide resolved