nunit / nunit-console

NUnit Console runner and test engine
MIT License
212 stars 149 forks source link

Eliminating locking of assembly file while test is loaded #1425

Open PatrickLeGarrec opened 1 week ago

PatrickLeGarrec commented 1 week ago

Hi :)

CopyFileWithoutLock(sourceFilePath, destinationFilePath);   // my FileStream implementation

ITestEngine engine = TestEngineActivator.CreateInstance();
TestPackage package = new TestPackage(destinationFilePath);
ITestRunner runner = engine.GetRunner(package);
XmlNode explore = runner.Explore(TestFilter.Empty);

The file copy is ok and the exploration works.

then i redo a copy from the same file to the same destination

CopyFileWithoutLock(sourceFilePath, destinationFilePath);

==> Failed ==> " is being used by another process " The file is locked.

Then i redo the operation without exploring, with x2 successfull copy operations without any lock.

CopyFileWithoutLock(sourceFilePath, destinationFilePath);

ITestEngine engine = TestEngineActivator.CreateInstance();
TestPackage package = new TestPackage(destinationFilePath);
ITestRunner runner = engine.GetRunner(package);

CopyFileWithoutLock(sourceFilePath, destinationFilePath);

The file copy is ok ==> no lock.

It seems that the engine.GetRunner cause lock, even if i dispose() both engine and runner. or runner.Explore.

Any clue ?

CharliePoole commented 1 week ago

Most of the methods defined in ITestRunner , including Explore, force a call to ItestRunner.Load, which (obviously) loads the test assemblies specified in your TestPackage. This has always implied a guarantee to the caller that the loaded test assemblies will remain loaded until they are unloaded or until the process exits.

This behavior is required by any runner, e.g. a gui, which displays a representation of the tests and allows the user to select individual tests for running or view information about such tests. So this has to be "by design" or "won't fix".

You could try actually unloading the explored tests, which may work or not depending on what else your application is doing with them. As an example of the problems which may arise, test cases randomly generated by nunit will not be the same across an unload.

CharliePoole commented 1 week ago

If you encounter a bug using Unload, we can reopen this.

PatrickLeGarrec commented 1 week ago

You describe perfectly what i needed.

Unload does not work too but it is normal. I'm trying to do something that's not possible that way.

I create a small demo project to check the capabilities of the engine.

I was thinking about your test-centric GUI. You load Nunit dll and discover the tests as usual, and run them. I do the same it works nice.

but you need to explore the unknown .dll just loaded, and the user may load another dll, etc... I mean you don't have any lock rather than i have :)

Thanks anyway

PatrickLeGarrec commented 1 week ago

After runner.Explore, even with a safe unload() under the same process, the file is locked. We tried to load a different dll just after and unload() => same behavior. the old file is still locked, and the new one too. Each dll you load with runner.explore is definitly locked. We have checked our design and nobody understand what's wrong. It seems unload() does nothing.

CharliePoole commented 1 week ago

Can you share a minimal repro? If so, I'll take another look. While the tests must be kept loaded, that does not imply that the file from which they were loaded should be locked. In the V2 and TC GUIs, for example, that file is not locked but is watched to see if it changes. Since V3, NUnit itself has not had a GUI, so some code that is not exercised regularly can end up being removed.

PatrickLeGarrec commented 1 week ago

Here is : https://github.com/PatrickLeGarrec/DemoNUnitEngine Thanks in advance for your help.

PatrickLeGarrec commented 1 week ago

Alternative: We copied and renamed the test.dll and explore the renamed file. it works and no lock persist anymore because there is no dependency on this new file. But the explored tests are these that have been written in the original test.dll with the dependency on.

The dll must be a renamed copy of the dependency (detail of the renamed file is still test.dll) to work properly. We understood that nUnit engine did not need to do that. We were wrong. that said, even if you explore another renamed dll with others tests, they are ignored and only the original tests in the dependant test.dll are consistent. So, a renamed dll have no interest at all.

