Add support for views + SingleFileExe #24925

pranavkm · 2020-08-14T23:38:25Z

Exclude views dll \ pdb from single file archive
When loading related parts, check if the Assembly is already loaded.

~~I tried writing a template test for this, but we don't build rid specific M.A.A before the template tests are available. I'll add a CTI item for this.~~

davidfowl · 2020-08-14T23:51:12Z

src/Mvc/Mvc.Core/src/ApplicationParts/RelatedAssemblyAttribute.cs

+                }
+                catch (IOException)
+                {
+                    // The assembly isn't already loaded. Patience, we'll attempt to load it from disk next.


First chance exceptions creeping into ASP.NET Core... Making me sad. Do we need this check?

Is this perf critical in some sense? If not, I think a cleaner way would be to skip this check and simply try to load the assembly. If it's already loaded, runtime will relatively quickly return the loaded assembly from cache.
The only problem might be for assemblies which are in the single-file bundle, but could also appear on disk (possibly in a different version) - is that something we need to handle?

This is not a perf critical scenario. It happens one time at the start of the application. Could you elaborate what "simply try to load the assembly" means? What API are we talking of?

The only problem might be for assemblies which are in the single-file bundle, but could also appear on disk (possibly in a different version) - is that something we need to handle?

It's possible to abuse the api to do this, but it's not something we would claim to support.

"simply try to load the assembly" - I meant basically remove the check and just always run the code below it - which will find the file on disk and load it (hopefully via something like AssemblyLoadContext.LoadFromAssemblyPath). Runtime has a cache which per-ALC stores all loaded assemblies - if it's asked to load the same file again, it will simply return the already loaded assembly. That check happens relatively early on and is pretty fast (no file IO if I remember correctly).

davidfowl · 2020-08-14T23:53:18Z

src/Mvc/Mvc.Core/src/ApplicationParts/RelatedAssemblyAttribute.cs

@@ -106,7 +123,7 @@ public static IReadOnlyList<Assembly> GetRelatedAssemblies(Assembly assembly, bo
                    }
                }

-                var relatedAssembly = loadFile(relatedAssemblyLocation);
+                relatedAssembly = loadFile(relatedAssemblyLocation);


Also this LoadFile isn't correct as it loads the assembly into a different load context. @vitek-karas What would we be using instead? AssemblyLoadContext.GetLoadContext(typeof(RelatedAssemblyAttribute)).LoadFormAssemblyFile?

It depends on what the goal is. If the usage of the related assemblies is to "attach" some code to an existing assembly, then I think it would make sense to load the related assemblies into the same load context as the "annotated" assembly.
So something like AssemblyLoadContext.GetLoadContext(assembly).LoadFromAssemblyPath (assembly is where we look for the attributes).

