Nunit-console: Test runner does not close fired up proceseses.

[TimonPost]

When I fire up a test runner, it starts two processes:

After the runner has performed the test round it should close the processes. I try to accomplish this by setting one of the following configurations:

static void Main(string[] args)
        {
            // Get an interface to the engine
            using (ITestEngine engine = TestEngineActivator.CreateInstance())
            {
                var settings = engine.Services.GetService<ISettings>();

                settings.SaveSetting("DisposeRunners", true);
                settings.SaveSetting("StopOnError", true);
                settings.SaveSetting("DebugAgent", false);
                settings.SaveSetting("DebugTests", false);

                // Create a simple test package - one assembly, no special settings
                TestPackage package = new TestPackage(@"E:\programming\FaultifyNew\Faultify\Benchmark\Faultify.Benchmark.NUnit\bin\Debug\netcoreapp3.1\test-duplication-0\Faultify.Benchmark.NUnit.dll");
                package.AddSetting("DisposeRunners", true);
                package.AddSetting("StopOnError", true);
                package.AddSetting("DebugAgent", false);
                package.AddSetting("DebugTests", false);

                // Get a runner for the test package
                using (ITestRunner runner = engine.GetRunner(package))
                {
                    // Run all the tests in the assembly
                    XmlNode testResult = runner.Run(null, TestFilter.Empty);

                    runner.StopRun(true);
                    runner.Unload();
                }
            };

            Console.ReadLine();
        }

None of them seem to work and have no effect. How can I make sure the processes fired-up by the runner are closed? They are locking my files which prevents me to use them.'

Info
"NUnit.Engine" Version="3.12.0"
netcoreapp3.1

Files

NUnitRunner.zip

[CharliePoole]

The ISettings service is provided by the engine for use by runners, wanting to save their settings. The engine does not use any settings you add to the service. Those setting are not used by the engine, whose behavior is entirely under the control of the TestPackage and its settings. However, that didn't cause your problem, since you duplicated the same settings in the package.

In fact, I would normally perform my initial tests of a runner without any setting added to the TestPackage. You should be able to run tests in a simple test assembly using default settings before proceeding further.

Your linked project file includes package references to the NUnit Test Adapter and the NUnit V2 framework driver extension. The former is not needed and may bring in other references you don't want. The latter is only used for V2 tests, which can only run under .NET Framework. In any case, extensions are not used by adding references to them in your project. Remove both.

Your runner has a project reference to your tests. Remove that as well. Tests are found by their path in the TestPackage.

You should also ensure that you are using a simple, test assembly, so we can rule out the usual cause of locking: that the tests themselves are holding some resource and not releasing it.

If the above doesn't resolve your problem, then we can dig deeper.

[TimonPost]

Thanks for your answer. I debugged some hours today and tried everything, among the things you mentioned. I appreciate any further insight or help.

New Changes

• Removed those unused deps.
• Removed all custom configurations.
• Remove test dependency such that NUnit references this test from its own /bin.
• Simplified tests

Test File

• My process consumes two times a reference to the test file.

• When I run a manual garbage collector one of those references is cleaned.

question: Why has it loaded the test file two times in a row while I only use one TestPacket?

New Source Code

• To run it Change BASE_TEST_DIR to match your path.

NUnitRunner (2).zip

static void Main(string[] args)
        {
            RunEngine();

            GC.Collect();
            GC.WaitForPendingFinalizers();

            for (int i = 0; i < 10; i++)
            {
                Thread.Sleep(1000);
            }
        }

        static void RunEngine()
        {
            // Get an interface to the engine
            using (TestEngine engine = CreateTestEngine())
            {
                // Create a simple test package - one assembly, no special settings
                TestPackage package = new TestPackage(Path.Combine(BASE_TEST_DIR, "Tests.dll"));

                // Get a runner for the test package
                using (var runner = engine.GetRunner(package))
                {
                    // Run all the tests in the assembly
                    XmlNode testResult = runner.Run(null, TestFilter.Empty);
                    runner.StopRun(true);
                    runner.Unload();
                    runner.Dispose();
                }

                engine.Dispose();
                engine.Services.ServiceManager.StopServices();
                engine.Services.ServiceManager.Dispose();
            }
        }

        static TestEngine CreateTestEngine()
        {
            var engine = new TestEngine();
            engine.InitializeServices();
            engine.WorkDirectory = BASE_TEST_DIR;
            return engine;
        }

_I know calling dispose() while using 'using' is a bit weird, just like to be very very sure_

[TimonPost]

I found the source of the issue, Mono.Cecil.AsseblyDefinition is not disposed of, this locks the assembly until the caller application closes.

