#123 Support module nested references loading in Platform modular system. #124

yecli · 2019-01-11T18:08:14Z

Added test for nested module dependencies loading;
Reworked LoadContextAssemblyResolver assembly loading: every module assembly with all level dependencies is loaded into AssemblyLoadContext.Default to allow shared types to be used;
Set all module Web projects option to true to have the possibility to copy all dependencies from its bin folder (using this approach it should work on prod env where we cannot just restore packages). Futher investigation and revision could be done based on:
dotnet/cli#10502;
Set win10-x64 option to VirtoCommerce.ImageToolsModule.Web project to address problem with NuGet package Microsoft.Windows.Compatibility, which do not copy its dependency "System.Private.ServiceModel.dll" on publishing. Explicit reference to System.ServiceModel.Http 4.5.3 package did not help, setting RID helped, though it makes module platform dependent. Info:
.NET Core 2.0 self-contained app: Could not load file or assembly 'System.Private.ServiceModel, Version=4.1.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' dotnet/wcf#2349, https://stackoverflow.com/questions/50746592/system-private-servicemodel-missing-in-azure-app-service-at-runtime;
Removed redundant LoadContextAssemblyResolver singleton registration;
Changed Windows specific "" path separator to platform independent Path.DirectorySeparatorChar in LocalStorageModuleCatalog probing folder;
Excluded copying assembly related dlls that are in TPA;
Splitted assembly file extensions (dll, exe) and assembly service file extensions (pdb, xml, .deps.json) for using in filtering;
Fixed couple of Sonar warnings;

kostyrin · 2019-01-16T15:22:24Z

Modules/Module1/Module1.Test/AssemblyLoadTest.cs

+            // Explicit loading of "ICSharpCode.SharpZipLib.dll" (dependency of referenced assembly Module1.Data) to default ALC would make test passing even without proper dependency loading:
+            // System.Runtime.Loader.AssemblyLoadContext.Default.LoadFromAssemblyPath(Environment.ExpandEnvironmentVariables(@"%userprofile%\.nuget\packages\sharpziplib\1.1.0\lib\netstandard2.0\ICSharpCode.SharpZipLib.dll"));
+            var controllerType = webRuntimeAssembly.GetType("Module1.Web.Controllers.ThirdPartyController");
+            Assert.NotNull(controllerType);


move all asserts to down, look at https://github.com/dotnet-architecture/eShopOnWeb/blob/master/tests/UnitTests/ApplicationCore/Entities/BasketTests/AddItem.cs

kostyrin · 2019-01-16T15:22:34Z

Modules/Module1/Module1.Test/AssemblyLoadTest.cs