In case of unknown nUnit compliant dll, you are not able to use the engine if your don't have a dependency on the binary of that dll. we understand that.

In that case, that create a major usage limitation of the engine, and we question ourselves how TC-GUI works. Because it looks to do not require dependency on the loaded dll. It act as a tool.

PatrickLeGarrec commented 1 week ago

i downgrade to Engine 3.13 and NUnitTestAdapter 4.2.0 (and Nunit framework 3.13 instead of 4.1.0) Now the Test.dll is found without dependency as it should be normally. The behavior is now correct. But, the lock is still there...

The 3.17 and Adapter 4.5.0 are not working well according to this https://docs.nunit.org/articles/vs-test-adapter/Adapter-Engine-Compatibility.html

Also despite the document, the adapter 4.5.0 with engine 3.15.4 does not work too. (file not found).

Maybe the begining of an explanation about the strange dependency requirement with 3.17. The last message i wrote is a bad analysis because the downgrade fix the weird stuff and dependency (which should never exist for me).

But it does not explain the lock on the file.

CharliePoole commented 1 week ago

IMO it was a mistake to require tests run under test explorer to reference the appropriate adapter. In the design of NUnit - and of other X-unit style frameorks written in the same era - a test should have no information about how or by which runner it will be executed. Unfortunately, that ship has sailed.

I'll look at your repro after I finish a few other things. As you state, there's no reason the file should be locked.

PatrickLeGarrec commented 1 week ago

No problem Charlie, take your time, it's holiday time here so :) I m still investigating and found some clues. I will try to do not flood the thread. I note i missed some packages with some earlier versions. actually the test.dll is explored well with older libraries, not with the last ones. But yes, the file is lock. and i m investigating...

PatrickLeGarrec commented 1 week ago

I update the repo with a better version. The package combo is old but the exploration now works with the same code. Earliers versions (NUnit / Engine / Adapter) make the Test.dll not found by the engine.

[EDIT] i confirm that only engine 3.14.0 / Adapter 4.3.0 / Nunit 3.14.0 read the dll alone. all earlier versions failed. lock is still there in any version. earlier version read the file only with a dependency on the test.dll. (reflection issue ?)

PatrickLeGarrec commented 6 days ago

Well, i have ran deeper into the engine code and found that the issue would be caused by the Assembly loading. It is known that Assembly.Load and LoadFrom cause lock, and it is better to load in memory with File.ReadAllBytes instead. but it may occurs some memory leak. I have tried to change that but the lock is still there. The parts of code are under NETSTANDARD2_0.

TestEngineActivator.cs : This code is called.

#if NETSTANDARD2_0
        /// <summary>
        /// Create an instance of the test engine.
        /// </summary>
        /// <returns>An <see cref="NUnit.Engine.ITestEngine"/></returns>
        public static ITestEngine CreateInstance()
        {
            var apiLocation = typeof(TestEngineActivator).Assembly.Location;
            var directoryName = Path.GetDirectoryName(apiLocation);
            var enginePath = directoryName == null ? DefaultAssemblyName : Path.Combine(directoryName, DefaultAssemblyName);

            //var assembly = Assembly.LoadFrom(enginePath);
            var assembly = Assembly.Load(File.ReadAllBytes(enginePath));    // <==

            var engineType = assembly.GetType(DefaultTypeName);
            return Activator.CreateInstance(engineType) as ITestEngine;
        }
#else

But the lock is still there.

Also, i see AssemblyLoadContext and CustomAssemblyLoadContext under NETCOREAPP3_1_OR_GREATER According to this https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability , it seems that the context is never unload(). i maybe wrong and need to investigate more.

I m sure that the lock is due to this, because if i neutralize the call to LoadResult from MasterTestRunner.cs Explore

  public XmlNode Explore(TestFilter filter)
  {
   //   LoadResult = PrepareResult(GetEngineRunner().Explore(filter))
   //       .MakeTestRunResult(TestPackage);
      XmlNode node = null;
      return node; 
    //  return LoadResult.Xml;
  }