• https://github.com/nunit/nunit-console/blob/master/src/NUnitEngine/nunit.engine.core/Services/DriverService.cs#L76
• https://github.com/nunit/nunit-console/blob/master/src/NUnitEngine/nunit.engine/Services/RuntimeFrameworkService.cs#L233
• https://github.com/nunit/nunit-console/blob/master/src/NUnitEngine/nunit.engine.core/Drivers/NUnitNetStandardDriver.cs#L84
• https://github.com/nunit/nunit-console/blob/master/src/NUnitEngine/nunit.engine.core/Extensibility/ExtensionAssembly.cs#L37

The second lock is caused by: https://github.com/nunit/nunit-console/blob/master/src/NUnitEngine/nunit.engine.core/Drivers/NUnitNetCore31Driver.cs#L84. Somehow we should have to release the resource it contains.

In my case it was DriverService and NUnitNetCore31Driver causing the problems.

[CharliePoole]

I'll take a closer look, although I find this surprising. Did you modify the nunit code to fix it or did you use a workaround in your runner?

We use AssemblyDefiniition the way we do because it's understood to merely read, but not load the assembly. I'll try to verify what you are saying and we can take it from there.

[TimonPost]

I can confirm that when disposing of AssemblyDefinition the resource is released. It is possible to load AssemblyDefinition from a stream if that might be needed. One addition, when I ran a manual garbage collector, the AssemblyDefinitions did release the assembly file. Then the left problem is only CustomAssemblyLoadContext. However, I think it's clean to dispose of the AssemblyDeffinitinon directly after usage.

For NUnitNetCore31Driver we need to call Unload() on CustomAssemblyLoadContext, however, I am experiencing some complications on that part.

This is an interesting resource regarding the unload functionality:
https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability

The following fields have to be disposed of in order for CustomAssemblyLoadContext.Unload() to be working.

        Assembly _testAssembly;
        Assembly _frameworkAssembly;
        object _frameworkController;
        Type _frameworkControllerType;

Those fields are stored locally as class fields hence we can not dispose of them immediately after use in the same function as Microsoft proposes.

[MethodImpl(MethodImplOptions.NoInlining)]
static int ExecuteAndUnload(string assemblyPath, out WeakReference alcWeakRef)
{
    var alc = new TestAssemblyLoadContext();
    Assembly a = alc.LoadFromAssemblyPath(assemblyPath);

    alcWeakRef = new WeakReference(alc, trackResurrection: true);

    var args = new object[1] {new string[] {"Hello"}};
    int result = (int) a.EntryPoint.Invoke(null, args);

    alc.Unload();

    return result;
}

I was thinking about implementing IDispose for this class and execute the following logic:

 // make sure they are null
 _frameworkAssembly = null;
 _testAssembly = null;
 _frameworkController = null;
 _frameworkControllerType = nul

// make sure they are cleaned up
 GC.Collect();
 GC.WaitForPendingFinalizers();

 // now call unload when there are no references anymore
 _assemblyLoadContext.Unload();

// wait till all resources are released
 GC.Collect();
 GC.WaitForPendingFinalizers();

[CharliePoole]

@TimonPost Yes, the Microsoft reference describes perfectly the problem that use of the metadata assembly (derived from Mono.Cecil) is intended to avoid. However, it's not quite the problem we have, as AssemblyDefinition doesn't load the assembly.

What happens is that the assembly is read into memory as a file stream. No loadcontext is involved. Logically, it seems to me that AssemblyDefinition should create it's tables and then close the stream. However, it doesn't do that.

My guess is that the stream needed to be kept open in Mono.Cecil because it supports modifying the assembly. I removed all that capability, so maybe this can be changed now, but it will take testing. I think closing the AssemblyDefinition once you're done with it is probably the right thing to do in the interim. If I make changes, I'll leave an empty Dispose to avoid breaking anyone's code.

[TimonPost]

Nice, I have created the pull request. I am not sure what the policies are and or whether you like to take this over. How would you like to go further with this?

Also, it is possible to modify an assembly without it directly being attached to a file stream (a.k.a editing it in memory). At least in the newer version of Mono.Cecil I had done it like this:

var bytes = File.ReadAllBytes("path");
var assembly = AssemblyDefinition.ReadAssembly(new MemoryStream(bytes));

// modify

assembly.Write(path);

[CharliePoole]

That would be OK in Mono.Cecil, because it's designed for modifying an assembly. However, it seems slightly unsafe in the unusual case case where two processes might be trying to modify the same assembly. However, since the metadata assembly we use is expressly designed for read-only use, I think we can just tell people "don't do that!" :wink:

Our policy is that two of us have to review your PR and approve it for merging. I can do one of them and @ChrisMaddock should probably do the other since he wrote the .NET Core framework driver.

[ChrisMaddock]

