Is ExecuteInTempDomain really necessary?

[meareindel]

Dear Oleg,

I've tried to use WiX# in .NET Core runtime and it failed with something like "type 'System.AppDomainSetup' could not be loaded from assembly 'mscorlib...'". I made some investigation and have found that for some reason it runs some code in separate AppDomain, clone of current. Particularly two methods do that, one:

wixsharp/Source/src/WixSharp/ProjectValidator.cs

Lines 183 to 190 in 51ec873

	public static void ValidateCAAssembly(string file)
	{
	//need to do it in a separate domain as we do not want to lock the assembly
	Utils.ExecuteInTempDomain<AsmReflector>(asmReflector =>
	{
	asmReflector.ValidateCAAssembly(file, typeof(CustomActionAttribute).Assembly.Location);
	});
	}

and two:

wixsharp/Source/src/WixSharp/Utils.cs

Lines 120 to 127 in 51ec873

	internal static string OriginalAssemblyFile(string file)
	{
	//need to do it in a separate domain as we do not want to lock the assembly
	return (string)ExecuteInTempDomain<AsmReflector>(asm =>
	{
	return asm.OriginalAssemblyFile(file);
	});
	}

Both contains same comment mentioning that all of it was made to avoid some assembly locks. But as far as I understand the logic provided by both methods and under-the-hood mechanics of AppDomain, there is no need to run that very code in separate domain.

Consider internals of method two:

wixsharp/Source/src/WixSharp/ProjectValidator.cs

Lines 211 to 234 in 51ec873

	public string OriginalAssemblyFile(string file)
	{
	string dir = IO.Path.GetDirectoryName(IO.Path.GetFullPath(file));
	System.Reflection.Assembly asm = AppDomain.CurrentDomain.GetAssemblies().FirstOrDefault(a =>
	{
	try
	{
	return a.Location.SamePathAs(file); //some domain assemblies may throw when accessing .Locatioon
	}
	catch
	{
	return false;
	}
	});
	if (asm == null)
	asm = System.Reflection.Assembly.ReflectionOnlyLoadFrom(file);
	// for example 'setup.cs.dll' vs 'setup.cs.compiled'
	var name = asm.ManifestModule.ScopeName;
	return IO.Path.Combine(dir, name);
	}

It contains current domain assemblies enumeration first and loads assembly into the domain only if there is no currently loaded assemblies with matching Location property. So it is already lock-free if I didn't miss something. The only thing that could be additionally implemented here is to unload assembly that has just been loaded to take it's scope name.

Internals of method one are indeed assembly-locking implemented but it could be easily rewritten to use the same lock-free implementation of above. The only thing that I couldn't understand properly is assembly resolvers added here and there - for the first look they do something strange such as loading the very same assembly on each resolve event (resolver delegate in ValidateCAAssembly) or the very same things that default .net assembly resolve does (DefaultDomain_AssemblyResolve).

Could you please provide some details around this? Git blame for it points to the moving from sourceforge initial commit so there is no details.

As far as I've tested, that AppDomain-cloning mechanics is the only thing that prevent Wix# to run in .NET Core runtime so it would be nice if it has some simple solution. Anyway it could be done with generating separate assembly with necessary code at runtime and running it in a separate child process and I even would be glad to provide a PR for that, but it obviously will not be as simple :)

At the worst for start it could be simple switch to disable custom action validation and to return input string as original assembly name - that is what I've done to simply test if all the rest would work under .NET Core.

[oleg-shilo]

Hi @meareindel, please give me some time to process your feedback properly.
txs

[oleg-shilo]

OK, I have investigated the problem.

But as far as I understand the logic provided by both methods and under-the-hood mechanics of AppDomain, there is no need to run that very code in separate domain.

Unfortunately it is not true for ValidateCAAssembly. The "ReflectionOnlyLoadFrom" approach cannot be used for browsing assembly attributes and this is what is required for validating the CA. It checks that the methods with CustomActionAttribute attribute are public.

The assembly being validated must be Loaded as calling ReflectionOnlyLoad triggers "System.InvalidOperationException: 'It is illegal to reflect on the custom attributes of a Type loaded via ReflectionOnlyGetType ...". Meaning that temp AppDomain is the only reliable way to avoid locking.

There is also a possibility to load the assembly from bytes, but I decided to provide a mechanism for simply disabling this validation. The truth to be told, this validation is a nice feature to have to assist the user with troubleshooting when he/she makes a mistake when defining CA. But it's not really required for building MSIs.