Note though that this may be a breaking change. Assembly.LoadFile has the specific behavior that it will load into a new load context for every file. Side effect of this is that it pretty much always succeeds (there will never be any versioning problems) - so if the related assemblies contain two assemblies of the same name but from different place and thus potentially different version, LoadFile will not fail on them. Loading into a manually picked ALC might fail (depends if there's any load context isolation in play or not).
That said LoadFile typically leads to more subtle versioning problems - even though the assembly specified is loaded in isolation and thus always works, its dependencies are NOT isolated and will fallback to the Default ALC. So for example if I have two assemblies which both use Newtonsoft.Json but one version 11 and the other version 12 and I load those two assemblies via LoadFile, it will likely lead to some kind of problem - either loading of references for one of them will fail (if 11 gets loaded before 12 is asked for), or I might get weird exceptions later on (if 12 is used for reference to 11 and there are behavioral changes in 12).

All in all - I think it would be great to also look in detail into what assembly loading behavior is desirable in this code. That said it's not really related or necessary for single-file use cases (those will pretty much work as before).

It likely has no meaningful observable difference (other than avoiding a new context) as these assemblies have unique names and share references with the app

To expand on @davidfowl's comment, these assemblies contain the result of compiling views that the application cannot reference. They're loaded so that MVC can discover and execute them. They also have the exact set of dependencies as the application so there's little risk of version conflict.

In that case using AssemblyLoadContext.GetLoadContext(assembly).LoadFromAssemblyPath(path) is probably the best solution. It should work even if the code is using multiple ALCs (right now it probably never does), so good for the future and it saves you the additional ALCs. It also sort of guards against potential "misuse" which you don't want to support anyway. Having the related assemblies share the same ALC as the "main" assembly will also have the nice side-effect that they will share the dependency resolution with the "main" assembly. So if in the future you add some logic to how dependencies are resolved and so on, the related assemblies would just keep on working.

davidfowl · 2020-08-15T04:29:52Z

@DamianEdwards how do you feel about this.

cc @glennc @LadyNaggaga

vitek-karas · 2020-08-16T19:25:48Z

src/Mvc/Mvc.Core/src/ApplicationParts/RelatedAssemblyAttribute.cs

+            var assemblyLocation = assembly.Location ?? AppContext.BaseDirectory;
            var assemblyDirectory = Path.GetDirectoryName(assemblyLocation);


This is problematic. For normal file-based assembly C:\temp\MyApp.dll the assemblyDirectory will end up C:\temp. But for single-file in C:\temp\MyApp.exe the assemblyDirectory will end up as C:\ - very likely wrong.

vitek-karas · 2020-08-16T19:28:11Z

src/Mvc/Mvc.Core/src/ApplicationParts/RelatedAssemblyAttribute.cs

+                }
+                catch (IOException)
+                {
+                    // The assembly isn't already loaded. Patience, we'll attempt to load it from disk next.


Is this perf critical in some sense? If not, I think a cleaner way would be to skip this check and simply try to load the assembly. If it's already loaded, runtime will relatively quickly return the loaded assembly from cache.
The only problem might be for assemblies which are in the single-file bundle, but could also appear on disk (possibly in a different version) - is that something we need to handle?

vitek-karas · 2020-08-16T20:23:34Z

src/Mvc/Mvc.Core/src/ApplicationParts/RelatedAssemblyAttribute.cs

@@ -106,7 +123,7 @@ public static IReadOnlyList<Assembly> GetRelatedAssemblies(Assembly assembly, bo
                    }
                }

-                var relatedAssembly = loadFile(relatedAssemblyLocation);
+                relatedAssembly = loadFile(relatedAssemblyLocation);


It depends on what the goal is. If the usage of the related assemblies is to "attach" some code to an existing assembly, then I think it would make sense to load the related assemblies into the same load context as the "annotated" assembly.
So something like AssemblyLoadContext.GetLoadContext(assembly).LoadFromAssemblyPath (assembly is where we look for the attributes).

