AppDomain configuration is serialized without using BinFmt

[MichalPavlik]

Fixes #8922

Context

MSBuild uses binary formatter for AppDomainSetup serialization. We can transfer only configuration bytes.

Changes Made

Byte array representing the configuration is used instead of serialization of the AppDomainSetup type.

Testing

New unit tests are passing.

Notes

The change is behind a 17.10 changewave and IsBinaryFormatterSerializationAllowed trait.

[MichalPavlik]

I guess this change should be reversable by env variable/change wave...

[JanKrivanek]

Looks good.

Do we have any existing tests executing this code path?

[JanKrivanek]

I guess this change should be reversable by env variable/change wave...

Let's put it behind 17_10 ChangeWave introduced by #9318

[ladipro]

The documentation says that GetConfigurationBytes() returns null if SetConfigurationBytes() has not been called. So it doesn't look like it's a replacement for serialization, unfortunately.

https://dotnetfiddle.net/Zi96aB

[JanKrivanek]

The documentation says that GetConfigurationBytes() returns null if SetConfigurationBytes() has not been called. So it doesn't look like it's a replacement for serialization, unfortunately.

https://dotnetfiddle.net/Zi96aB

@MichalPavlik - can you look into whether there can be any viable serialization alternative? Alternatively what are all the codepaths that leverage the AppDomainSetup and what feature(s) would be affected (and how) by the lack of transfering of this

[JanKrivanek]

The documentation says that GetConfigurationBytes() returns null if SetConfigurationBytes() has not been called. So it doesn't look like it's a replacement for serialization, unfortunately.

https://dotnetfiddle.net/Zi96aB

@ladipro - I'll need to look once again once I have some free cycles (or @MichalPavlik can look in the meantime) - but IIRC from my past (though a very quick and possibly errorneous) investigation - the only place where transfered AppDomainSetup was actually used is this:

msbuild/src/Shared/TaskLoader.cs

Lines 91 to 99 in c36a54e

	// Make sure we copy the appdomain configuration and send it to the appdomain we create so that if the creator of the current appdomain
	// has done the binding redirection in code, that we will get those settings as well.
	AppDomainSetup appDomainInfo = new AppDomainSetup();
	// Get the current app domain setup settings
	byte[] currentAppdomainBytes = appDomainSetup.GetConfigurationBytes();
	// Apply the appdomain settings to the new appdomain before creating it
	appDomainInfo.SetConfigurationBytes(currentAppdomainBytes);

where it uses just the Get/Set ConmfigurationBytes. If that's really the only usage - then serializing the config bytes upfront (whether they have been set or not) should be OK, correct? The other question is if that preexisting code is correct - but we have no complains about the functionality currently.

[ladipro]

@JanKrivanek I believe that the ultimate source of appDomainSetup in the code you linked is here:

msbuild/src/Build/BackEnd/BuildManager/BuildManager.cs

Line 2817 in c36a54e

, AppDomain.CurrentDomain.SetupInformation

The Get/SetConfigurationBytes calls are done for completeness but we definitely rely on the rest as well.

To hit the relevant code path, build the following project with MSBUILDNOINPROCNODE=1:

<Project>
  <Target Name='Build'>
    <RegisterAssembly Assemblies="nonexistent.dll" />
  </Target>
</Project>

With the changes in the PR is fails with

error MSB4061: The "RegisterAssembly" task could not be instantiated from "Microsoft.Build.Tasks.Core, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a". Object reference not set to an instance of an object.

[ladipro]

Needless to say, we should add the missing test coverage as part of this work.

[JanKrivanek]

100% agree on the need to have the associated codepaths covered by the tests - @MichalPavlik can you please ensure those are added? And then compare results prior and after change?

I'd like to see what code is consuming the remoted AppDomainSetup (to confirm there is any beyond the TaskLoader getting the configuration bytes) and ideally what specifically is it accessing. Then we should think about how to extract and transfer such additional state.

[ladipro]

The Get/SetConfigurationBytes calls are done for completeness but we definitely rely on the rest as well.

I take this back, I couldn't find anything else consuming the AppDomainSetup as a whole. The build is failing because we don't always create the AppDomainSetup on the receiving side - the bug you just pointed out.

What do you think about refactoring the code a bit so that only the byte[] is passed around? This would help both readability (it's clearer what's actually used) and a tiny bit of perf (no need to create an instance of AppDomainSetup when the only thing it carries are the configuration bytes).

[JanKrivanek]

What do you think about refactoring the code a bit so that only the byte[] is passed around? This would help both readability (it's clearer what's actually used) and a tiny bit of perf (no need to create an instance of AppDomainSetup when the only thing it carries are the configuration bytes).

Absolutely!

[JanKrivanek]

Looks great now! Plus we have assurance by the tests - perfect!
I'd want to see the changewave documented in userfacing .md, plus quick offline confirmation we want to preserve the AppDomainSetup outside the TaskLoader. Then I'll sign-off

[JanKrivanek]

Thank you for all the effort! ;-)