+    public class AssemblyLoadTest
+    {
+        [Fact]
+        public void TestAssemblyLoadThroughReflection()


read https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices

kostyrin · 2019-01-16T15:23:59Z

Modules/Module1/Module1.Test/Module1.Test.csproj

+
+  <ItemGroup>
+    <Reference Include="Microsoft.Extensions.Logging.Abstractions">
+      <HintPath>..\..\..\..\..\Program Files\dotnet\sdk\NuGetFallbackFolder\microsoft.extensions.logging.abstractions\2.1.1\lib\netstandard2.0\Microsoft.Extensions.Logging.Abstractions.dll</HintPath>


kostyrin · 2019-01-16T15:26:47Z

...dule-imageTools/VirtoCommerce.ImageToolsModule.Web/VirtoCommerce.ImageToolsModule.Web.csproj

-      <!--<CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>-->
+    <!--This line is necessary to copy all dependencies in the bin folder-->
+    <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>
+    <RuntimeIdentifier>win10-x64</RuntimeIdentifier>


maybe it is redundant

In implented dependency load system with copying all dependencies to output it is required. Will try to avoid dependency assembly copying by including NuGet and .NET caches to additional probing paths.
Explanation:
Set win10-x64 option to VirtoCommerce.ImageToolsModule.Web project to address problem with NuGet package Microsoft.Windows.Compatibility, which do not copy its dependency "System.Private.ServiceModel.dll" on publishing. Explicit reference to System.ServiceModel.Http 4.5.3 package did not help, setting RID helped, though it makes module platform dependent. Info:
dotnet/wcf#2349, https://stackoverflow.com/questions/50746592/system-private-servicemodel-missing-in-azure-app-service-at-runtime;

kostyrin · 2019-01-16T15:28:19Z

Modules/Module1/Module1.Data/Services/MyThirdPartyService.cs

+            var bytes = Encoding.UTF8.GetBytes(message);
+            using (var inStream = new MemoryStream(bytes))
+            {
+                var outStream = CreateToMemoryStream(inStream, "TestZipEntry");


using or close

kostyrin · 2019-01-16T15:40:35Z

VirtoCommerce.Platform.Modules/AssemblyLoading/DependencyReader.cs

+namespace VirtoCommerce.Platform.Modules.AssemblyLoading
+{
+    // Based on https://github.com/natemcmaster/DotNetCorePlugins/blob/ce1b5cc94e8e9ce018f9304368cc69897cc70cca/src/Plugins/Loader/DependencyContextExtensions.cs
+    public static class DependencyReader


it's not extension. bad idea use static, better to use with DI, read https://stackoverflow.com/questions/241339/when-to-use-static-classes-in-c-sharp

I think is ok, just need to change methods signatures to be more like to extensions class semantic

Read the SO post and think that it is still a suitable choice as it is true utility class with strictly defined function.
Could change it back to an extension.

tatarincev · 2019-01-25T06:49:10Z

VirtoCommerce.Platform.Modules/LoadContextAssemblyResolver.cs

+            // Fill _basePath and _additionalProbingPaths based on assemblyPath
+            // In fact we could add some _additionalProbingPaths to support assembly storages like NuGet package cache (), if we would need
+            var assemblyDirectory = Path.GetDirectoryName(assemblyUri.LocalPath);
+            _basePath = assemblyDirectory;


It is not really good to change object field inside read only functions, could we avoid this?

tatarincev · 2019-01-25T07:58:21Z

VirtoCommerce.Platform.Modules/LoadContextAssemblyResolver.cs

+        private bool SearchForLibrary(ManagedLibrary library, out string path)
+        {
+            // 1. Check for in _basePath + app local path
+            var localFile = Path.Combine(_basePath, library.AppLocalPath);


this code cognitive complexity can be simplified by generation array of search paths and do single linq call FirstOrDefault

Candidates pregeneration will add significant overhead. The common scenario would be just finding dependency inside _basePath, which generally speaking means one string creation, comparison and return. Sometimes dependency could be found inside _additionalProbingPaths, it could add couple of string creations. If we will pregenerate all the strings, method would become about 5 times slower.
And i think algorithm is readable and self-described, probably algorithm readability would not change much if we switch to pregeneration array.
Is generalization here costs 5-times performance loss?

tatarincev · 2019-01-25T07:59:41Z

VirtoCommerce.Platform.Modules/AssemblyLoading/DependencyReader.cs

+        private static IEnumerable<ManagedLibrary> ResolveRuntimeAssemblies(DependencyContext depContext, RuntimeFallbacks runtimeGraph)
+        {
+            var rids = GetRids(runtimeGraph);
+            return from library in depContext.RuntimeLibraries


Could you replace this linq expression to extensions method syntax?

tatarincev · 2019-01-25T08:03:05Z

VirtoCommerce.Platform.Modules/LoadContextAssemblyResolver.cs

                {
-                    bool assemblyPredicate(RuntimeLibrary runtime)
+                    var loadedAssembly = LoadAssemblyCached(dependency) ?? throw GenerateAssemblyLoadException(dependency.Name.Name, assemblyPath);
+                    if (mainAssemblyName.Equals(loadedAssembly.GetName().Name, StringComparison.OrdinalIgnoreCase))


Can be replaced with .EqualsInvariant

- Used PluginLoader for this;

…tem: - Reworked LoadContextAssemblyResolver assembly loading: every module assembly with all level dependencies is loaded into AssemblyLoadContext.Default to allow shared types to be used; - Set all module Web projects <CopyLocalLockFileAssemblies> option to true to have the possibility to copy all dependencies from its bin folder (using this approach it should work on prod env where we cannot just restore packages). Futher investigation and revision could be done based on: https://github.com/dotnet/cli/issues/10502; - Set <RuntimeIdentifier>win10-x64</RuntimeIdentifier> option to VirtoCommerce.ImageToolsModule.Web project to address problem with NuGet package Microsoft.Windows.Compatibility, which do not copy its dependency "System.Private.ServiceModel.dll" on publishing. Explicit reference to System.ServiceModel.Http 4.5.3 package did not help, setting RID helped, though it makes module platform dependent. Info: dotnet/wcf#2349, https://stackoverflow.com/questions/50746592/system-private-servicemodel-missing-in-azure-app-service-at-runtime; - Removed redundant LoadContextAssemblyResolver singleton registration; - Changed Windows specific "\" path separator to platform independent Path.DirectorySeparatorChar in LocalStorageModuleCatalog probing folder; - Fixed Sonar warning in ModuleInitializer.

…tem: - Excluded copying assembly related dlls that are in TPA; - Splitted assembly file extensions (dll, exe) and assembly service file extensions (pdb, xml, .deps.json) for using in filtering; - Removed unused McMaster.NETCore.Plugins package reference from VirtoCommerce.Platform.Modules.csproj; - Fixed couple of Sonar warnings;

…tem: - Implemented "api/platform/modules/restart" action using IApplicationLifetime.StopApplication();

…tem: - Added AdditionalProbingPath filling based on runtimeconfig.json and runtimeconfig.dev.json for Development env + NuGet and DotNet package caches for Production env; - Removed all NuGet assembly copying to Modules probing folder. So it works on DEV env, for PROD we will need to prepare packages with required dependencies; - Rebased on the latest master; Review: - Reworked test according to https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices; - Arranged DependencyReader as an extension; - Other fixes;

…tem: - Removed copying NuGet assemblies to output in Module1;

yecli requested review from tatarincev and kostyrin January 11, 2019 18:08

kostyrin reviewed Jan 17, 2019

View reviewed changes

tatarincev reviewed Jan 25, 2019

View reviewed changes

yecli added 5 commits January 25, 2019 15:14

- Added tests for module assembly loading;

bd8fbcc

- Used PluginLoader for this;

#123 Support module nested references loading in Platform modular sys…

f7b8c89

…tem: - Implemented "api/platform/modules/restart" action using IApplicationLifetime.StopApplication();

yecli force-pushed the feature/assembly-references-deep-loading branch from 44a179c to 3007f80 Compare January 25, 2019 21:16

#123 Support module nested references loading in Platform modular sys…

47d8b57

…tem: - Removed copying NuGet assemblies to output in Module1;

tatarincev approved these changes Feb 6, 2019

View reviewed changes

tatarincev merged commit dc56b97 into master Feb 6, 2019

tatarincev deleted the feature/assembly-references-deep-loading branch February 6, 2019 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#123 Support module nested references loading in Platform modular system. #124

#123 Support module nested references loading in Platform modular system. #124

yecli commented Jan 11, 2019

kostyrin Jan 16, 2019

kostyrin Jan 16, 2019

kostyrin Jan 16, 2019

kostyrin Jan 16, 2019

yecli Jan 25, 2019

kostyrin Jan 16, 2019

kostyrin Jan 16, 2019

tatarincev Jan 25, 2019

yecli Jan 25, 2019

tatarincev Jan 25, 2019

tatarincev Jan 25, 2019

yecli Jan 25, 2019

tatarincev Jan 25, 2019

tatarincev Jan 25, 2019

#123 Support module nested references loading in Platform modular system. #124

#123 Support module nested references loading in Platform modular system. #124

Conversation

yecli commented Jan 11, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment