Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. #14576

mjkkirschner · 2023-11-06T22:43:40Z

Purpose

ReflectionOnlyLoadContext is no longer supported in net6. We use it when publishing packages to allow authors to add and remove dependencies of different versions of assemblies since those loads would usually fail if loaded in the same context as the running application.

A lot of this can be improved later when/if we move to assembly load contexts per package - but we'll need to do that when we have more time.

For now this tries to maintain similar behavior to the legacy behavior - it's not exact as MLC do not behave exactly like ReflectionOnlyLoadContexts -

This PR uses one shared static MLC, analogous to the shared reflection only load context that lived for application lifetime.

This PR also adds a new dependency on version 6 of the meta data load context package.

Questions for the team:

do I need to add something to the about box for the metaDataLoad package? I think it's also in the runtime folder. (ie part of dotnet runtime)

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

Move from ReflectionOnlyLoadContext to MetadataLoadContext for publishing packages.

remove test only code one actual test failure

dyn files still broken.

rename method

src/DynamoPackages/PackageLoader.cs

github-actions · 2023-11-06T22:51:57Z

⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::1 new file(s) have been added
(Updated: 2023-11-09-20:24:08)

pinzart90 · 2023-11-07T02:30:57Z

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs

+            var resolver = new PathAssemblyResolver(assemblyPaths);
+            var mlc = new MetadataLoadContext(resolver);
+
+            //TODO for now, we leak the mlc, but it's unlikely users will publish


how are we leaking the mlc ?
is that why we are not cleaning up the mlc ?
There might be a lot of assemblies loaded from the runtime libs....are you saying it is not probable that a user publishes multiple times in the same session ?

yes, I am not disposing the mlc here - its assemblies seem to live as package dependencies for quite some time. If we moved to only using one MLC per session I think there would be a smaller chance of any appreciable memory leak - ie it would be more like using the ReflectionLoadContext.

Yes I think it's unlikely that a user publishes multiple times in a single session.

At this point it's not clear to me that there is ever a good time to dispose these mlcs.

What do you think?

What happens when you dispose the mlc at the end of this function? Is it holding onto assembly references that are also owned by the package?

The assemblies loaded in the mlc seem to be stored in the package view model
As Mike said, it is not obvious how long they are kept around

If we try to use a single mlc, would that increase the chance of conflicts that were not there with reflectionOnly ?
Also, seems like mlc is a little more permissive than reflectionOnly. As an example I think BadImageFormatException exceptions will not eever be thrown.

We should test this feature as much as possible (in revit and sandbox) since we do not know all the implications yet

@pinzart90 I've tested your concern, and even though the docs don't mention it, it throws the same exception when loading a native assembly.

Also I have written a test for it.

pinzart90 · 2023-11-07T02:50:54Z

src/DynamoPackages/PackageLoader.cs