(Reopening as #895 is only a partial solution)

Thanks for digging into this Timon - really appreciate the debugging and research you've done on this!

Your diagnosis sounds spot on to me. Architecturally it becomes a bit more complex in that we don't have any Disposable IFrameworkDrivers yet, but I agree that I think it's necessary to support .NET Core properly.

The question is, if IFrameworkDrivers become IDisposable, who's responsible for disposing them? I _think_ it makes sense for it to be the ITestRunner, and ITestRunner's are already disposable, which gives us a clear point to do it. @CharliePoole - would you agree?

Timon - are you happy to take this forward? I think the best way would be to first make the IFrameworkDriver interface disposable, and implement assembly context unloading you've described. We'd then need to ensure all the different ITestRunner's are disposing of any FrameworkDriver's they create, when they're disposed themselves.

[ChrisMaddock]

Hmm - although I guess doing that change would become a breaking change for any extension IFrameworkDriver's, right? That's not great.

Maybe our short term solution is to, rather than make IFrameworkDriver disposiable, instead test if each framework driver instance implements IDisposable and dispose accordingly. Then from v4, we could make IFrameworkDriver IDisposable.

[TimonPost]

Thanks for the PR approval, I had a look at this particular issue however I was not sure how to fix it without making a lot of changes.

Let's define the issue so that it is clear what the problem exactly is:

Currently, an assembly is loaded by this code.

var assemblyLoadContext = new CustomAssemblyLoadContext(assemblyPath);
_testAssembly = assemblyLoadContext.LoadFromAssemblyPath(assemblyPath);

Next, the following private fields are initialized:

Assembly _testAssembly;
Assembly _frameworkAssembly;
object _frameworkController;
Type _frameworkControllerType;

Those have to be released in order for CustomAssemblyLoadContext.Unload() to be able to release its resources.

_microsoft article:_

So the question becomes:

• How can we make sure that those fields are released at the end of the driver usage.

[TimonPost]

I guess doing that change would become a breaking change for any extension IFrameworkDriver's, right?

If we implement IDisposable at IFrameworkDriver then yea, the inheriting class must define the IDisposable, and users having their own IFrameworkDriver implementation will have to update their code. I would add that this issue is only occurring in NUnitNetCore31Driver.

Solutions I thought of:

• Perform some logic in the Deconstructor but I am not sure if that is desired behavior.
• Perform the logic in IDisposable, requires breaking changes.
• Do not store references as local fields but manage the assembly as Microsoft proposes with a weak reference. Requires no breaking changes, but might add overhead.

[CharliePoole]

In practical terms, it's not clear that anyone but us has ever implemented an IFrameworkDriver. Our Core 3.1 framework driver is, of course, under our control.

[ChrisMaddock]

In practical terms, it's not clear that anyone but us has ever implemented an IFrameworkDriver.

This is true, but I still think we should put a reasonable amount of effort to maintaining these publicly defined interfaces where possible. Agreed this should be a consideration if there's not a nice, non-breaking solution.

Solutions I thought of:
Perform some logic in the Deconstructor but I am not sure if that is desired behavior.
Perform the logic in IDisposable, requires breaking changes.
Do not store references as local fields but manage the assembly as Microsoft proposes with a weak reference. Requires no breaking changes, but might add overhead.

The weak references sounds like the cleanest solution, if we can make it work! I'm not too keen on the deconstructor if we can avoid it - deconstructors always feels like a bit of a workaround.

Option 4: we could make NetCore31Driver IDisposable, but _not_ the IFrameworkDriver interface. When we want to dispose them, we would then need to typecheck if the driver implements IDisposable (i.e. _driver is IDisposable) and dispose - which isn't so nice in terms of type-safety, but could tie us through to NUnit Engine v4, and we could then make the breaking change then.

The weak reference solution seems like the option most in keeping with how AssemblyLoadContext was intended to be managed though, if we can make it work. Would you be interested in taking a look at it, Timon?

[ChrisMaddock]

Side note: I think we'll also need to enable isCollectable on CustomAssemblyLoadContext.

Docs: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext.-ctor?view=net-5.0#System_Runtime_Loader_AssemblyLoadContext__ctor_System_Boolean_

[TimonPost]

Yea that is correct! Also see: https://stackoverflow.com/questions/123391/how-to-unload-an-assembly-from-the-primary-appdomain#:~:text=You%20can%20not%20unload%20an,the%20life%20of%20the%20appdomain.

Currently, I am trying to get XUnit working for my app but also there I have issues with loading/unloading assemblies. There are areas of overlap. Perhaps after I am finished with that I have a better perspective on how to resolve this issue. Upto now I was a bit unfamiliar with how assemblies were loaded into an Appdomain.

[TimonPost]

Closing this issue in favor of https://github.com/nunit/nunit-console/issues/924 since this issue brought up two problems where one is fixed in #895.