Remove dependence on AppDomain from tests #6937
Conversation
ReubenBond
force-pushed
the
net5/appdomain-replacement
branch
from
74c81ac
to
4f3ff9c
Compare
ReubenBond
force-pushed
the
net5/appdomain-replacement
branch
from
4f3ff9c
to
94d71bc
Compare
ReubenBond
force-pushed
the
net5/appdomain-replacement
branch
17 times, most recently
from
312fd0e
to
91ac77a
Compare
| await Task.Delay(5000); | ||
| if (!Process.GetProcesses().Any(p => p.Id == monitorProcessId)) | ||
| { | ||
| Console.Error.WriteLine($"Parent process {monitorProcessId} has exited"); |
There was a problem hiding this comment.
I suppose it's fine since the parent process died already, but just double-checking whether you want to attempt a graceful shutdown? Maybe for some users that's helpful even though not ideal to rely on it?
There was a problem hiding this comment.
I considered it, but I think that fast killing is more appropriate here. We want to avoid zombies and we don't want to annoy developers when they're rebuilding their app but some old process is locking their files
ReubenBond
force-pushed
the
net5/appdomain-replacement
branch
from
91ac77a
to
2f9268a
Compare
| configurationBuilder.Add(source); | ||
| } | ||
| var configuration = configurationBuilder.Build(); | ||
| var handle = await this.CreateSiloAsync(siloSpecificOptions.SiloName, configuration); |
There was a problem hiding this comment.
I remember that I had to use a list of ConfigurationSource because of serialization issues if I passed IConfiguration directly... didn't you have the same issue? Perhaps now it's fine, just double checking
There was a problem hiding this comment.
I don't recall the details though... perhaps it's because IConfiguration could be literally anything that fulfills the interface (which might not serialize fine across app domains, or could have unexpected behavior for some users expecting to control that configuration from within the test after it's been sent over the wire). Where as concrete implementations of ConfigurationSource generally serialize fine, and are less prone to misinterpretation once they are sent over the wire.
There was a problem hiding this comment.
I recall something along those lines. There may have been a later change to the IConfiguration library to add the AsEnumerable() method that we use here to make sure that this is serializable: we end up serializing KeyValuePair<string, string>[], which will not cause an issue.
Users shouldn't expect to be able to control their config after it's been set, especially not in a test cluster. I don't think the old version of this handled that well, anyway. It could only work for file sources, but IIRC they weren't serializable (and they aren't designed to be - we shouldn't be trying to serialize such types)
There was a problem hiding this comment.
ah, perfect then, certainly the AsEnumerable() method didn't exist before, we were asking for something like that in the past, so I guess they finally added it. And I didn't see the usage here as it was merged before I could review :)
Good job, I'm glad using processes is a thing now, and looks like the abstraction pretty much held up :)
EDIT: Note that this is based on #6971, so that needs to go in first, before we can squash & rebase this on top.
AppDomains cannot be used on .NET Core or above, so we need another mechanism. This PR implements a silo handle which launches a standalone process, so that we can test versioning, etc, on .NET Core and above - just as @jdom intended when redesigning the TestCluster for 2.0.
It uses stdin and stdout to control the test processes, and the test processes need to include a
Program.csfile which calls intoStandaloneSiloHost.Main- the defaultMain(...)which is added to test projects during build is a no-op, so this is not a problem.It also requires that an executable assembly is produced, so there are some csproj changes necessary.