+                //where reflection only context would throw FileLoadException
+                //we try to maintain that legacy behavior for Dynamo 3.0.
+                if (mlc.GetAssemblies().Select(x => x.GetName() == assemName).Any()) {
+                    throw new FileLoadException(filename);


does this happen ?

yes, it happens in the tests that I have removed the failure category from in this PR. They rely on these exceptions being thrown for .dlls with the same mvid.

the metaDataLoadContext will throw an exception by default for assemblies with the same name with different mvids - but apparently the behavior for reflection only load context was different.

I've made this smarter I think.

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs

mjkkirschner · 2023-11-09T02:21:46Z

test/DynamoCoreWpfTests/PublishPackageViewModelTests.cs

@@ -17,7 +18,7 @@ namespace DynamoCoreWpfTests
    public class PublishPackageViewModelTests: DynamoViewModelUnitTest
    {

-        [Test, Category("Failure")]


this seems to be passing... this test is unrelated to this task so if it fails on CI I will just revert this.

aparajit-pratap · 2023-11-09T03:14:25Z

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs

+            var runtimeAssemblies = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll");
+            // Create PathAssemblyResolver that can resolve assemblies using the created list.
+            var resolver = new PathAssemblyResolver(runtimeAssemblies);
+             return new MetadataLoadContext(resolver);


Why are we no longer doing this:

//and all the package assemblies. foreach (var assemblyFile in pkg.EnumerateAssemblyFilesInPackage()) { assemblyPaths.Add(assemblyFile); } // Create PathAssemblyResolver that can resolve assemblies using the created list. var resolver = new PathAssemblyResolver(assemblyPaths); var mlc = new MetadataLoadContext(resolver);

I found that it does not appear to be necessary to add the assemblies to the MLC as we pass the full path to the file we want to load.

Additionally, we only need the dependencies loaded if want to inspect those types - but we don't do much with the assemblies as far as I can tell except for accessing the location and assembly name.

In an MLC the dependencies are not required to load the assembly AFAIK.

I do not see any immediate need to use those types...but as soon as we do ...we will have exceptions thrown. THese assemblies are not exposed publicly right ?

no, and in addition this would have caused the same issue with ReflectionOnlyLoadContext.

actually, the PublicPackageViewModel.Assemblies property is public and so is the class. These classes are only used in the UI layer though AFAIK and are not easily accessible from anywhere but our views, ie not from node or extensions without clients hacking around.

So, I'm a bit confused after reading this, just trying to understand it better. The docs seem to say that the resolver that is passed to the mlc resolves assemblies that are dependencies of the assembly being loaded (by the mlc). I can understand that this should include runtime assemblies, which most likely the assembly being loaded (for inspection) has a dependency on but won't it also have dependencies most likely on the other helper assemblies that are in the same package folder itself for the package that's being published? Or are you saying that since the assembly being loaded and the other assemblies are in the same location, that isn't required? Maybe I'm missing something.

yes, the binary we are loading very likely will have dependencies on other binaries in the package - but it doesn't matter. It does not matter because the MLC won't fail to load a binary simply because a dependency is not loaded.

It only becomes an issue when you try to inspect a type and that type has a dependency on an assembly the resolver cannot resolve.

But in our case, as far as I can tell, we don't do any inspection, we just look at the assembly name and location... we're just using the MLC to do some sanity checking that the assembly is a real managed assembly.

ReflectionLoadContext also behaved like this, except instead of a pathresolver, you had to implement the ReflectionOnlyAssemblyResolve which we did not do.

pinzart90 · 2023-11-09T14:50:49Z

src/DynamoPackages/PackageLoader.cs

+                    //if loading the assembly did not actually add a new assembly to the MLC
+                    //then we've loaded it already, and our current behavior is to
+                    //disable publish when a package contains the same assembly twice.
+                    if (mlcAssemblies2.Count() == mlcAssemblies.Count())


is this the best way to check if the assembly has been loaded successfully ?
assem = mlc.LoadFromAssemblyPath(filename);
assem != null is not good enough ?

I'm not checking if the assembly has been loaded successfully, I am checking if it had been loaded already.

pinzart90

couple of questions then LGTM

…data2

mjkkirschner · 2023-11-09T21:48:39Z

passed:
https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1241/
waiting for master-5

aparajit-pratap · 2023-11-10T01:06:57Z

test/DynamoCoreWpfTests/PublishPackageViewModelTests.cs

@@ -99,7 +100,34 @@ public void CanPublishLateInitializedJsonCustomNode()
        }


-        [Test, Category("Failure")]
+        [Test]
+        public void NewPackageDoesNotThrow_NativeBinaryIsAddedAsAdditionalFile_NotBinary()


Does the dependencies field in the pkg.json being empty indicate that the native assembly is not added as a dependent binary but as an additional file?

the dependencies field points to other packages

mjkkirschner and others added 16 commits October 30, 2023 21:07

fix first test

f21d3cd

work on docs browser

26c6e27

remove test only code one actual test failure

fix test and broken docs image loading from pkgs

7bc8727

dyn files still broken.

add another test, fix dyn loading

dd080a2

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into pkgtest

7504a03

fix asm version

7ddb5b3

rename method

replace binary with renamed one

b76d815

try to fix tests

5d528cb

Merge branch 'master' into pkgtest

ba7240c

Merge branch 'master' into pkgtest

cbca65f

rename package with node docs assembly and namespace

ec946b6

review comments

e74a4f4

conflict

e498b45

merge undid my changes

8abc4fe

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into pkgtest

fbd1d69

fix pkg publish version tests

4c59043

mjkkirschner requested review from pinzart90, sm6srw and aparajit-pratap November 6, 2023 22:43

mjkkirschner commented Nov 6, 2023

View reviewed changes

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

QilongTang added this to the 3.0 milestone Nov 7, 2023

pinzart90 reviewed Nov 7, 2023

View reviewed changes

mjkkirschner commented Nov 7, 2023

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Nov 8, 2023

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs Outdated Show resolved Hide resolved

mjkkirschner added 2 commits November 8, 2023 21:11

add test

b22c889

add package with native dep

a4ef81c

mjkkirschner changed the title ~~Replace reflection only load context with metadataLoadContext for publishing packages.~~ Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. Nov 9, 2023

mjkkirschner commented Nov 9, 2023

View reviewed changes

aparajit-pratap reviewed Nov 9, 2023

View reviewed changes

pinzart90 reviewed Nov 9, 2023

View reviewed changes

pinzart90 approved these changes Nov 9, 2023

View reviewed changes

mjkkirschner added 3 commits November 9, 2023 12:33

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into meta…

b10d203

…data2

fix some tests

c77be09

fix tests

8d7ed18

mjkkirschner merged commit ace131f into DynamoDS:master Nov 9, 2023
17 checks passed

mjkkirschner deleted the pkgtest branch November 9, 2023 23:58

aparajit-pratap reviewed Nov 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. #14576

Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. #14576

mjkkirschner commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

pinzart90 Nov 7, 2023

mjkkirschner Nov 7, 2023

aparajit-pratap Nov 8, 2023

pinzart90 Nov 8, 2023

pinzart90 Nov 8, 2023

pinzart90 Nov 8, 2023

mjkkirschner Nov 9, 2023

pinzart90 Nov 7, 2023

mjkkirschner Nov 7, 2023

mjkkirschner Nov 9, 2023

mjkkirschner Nov 9, 2023

aparajit-pratap Nov 9, 2023

mjkkirschner Nov 9, 2023 •

edited

Loading

pinzart90 Nov 9, 2023

mjkkirschner Nov 9, 2023

mjkkirschner Nov 9, 2023 •

edited

Loading

aparajit-pratap Nov 10, 2023 •

edited

Loading

mjkkirschner Nov 10, 2023 •

edited

Loading

pinzart90 Nov 9, 2023

mjkkirschner Nov 9, 2023

pinzart90 left a comment

mjkkirschner commented Nov 9, 2023

aparajit-pratap Nov 10, 2023 •

edited

Loading

mjkkirschner Nov 10, 2023

Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. #14576

Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. #14576

Conversation

mjkkirschner commented Nov 6, 2023 • edited Loading

Purpose

Declarations

Release Notes

github-actions bot commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 left a comment

Choose a reason for hiding this comment

mjkkirschner commented Nov 9, 2023

aparajit-pratap Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

mjkkirschner Nov 9, 2023 •

edited

Loading

mjkkirschner Nov 9, 2023 •

edited

Loading

aparajit-pratap Nov 10, 2023 •

edited

Loading

mjkkirschner Nov 10, 2023 •

edited

Loading

aparajit-pratap Nov 10, 2023 •

edited

Loading