-
Notifications
You must be signed in to change notification settings - Fork 636
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.
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
DYN-7691: Add assembly load contexts to packages #15424
base: master
Are you sure you want to change the base?
Changes from 3 commits
f314596
754a48b
ff6a99a
a1a2ecb
717b6a1
bd259e4
19d4f2f
241081b
196b596
111587f
7cd30d6
da7753f
d4bbd2b
d256457
2fb0e1a
3a23e80
d8d91fe
b90f7c0
561919b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
using System; | ||
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.Loader; | ||
|
||
namespace DynamoPackages | ||
{ | ||
internal class PkgAssemblyLoadContext : AssemblyLoadContext | ||
{ | ||
private AssemblyDependencyResolver _resolver; | ||
public PkgAssemblyLoadContext(string name, string pluginPath, bool unloadable = true) : base(name, unloadable) | ||
{ | ||
_resolver = new AssemblyDependencyResolver(pluginPath); | ||
|
||
} | ||
|
||
protected override Assembly Load(AssemblyName assemblyName) | ||
{ | ||
var oldAss = Default.Assemblies.FirstOrDefault(x => x.GetName().Equals(assemblyName)); | ||
if (oldAss != null) | ||
pinzart90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
// not sure about this. Hoping we can avoid loading assemblies that are alrady loaded in the default context. | ||
return null; | ||
} | ||
|
||
string assemblyPath = _resolver.ResolveAssemblyToPath(assemblyName); | ||
if (assemblyPath != null) | ||
{ | ||
var newAss = LoadFromAssemblyPath(assemblyPath); | ||
return newAss; | ||
} | ||
|
||
return null; | ||
} | ||
|
||
protected override IntPtr LoadUnmanagedDll(string unmanagedDllName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the fact that you can override this function mean that you can also load unmanaged assemblies in isolation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly no, you can only override the search algorithm |
||
{ | ||
string libraryPath = _resolver.ResolveUnmanagedDllToPath(unmanagedDllName); | ||
if (libraryPath != null) | ||
{ | ||
return LoadUnmanagedDllFromPath(libraryPath); | ||
} | ||
|
||
return IntPtr.Zero; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,13 +4,15 @@ | |
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.InteropServices; | ||
using System.Runtime.Loader; | ||
using Dynamo.Core; | ||
using Dynamo.Exceptions; | ||
using Dynamo.Extensions; | ||
using Dynamo.Interfaces; | ||
using Dynamo.Logging; | ||
using Dynamo.Models; | ||
using Dynamo.Utilities; | ||
using DynamoPackages; | ||
using DynamoPackages.Properties; | ||
using DynamoUtilities; | ||
|
||
|
@@ -200,9 +202,45 @@ private void TryLoadPackageIntoLibrary(Package package) | |
{ | ||
var dynamoVersion = VersionUtilities.PartialParse(DynamoModel.Version); | ||
|
||
package.EnumerateAdditionalFiles(); | ||
|
||
// If the additional files contained an extension manifest, then request it be loaded. | ||
var extensions = package.AdditionalFiles.Where( | ||
file => file.Model.Name.Contains("ExtensionDefinition.xml") || file.Model.Name.Contains("ViewExtensionDefinition.xml")).ToList(); | ||
|
||
AssemblyLoadContext pkgLoadContext = AssemblyLoadContext.Default; | ||
|
||
bool isolateAnyPackage = DynamoModel.FeatureFlags?.CheckFeatureFlag("IsolateAnyPackage", false) ?? false; | ||
bool isolateThisPackage = DynamoModel.FeatureFlags?.CheckFeatureFlag("IsolatePackage_" + package.Name, false) ?? false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should check per version ? |
||
if (isolateAnyPackage || isolateThisPackage) | ||
{ | ||
if (extensions.Count() == 1) | ||
{ | ||
var ext = extensions[0]; | ||
string entryPoint = string.Empty; | ||
try | ||
{ | ||
entryPoint = ExtensionLoader.GetExtensionPath(ext.Model.FullName); | ||
if (!string.IsNullOrEmpty(entryPoint)) | ||
{ | ||
pkgLoadContext = new PkgAssemblyLoadContext(package.Name + "@" + package.VersionName, Path.Combine(package.RootDirectory, entryPoint)); | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
Log(ex); | ||
} | ||
} | ||
else if (package.Header.node_libraries.Any()) | ||
{ | ||
string entryPoint = new AssemblyName(package.Header.node_libraries.First()).Name + ".dll"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there could be more than one node_library right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try the root package location first...then I'll circle back to this if needed |
||
pkgLoadContext = new PkgAssemblyLoadContext(package.Name + "@" + package.VersionName, Path.Combine(package.BinaryDirectory, entryPoint)); | ||
} | ||
} | ||
|
||
pinzart90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
List<Assembly> blockedAssemblies = new List<Assembly>(); | ||
// Try to load node libraries from all assemblies | ||
foreach (var assem in package.EnumerateAndLoadAssembliesInBinDirectory()) | ||
foreach (var assem in package.EnumerateAndLoadAssembliesInBinDirectory(pkgLoadContext)) | ||
{ | ||
if (assem.IsNodeLibrary) | ||
{ | ||
|
@@ -250,8 +288,6 @@ private void TryLoadPackageIntoLibrary(Package package) | |
var customNodes = OnRequestLoadCustomNodeDirectory(package.CustomNodeDirectory, packageInfo); | ||
package.LoadedCustomNodes.AddRange(customNodes); | ||
|
||
package.EnumerateAdditionalFiles(); | ||
|
||
// If the additional files contained an extension manifest, then request it be loaded. | ||
var extensionManifests = package.AdditionalFiles.Where( | ||
file => file.Model.Name.Contains("ExtensionDefinition.xml") && !file.Model.Name.Contains("ViewExtensionDefinition.xml")).ToList(); | ||
|
@@ -751,11 +787,11 @@ internal static void CleanSharedPublishLoadContext(MetadataLoadContext mlc) | |
/// <param name="filename">The filename of a DLL</param> | ||
/// <param name="assem">out Assembly - the passed value does not matter and will only be set if loading succeeds</param> | ||
/// <returns>Returns true if success, false if BadImageFormatException (i.e. not a managed assembly)</returns> | ||
internal static bool TryLoadFrom(string filename, out Assembly assem) | ||
internal static bool TryLoadFrom(AssemblyLoadContext alc, string filename, out Assembly assem) | ||
{ | ||
try | ||
{ | ||
assem = Assembly.LoadFrom(filename); | ||
assem = alc.LoadFromAssemblyPath(filename); | ||
return true; | ||
} | ||
catch (FileLoadException e) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this needs to be isolated or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh - this is just for publishing, I mean, I still think what I stated is the way to go, but maybe less important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might add a new task for this