The next release will have a special setting for cases like yours:

Compiler.AutoGeneration.ValidateCAAssemblies = false;

However internal static string OriginalAssemblyFile(string file) indeed can be replaced with the ReflectionOnly-based implementation (fixed).

...for the first look they do something strange such as loading the very same assembly on each resolve event (resolver delegate in ValidateCAAssembly)

Correct. The solution is deliberate but lacking comments (already addressed).

ValidateCAAssemblyImpl loads the assembly from file for validation. Though for this to happen the AppDomain will need to be able to resolve the only dependence assembly file has - dtfAsm. Thus ValidateCAAssembly always resolves not found dependency assembly to dtfAsm (regardless of args.Name value) when AssemblyResolve is fired.

---

Meaning that from the next release you will be able to host WixSharp in .NET Core app as long as you set Compiler.AutoGeneration.ValidateCAAssemblies to false before building your first msi.

[meareindel]

Thank you for your detailed answer and for next release solution!

But I just wonder:

Meaning that temp AppDomain is the only reliable way to avoid locking.

Why wouldn't one just load assembly in current domain if it is not already loaded? And if it is loaded, simply use the loaded one?

[oleg-shilo]

Unfortunately it's not that simple. There are two aspects of this problem:

Assembly locking
It needs to be prevented to allow CA assembly to be be copied and moved around quite a few times during the build. It is needed for:

• clean msi building with removing the all temp files at the end
• proper assembly packaging by DTF tools, which do not have the ability to do custom asm probing thus the all asms must be gathered in the same place.

Assembly Unloading
Ideally we want the build script AppDomain (AppDomain.Current) to be free from any assemblies that are part of the msi being built. The only exception is the build script assembly.
The CA assemblies can be external (not only the build script). Thus loading them in the current domain can eventually pollute to the point of over saturation. This is quite real scenario as I see users more and more often using a single CLR process/session for building more than a single msi. This in turn creates an opportunity for collision between multiple assemblies being browsed with Reflection for packaging automatic CA methods (e.g. project event handlers).

Why wouldn't one just load assembly in current domain if it is not already loaded?

It would only help with the build script assembly. For external assemblies it will not make any difference.

These are the reasons why I prefer to maintain the existing workflow, which is not particularly elegant but reliable. And for anything else to allow disabling the CA assembly validation completely.

[meareindel]

I see now, thanks. But isn't there a solution for this to check first if assembly is not already loaded and if so then load it, validate and unload? Personally for me it would be enough to simply have the ability to disable CA validation but I just want to make that all clear for myself.

[oleg-shilo]

After some further considerations I have decided to add an extra mode for CA validation.

Compiler.AutoGeneration.ValidateCAAssemblies = CAValidation.InLocalAppDomain;
 . . . 
public enum CAValidation
{
        /// <summary>
        /// The CA assembly is loaded in the temporary remote AppDomain for validation.
        /// Assembly file is unlocked and at the end of the validation the assembly is unloaded.
        /// </summary>
        InRemoteAppDomain,
        /// <summary>
        /// The CA assembly is loaded in the current AppDomain for validation.
        /// Assembly file is unlocked but the assembly will not be unloaded at the end 
       /// of the validation. This mode may lead to the unpredictable behaviour.
        /// </summary>
        InCurrentAppDomain,
        /// <summary>
        /// CA assembly validation is disabled.
        /// </summary>
        Disabled
}

[meareindel]

My apologies. I had to investigate it a little bit further, and after I've done this, it turned out that there is no way to unload assembly from appdomain. There is issue at corefx though but it's still pending and moreover unusable at 3.5 fx anyway.

[oleg-shilo]

... it turned out that there is no way to unload assembly from appdomain...

Oh, that wasn't a problem. I knew very well about that CLR architectural flaw. I was the one who logged the first complain about the problem on MS Connect (when .NET was just released). Ironically MS fully accepted that it is a flaw but closed the issue because... "it was difficult to fix".

Anyway, I was just trying to find the most flexible API that would not affect the existing msi compiler workflow and yet allow any runtime hosting. In your case you would use CAValidation.Disabled or CAValidation.InCurrentAppDomain if it does not create any side effects.

As for, .NET Core hosting, be aware that if you use managed CA compiled with .NET Core compiler, they may not be compatible with MSI .NET hosting runtime. Thus the best chance is to have the CA assemblies compiled against "Standard 2".

[meareindel]

I got it, thanks!

And I think I've hurried to close issue and it should be reopen because the related enhancement has not released yed.