there is no lock on the assembly.

Clues :

DomainManager.cs :

 public class DomainManager : Service
 {
     static Logger log = InternalTrace.GetLogger(typeof(DomainManager));

     private static readonly PropertyInfo TargetFrameworkNameProperty =
         typeof(AppDomainSetup).GetProperty("TargetFrameworkName", BindingFlags.Public | BindingFlags.Instance);

     /// <summary>
     /// Construct an application domain for running a test package
     /// </summary>
     /// <param name="package">The TestPackage to be run</param>
     public AppDomain CreateDomain( TestPackage package )
     {
         AppDomainSetup setup = CreateAppDomainSetup(package);

    .....

AND DomainUnloader methods are never called.

class DomainUnloader
{
    private readonly AppDomain _domain;
    private Thread _unloadThread;
    private NUnitEngineException _unloadException;

    public DomainUnloader(AppDomain domain)
    {
        _domain = domain;
    }

    public void Unload()
    {
        _unloadThread = new Thread(new ThreadStart(UnloadOnThread));
        _unloadThread.Start();

        var timeout = TimeSpan.FromSeconds(30);

        if (!_unloadThread.Join((int)timeout.TotalMilliseconds))
        {
            var msg = DomainDetailsBuilder.DetailsFor(_domain,
                $"Unable to unload application domain: unload thread timed out after {timeout.TotalSeconds} seconds.");

            log.Error(msg);
            Kill(_unloadThread);

            throw new NUnitEngineUnloadException(msg);
        }

        if (_unloadException != null)
            throw new NUnitEngineUnloadException("Exception encountered unloading application domain", _unloadException);
    }

    private void UnloadOnThread()
    {
        try
        {
            // Uncomment to simulate an error in unloading
            //throw new CannotUnloadAppDomainException("Testing: simulated unload error");

            // Uncomment to simulate a timeout while unloading
            //while (true) ;

            AppDomain.Unload(_domain);
        }
        catch (Exception ex)
        {
            // We assume that the tests did something bad and just leave
            // the orphaned AppDomain "out there".
            var msg = DomainDetailsBuilder.DetailsFor(_domain,
                $"Exception encountered unloading application domain: {ex.Message}");

            _unloadException = new NUnitEngineException(msg);
            log.Error(msg);
        }
    }
}

also :

namespace NUnit.Engine.Internal
{
    internal class CustomAssemblyLoadContext : AssemblyLoadContext
    {
        private readonly AssemblyDependencyResolver _resolver;
        private readonly string _basePath;

        public CustomAssemblyLoadContext(string mainAssemblyToLoadPath)
        {
            _resolver = new AssemblyDependencyResolver(mainAssemblyToLoadPath);
            _basePath = Path.GetDirectoryName(mainAssemblyToLoadPath);
        }

        protected override Assembly Load(AssemblyName name)
        {
            var assemblyPath = _resolver.ResolveAssemblyToPath(name);
            return assemblyPath != null ? LoadFromAssemblyPath(assemblyPath) : null;
        }

        ....

never called too.

Note that i run engine 3.14.0 (not 3.17.0). the 3.17 just not read the assembly without adding a dependency on the test.dll itself as i explained, and for me it is a no go. and with 3.17 the lock is still there too. So that s why i downgrade to 3.14

I will return back to engine 3.17 / nUnit 4.1.0 / Adapter 4.5.0 (for the test only) to trace the engine code, to see how the AssemblyLoadContext and context Unload perform. Note i don't mix the adapter and engine as recommanded for the test.

And also to understand why the dll is not read at all.

PatrickLeGarrec commented 6 days ago

some news about engine 3.17

If i use the nuget package the test.dll is not found. (require a dependency on the test project to work, it s bad)

If i make a dependency directly on the engine 3.17 project (see below), the test.dll is found and the xml is ok and does not require dependency on the test, it s good)

317

Lock is still there, i will now check under this version

CharliePoole commented 6 days ago

You can also look at the latest 3.18 dev build on myget, currently https://www.myget.org/feed/nunit/package/nuget/NUnit.Engine/3.18.0-dev00058

PatrickLeGarrec commented 6 days ago

3.18.0-dev00058 => NUnit.Engine.NUnitEngineException: An exception occurred in the driver while loading tests. ---> System.IO.FileNotFoundException: Could not load file or assembly 'Test, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'.

318

Same behavior as 3.17 => file not found

lock is still there in any version, will continue to investigate on 3.17 source code

PatrickLeGarrec commented 6 days ago

i just remove test-centric.engine.metadata . Now it works, dll is read.

the test centric metadat is provided with the 3.17 package also. and fail to read the test. With the source code, there is no tc metadata and it works.

So the idea to remove the tc-metadata dependency fix the issue for me.

318_2

CharliePoole commented 6 days ago

We're talking about the 3.17 engine package, right?

So, the engine makes use of TC metadata to examine assemblies and decide how to run them. In order for the engine to work completely, it needs to be present. Are you sure there is not a copy around from some other source?

PatrickLeGarrec commented 6 days ago

3.17 and 3.18. i will check.

CharliePoole commented 6 days ago

The metadata is a build dependency but the dll is bundled with the package. We might want to change that.

PatrickLeGarrec commented 6 days ago

Well, you were right.
When i have used the engine source project as dependency, the tc-metadata 1.7.1 was in the dir, so i was wrong with my Analysis

Thanks for the information about the role of tc metadata dll.

about 3.17, what i notice is when i run the package, test.dll is not read. but if i make a dependency on test.dll, it works. i think i know why now.

in the Test debug dir (NUnit3TestAdapter 4.5.0), tc metadata dll is 1.7.1 30/10/2021 20:40 (EU datetime)

in the console debug dir with 3.17 engine package, tc metadata dll is 2.0.0. 05/03/2023 18:31

Now, i found something :

With the 3.17 source project, the tc-metadata dll is 1.7.1 30/10/2021 and works. the 3.17 package is 2.0.0 05/03/2023, and fail to read.

CharliePoole commented 6 days ago

TC Metadata version 2 has breaking changes, so if somebody (I think before me) updated 3.17 to use it they may not have correctly updated how they are doing it. Seems doubtful, but possible. The breaking change is that metadata 1.x uses the Mono.Cecil namespace, which caused conflicts with some user code. The 2.x version uses TestCentric.Metadata. There is also a 3.x, with another breaking change: the name of the assembly.

@PatrickLeGarrec It seems that you like digging deep into obscure problems. Would you like to help on this project? We can talk about it offline via email... mine is listed on my profile.

PatrickLeGarrec commented 6 days ago

Yes, i see the error about Mono.Cecil. Ok there is a dll version issue, it happen so often...

I can check the last version of the engine now, to find what goes wrong with the lock. It is an Assembly unload issue for sure, domain context concerns.

Yes we can talk offline, I m always aware about the future of NUnit and overall Microsoft activities.

PatrickLeGarrec commented 5 days ago

Hi Charlie, some fresh and good news today.

I found the lock origin, have a solution for it, and found a strange thing too. I explain :

TestAssemblyLoadContext.cs

=> TestAssemblyLoadContext is not collectible according to https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability and https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability It should be collectible. (Or there is a reason to do not be.. and i don't know why).

In order to allow Unload() on the TestAssemblyLoadcontext and unLoad the assembly(ies), in our case the tests (=package), isCollectible must be true.

public class TestAssemblyLoadContext : AssemblyLoadContext
{
    publicTestAssemblyLoadContext() : base(isCollectible: true)

Also, the Assembly should be loaded into the context as a ByteArray, and disposable. Load and LoadFrom cause lock, always.

The following code is an example of a working Assembly, collectible LoadContext without creating any lock on dll 👍

var alc = new TestAssemblyLoadContext();
Stream ms = new MemoryStream(File.ReadAllBytes(dllPath));
Assembly a = alc.LoadFromStream(ms);

// ...... doing our stuff here

alc.Unload();
ms.Dispose();

File.Delete(dllPath);     // <== NO LOCK

class TestAssemblyLoadContext : AssemblyLoadContext
{
    public TestAssemblyLoadContext() : base(isCollectible: true) {}

    protected override Assembly? Load(AssemblyName name)
    {
        return null;
    }
}

Stream ms = new MemoryStream(File.ReadAllBytes(dllPath)); is the key to replace the Assembly.Load and LoadFrom if you don't want to have some lock. But to avoid some memory leaks it s important to do not forget to dispose the stream. But if you want to keep the control and secure with a dll (like the engine itself), it may be interesting to keep the classic lock way. But in our case, the Test.dll (package) should not be locked for any reason.

Now the next step is to implement the fix.

I think it should be into the runner.unload() or dispose(). I found that engine.dispose() call the unload(), but.... do nothing :

TestAgentRunner.cs

  protected virtual void Dispose(bool disposing)
  {
      if (!_disposed)
      {
          if (disposing)
              Unload();

          _disposed = true;
      }
  }

public virtual void Unload() {  }

I don't know how the have access to TestAssemblyLoadContext and the Assembly (test) from here in order to perform the needed releasing operations.

PatrickLeGarrec commented 5 days ago

or for more efficiency on the resources release :

WeakReference testAlcWeakRef, testAWeakRef;

var alc = new TestAssemblyLoadContext();
Stream ms = new MemoryStream(File.ReadAllBytes(_dll));
Assembly a = alc.LoadFromStream(ms);

testAlcWeakRef = new WeakReference(alc, trackResurrection: true);
testAWeakRef = new WeakReference(a, trackResurrection: true);

alc.Unload();
ms.Dispose();

for (int i = 0; testAWeakRef.IsAlive && testAlcWeakRef.IsAlive && (i < 10); i++)    // 10 loop or more...
{
    GC.Collect();
    GC.WaitForPendingFinalizers();
}

File.Delete(_dll);

But not sure that it is usefull.

CharliePoole commented 5 days ago

@PatrickLeGarrec It seems to me that the Load or LoadFrom ought to work as well, provided that we correctly handle any "Unloading" events. Also, I'd like to compare the handling of this in nunit-engine vs my test-centric engine, which is used in a GUI. I don't rule out a major change such as you describe, but it's substantial so I'd like to try lesser changes, at leasat for the upcoming 3.18 release. Version 4 is another matter, of course.

Would you like to work on doing a PR?

CharliePoole commented 5 days ago

So, after all this... what in your opinion is the minimum code needed to demonstrate the error? Load + Unload + Check for file locked? We could do that for both .NET Framework using an AppDomain and .NET Core using an AssemblyLoadContext.

CharliePoole commented 5 days ago

I created unit tests, which demonstrate that the assembly files are locked when loaded and stay locked after unload. This is actually consistent for both .NET Framework and .NET Core but does not match what I thought was the historical behavior of NUnit under the .NET Framework. So... something changed over the years. Tests are currently in a local branch on my machine.

PatrickLeGarrec commented 5 days ago

Well... after digging a lot, i think i have reached the center of the earth ;) and flood the sea but anyway, it worth the price of time because i learn how the engine works.

So, the story about TestAssemblyLoadContext and the collectible=true gives the capabilities to perform an unload of the AssemblyLoadContext. if collectible is false, you can't. But by doing this, all the context with the others loaded assemblies are unload too. I think it s ok but at a very late stage, because a user can explore and perform some execution (run) of the tests, etc... without purging the context. but it is an other story...may be i m wrong.

But anyway, the changes may occurs a design break in the code. I agree with you.

Then, i retry some code without worry about the collectible flag and the unload, and it makes the things simple. I take the 3.17 code "vanilla" and just track for the lock. I found exactly where it is. and it is not what i was expected :

To help me i use a powertoy File Locksmith and finally there is a Pro, and a Con. I explain :

the following pic show the issue with basic code :

error

In the code, i found that

The vanilla code make lock :

Capture d’écran 2024-07-05 002948

My code DO NOT make lock

Capture d’écran 2024-07-05 002811

BUT there is a problem later :

Capture d’écran 2024-07-05 002413

NUnit.Engine.NUnitEngineException: An exception occurred in the driver while loading tests.
 ---> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ArgumentException: Argument name must not be the empty string (Parameter 'name')
   at NUnit.Framework.Guard.<ArgumentNotNullOrEmpty>g__ThrowArgumentNotNullOrEmpty|1_0(String name)
   at NUnit.Framework.Guard.ArgumentNotNullOrEmpty(String value, String name)
   at NUnit.Framework.Internal.Test..ctor(String pathName, String name, ITypeInfo typeInfo, IMethodInfo method)
   at NUnit.Framework.Internal.Test..ctor(String name)
   at NUnit.Framework.Internal.TestSuite..ctor(String name)
   at NUnit.Framework.Internal.TestAssembly..ctor(String assemblyNameOrPath)
   at NUnit.Framework.Api.DefaultTestAssemblyBuilder.Build(Assembly assembly, String assemblyNameOrPath, IDictionary`2 options)
   at NUnit.Framework.Api.DefaultTestAssemblyBuilder.Build(Assembly assembly, IDictionary`2 options)
   at NUnit.Framework.Api.NUnitTestAssemblyRunner.<>c__DisplayClass34_0.<Load>b__0()
   at NUnit.Framework.Api.NUnitTestAssemblyRunner.WrapInNUnitCallContext[T](Func`1 function)
   at NUnit.Framework.Api.NUnitTestAssemblyRunner.Load(Assembly assembly, IDictionary`2 settings)
   at NUnit.Framework.Api.FrameworkController.LoadTests()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
   --- End of inner exception stack trace ---
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at NUnit.Engine.Drivers.NUnitNetCore31Driver.ExecuteMethod(MethodInfo method, Object[] args) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Drivers\NUnitNetCore31Driver.cs:line 213
   at NUnit.Engine.Drivers.NUnitNetCore31Driver.ExecuteMethod(String methodName, Object[] args) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Drivers\NUnitNetCore31Driver.cs:line 195
   at NUnit.Engine.Drivers.NUnitNetCore31Driver.Load(String assemblyPath, IDictionary`2 settings) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Drivers\NUnitNetCore31Driver.cs:line 110
   at NUnit.Engine.Runners.TestAgentRunner.LoadDriver(IFrameworkDriver driver, String testFile, TestPackage subPackage) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Runners\TestAgentRunner.cs:line 149
   --- End of inner exception stack trace ---
   at NUnit.Engine.Runners.TestAgentRunner.LoadDriver(IFrameworkDriver driver, String testFile, TestPackage subPackage) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Runners\TestAgentRunner.cs:line 153
   at NUnit.Engine.Runners.TestAgentRunner.Load() in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Runners\TestAgentRunner.cs:line 134
   at NUnit.Engine.Runners.TestAgentRunner.EnsurePackageIsLoaded() in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Runners\TestAgentRunner.cs:line 265
   at NUnit.Engine.Runners.TestAgentRunner.Explore(TestFilter filter) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine.core\Runners\TestAgentRunner.cs:line 74
   at NUnit.Engine.Runners.MasterTestRunner.Explore(TestFilter filter) in C:\Repositories.Net\nunit-console-3.17.0\src\NUnitEngine\nunit.engine\Runners\MasterTestRunner.cs:line 214
   at NUnitWrappers.NUnitExplorer.Explore() in C:\Repositories.Net\DemoNUnitEngine-main-3-17_simple\ConsoleApp\NUnitExplorer.cs:line 33

Parameter 'name' is missing during reflection.

We know by doing this, the lock go away :

Stream ms = new MemoryStream(File.ReadAllBytes(assemblyPath));                      
_dependencyContext = DependencyContext.Load(loadContext.LoadFromStream(ms)); 

but we loose information, (location and codebase)... because this is just a flow of byte in memory. And reflection seems to not enjoy ...

To finish, i think there are very very few changes to bring to the code to make the lock away. But, the reflection issue is problematic,... and i don't have the knowledge with the engine.

a PR is not necessary... the goal is now to know if there are breakings change.

The lock on the file is problematic. We can find a solution, a non-sexy workaround with renaming and tmp file to purge at the beginning of a session, just before new lock appairs, but it is not the best, just not impossible.

CharliePoole commented 4 days ago

Separate problems exist for .NET Framework and .NET Core. In the .NET Framework case, we unload the test by unloading the AppDomain. This made sense in the early years of NUnit. BUT, there is a time delay for threads to finish, so we create a thread, which will wait for the domain to be unloaded, and simply return. This works in the nunit3-console context, because we only unload when we are ending the process. It usually works in a GUI, because the user does not click to open a new file very quickly.

PatrickLeGarrec commented 4 days ago

you can try this safe code


WeakReference testAlcWeakRef;
var alc = new TestAssemblyLoadContext();

// the no-lock version
Stream ms = new MemoryStream(File.ReadAllBytes(dllPath));
Assembly a = alc.LoadFromStream(ms);
ms.Dispose();

// the lock version
Assembly a = alc.Load(dllPath);

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

// and when you decide to purge the context :

alc.Unload();
for (int i = 0;  testAlcWeakRef.IsAlive && (i< 10); i++) // 10 or more...  
{
    GC.Collect();
    GC.WaitForPendingFinalizers();
}

// and the flag to the constructor to do not forget
    class TestAssemblyLoadContext : AssemblyLoadContext
    {
        public TestAssemblyLoadContext() : base(isCollectible: true) {}   // <= with the flag, give unload() available.

yes for .Net Framework it is more tricky.

PatrickLeGarrec commented 4 days ago

What i have discovered is that the lock persist even after a safe AssemblyLoadContext Unload() NetCore3.1+ with all the resources disposed. Only a ByteArray does not lock, or after the end of the process, lock is released of course.

PatrickLeGarrec commented 4 days ago

If we know why a name is missing during the reflection

 ---> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.ArgumentException: Argument name must not be the empty string (Parameter 'name')
   at NUnit.Framework.Guard.<ArgumentNotNullOrEmpty>g__ThrowArgumentNotNullOrEmpty|1_0(String name)
   at NUnit.Framework.Guard.ArgumentNotNullOrEmpty(String value, String name)

And if we found a valid workaround, the lock may be fixed by transfering data in memory. By changing how the dll is loaded into the loadcontext. The use of LoadFromStream is respectable i think. It just make no lock, no dependency with the physical file, and not bad at all with the design, plus a small gain in performance. Also during exploration or execution of tests, the file can be workable for another task.

the vanilla code

_dependencyContext = DependencyContext.Load(loadContext.LoadFromAssemblyPath(assemblyPath));

_dependencyContext is null

So for me the difference between the vanilla code and

Stream ms = new MemoryStream(File.ReadAllBytes(assemblyPath));                        
_dependencyContext = DependencyContext.Load(loadContext.LoadFromStream(ms));

is nothing.

PatrickLeGarrec commented 4 days ago

if i compare the Assembly returned by LoadFromAssemblyPath and LoadFromStream, they are the same, no difference

  Assembly asm = loadContext.LoadFromAssemblyPath(assemblyPath);

  Stream ms = new MemoryStream(File.ReadAllBytes(assemblyPath));
  Assembly asm2 = loadContext.LoadFromStream(ms);

  Comparer myComp = new Comparer(new CultureInfo(0x040A, false));
  int i =  myComp.Compare(asm, asm2);
  if (i==0) { Console.WriteLine("no difference"); }
CharliePoole commented 4 days ago

So does this workaround resolve your use case? If so, then I'd like to consider this approach for V4 rather than 3.18 - and for TestCentric possibly, as well.

PatrickLeGarrec commented 4 days ago

We will use a workaround based on renaming and tmp file to purge at the begining of each process session. It is non-sexy and a bad design but it will do the trick. In the case of a testing framework with an engine like this one, very potent (our choice by default), we wait for more. The fact to have a lock on the files is problematic when the platform is shared between desktop ui app, and an automatic runner designed to load and execute grappes of tests on test farms as fast as possible, workflow based.

So we are aware about the evolution of the engine. NUnit is pretty used now as an E2E web/api testing framework, connected to tierce party plugins of worldwide companies, and not only for vs unit testing. The engine is excellent, and how the tests are designed too. But for industrial testing solution, a lock on a file is a limitation.

About the version, we will wait, no problem with that :)

CharliePoole commented 4 days ago

@PatrickLeGarrec has surfaced an interesting problem and proposed a new way to load tests. While this approach is hidden from users, it could have side-effects and I prefer to avoid it for now and it only impacts those who make use of the engine directly rather than through the console. As a result, I'm re-categorizing this as a new feature and targeting the version 4 release.

Summary of earlier discussion: file locks may be avoided by reading the assembly into a memory stream and loading from that stream. @PatrickLeGarrec demonstrates how to do this for .NET Core but it should also be adaptable to use in .NET Framework.

A similar approach is already used in TestCentric.Metadata, which we utilize to examine assemblies without actually loading them.

PatrickLeGarrec commented 3 days ago

Here is the workaround to avoid the lock issue waiting for V4. If the engine is used in a workflow with file transfer (execution farms, active worspace, etc..) there is no workaround.

Desktop UI (the desktop app use an autoload system for quality of life convenience)

private void frmBaseLayout_Load(object sender, EventArgs e)
{
    // Purge all the old locked tests files at startup
    foreach (string strFile in Directory.GetFiles(AppDomain.CurrentDomain.BaseDirectory, searchPattern: "*_tmp.dll"))
    {
        if (Properties.Settings.Default.IsAutoLoad)
        {   // exclude the file to autoload
            if (strFile != Properties.Settings.Default.LastDllLoaded)
                File.Delete(strFile);
        }
        else File.Delete(strFile);
    } 
}

And when i load the tests from the Desktop GUI :

// ######################
// ### MANUAL LOADING ###
// ######################

public void UI_LoadAssembly()
{
    try
    {
        if (Handler.Instance.UcLoadIteration.openFileDialogAssembly.ShowDialog() == DialogResult.OK)
        {
            // generate a randomized temp file. get around the lock issue. see frmBaseLayout load() for the deleting routine.
            Random rnd = new();
            int randomIntInRange = rnd.Next(1000000);
            string fileName = Handler.Instance.UcLoadIteration.openFileDialogAssembly.SafeFileName;
            string fileNameNoExtension = fileName.Replace(".dll", "");
            string renamedFile = fileNameNoExtension + "." + randomIntInRange + "_tmp.dll";
            File.Copy(Handler.Instance.UcLoadIteration.openFileDialogAssembly.FileName, renamedFile, true);  // copy the new file
            Properties.Settings.Default.LastDllLoaded = renamedFile; 

            if (Handler.Instance.Fixture.IsExploredFixtureAssembly(renamedFile))  // explore the new file with engine
            {   ..... some code.....

For information purpose.

Have a nice day.