Note though that this may be a breaking change. Assembly.LoadFile has the specific behavior that it will load into a new load context for every file. Side effect of this is that it pretty much always succeeds (there will never be any versioning problems) - so if the related assemblies contain two assemblies of the same name but from different place and thus potentially different version, LoadFile will not fail on them. Loading into a manually picked ALC might fail (depends if there's any load context isolation in play or not).
That said LoadFile typically leads to more subtle versioning problems - even though the assembly specified is loaded in isolation and thus always works, its dependencies are NOT isolated and will fallback to the Default ALC. So for example if I have two assemblies which both use Newtonsoft.Json but one version 11 and the other version 12 and I load those two assemblies via LoadFile, it will likely lead to some kind of problem - either loading of references for one of them will fail (if 11 gets loaded before 12 is asked for), or I might get weird exceptions later on (if 12 is used for reference to 11 and there are behavioral changes in 12).

All in all - I think it would be great to also look in detail into what assembly loading behavior is desirable in this code. That said it's not really related or necessary for single-file use cases (those will pretty much work as before).

davidfowl · 2020-08-16T20:49:36Z

@vitek-karas the reason we need to do this is because we generate an assembly post build and put the dll in the output folder. That dll isn't in the deps file today and we find it by looking next to the main dll. If there was a way to load from the bundle, then it would be possible to load include the files.

@pranavkm did you look at making this an embedded resource? Do we I'm too late in the pipeline to do that?

vitek-karas · 2020-08-16T21:05:46Z

Currently the only way to put it into the bundle would be to include it in the deps file.. so I guess that's a no. Embedded resource would definitely work (depending on how complex would that be to make work within the build system though).

davidfowl · 2020-08-16T21:59:18Z

I tried writing a template test for this, but we don't build rid specific M.A.A before the template tests are available. I'll add a CTI item for this.

That's problematic. We need to automate this outside of CTI.

vitek-karas · 2020-08-18T15:44:32Z

src/Mvc/Mvc.Core/src/ApplicationParts/RelatedAssemblyAttribute.cs

+        private static readonly Func<string, Assembly> LoadFromAssemblyPathDelegate =
+            AssemblyLoadContext.GetLoadContext(typeof(RelatedAssemblyAttribute).Assembly).LoadFromAssemblyPath;


I would not do this - RelatedAssemblyAttribute is from Mvc.Core which is part of the framework, so pretty much always loaded into the default ALC. So this is the same as doing AssemblyLoadContext.Default.LoadFromAssemblyPath.
If in the future something loads the "main" assembly (the one with the related assemblies on it) into a secondary ALC this will likely be wrong and things might break. I think it should be AssemblyLoadContext.GetLoadContext(assembly).LoadFromAssemblyPath - where assembly is the "main" assembly.

pranavkm

This is updated. I had a chat with @vitek-karas and this seems like the most reasonable solution given how some views appear inside the bundle and others do not. I was also able to figure out a way to author a test. Hopefully it works on the CI 🤞

vitek-karas

Looks good - the parts I know about - so load contexts and single-file. Testing and such - somebody else should take a look as well.

javiercn

LGTM!

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 14, 2020

pranavkm requested review from davidfowl and javiercn August 14, 2020 23:38

davidfowl reviewed Aug 14, 2020

View reviewed changes

vitek-karas reviewed Aug 16, 2020

View reviewed changes

pranavkm force-pushed the prkrishn/related-assembly-part branch from 243b77d to 05a421b Compare August 18, 2020 00:04

vitek-karas reviewed Aug 18, 2020

View reviewed changes

Add support for views + SingleFileExe

2f0cf8d

pranavkm force-pushed the prkrishn/related-assembly-part branch from 05a421b to f729895 Compare August 19, 2020 00:52

pranavkm requested review from halter73, jkotalik, SteveSandersonMS, Tratcher and a team as code owners August 19, 2020 00:52

pranavkm requested a review from a team August 19, 2020 00:52

pranavkm changed the base branch from master to release/5.0 August 19, 2020 00:53

Changes per discussion. Add a test

ad8a927

pranavkm force-pushed the prkrishn/related-assembly-part branch from f729895 to ad8a927 Compare August 19, 2020 00:57

pranavkm commented Aug 19, 2020

View reviewed changes

pranavkm removed request for a team, halter73, SteveSandersonMS, Tratcher and jkotalik August 19, 2020 00:59

vitek-karas approved these changes Aug 19, 2020

View reviewed changes

Fixup test

57db593

javiercn approved these changes Aug 19, 2020

View reviewed changes

davidfowl approved these changes Aug 19, 2020

View reviewed changes

pranavkm mentioned this pull request Aug 20, 2020

Investigate why SingleFileExe tests fail in Helix #25103

Closed

Skip test for now

6a9241b

pranavkm force-pushed the prkrishn/related-assembly-part branch from 4d5e938 to 6a9241b Compare August 20, 2020 21:59

pranavkm merged commit 3e8c5c4 into release/5.0 Aug 21, 2020

pranavkm deleted the prkrishn/related-assembly-part branch August 21, 2020 02:47

		var assemblyLocation = assembly.Location ?? AppContext.BaseDirectory;
		var assemblyDirectory = Path.GetDirectoryName(assemblyLocation);

		private static readonly Func<string, Assembly> LoadFromAssemblyPathDelegate =
		AssemblyLoadContext.GetLoadContext(typeof(RelatedAssemblyAttribute).Assembly).LoadFromAssemblyPath;

Add support for views + SingleFileExe #24925

Add support for views + SingleFileExe #24925

Uh oh!

Conversation

pranavkm commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Aug 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Aug 16, 2020

Uh oh!

vitek-karas commented Aug 16, 2020

Uh oh!

davidfowl commented Aug 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pranavkm commented Aug 14, 2020 •

edited

Loading