-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eliminating locking of assembly file while test is loaded #1425
Comments
Most of the methods defined in 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. |
If you encounter a bug using Unload, we can reopen this. |
You describe perfectly what i needed. Unload does not work too but it is normal. I create a small demo project to check the capabilities of the engine. I was thinking about your test-centric GUI. but you need to explore the unknown .dll just loaded, and the user may load another dll, etc... Thanks anyway |
After runner.Explore, even with a safe unload() under the same process, the file is locked. |
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. |
Here is : https://github.com/PatrickLeGarrec/DemoNUnitEngine |
Alternative: We copied and renamed the test.dll and explore the renamed file. The dll must be a renamed copy of the dependency (detail of the renamed file is still test.dll) to work properly. 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. |
i downgrade to Engine 3.13 and NUnitTestAdapter 4.2.0 (and Nunit framework 3.13 instead of 4.1.0) 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. But it does not explain the lock on the file. |
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. |
No problem Charlie, take your time, it's holiday time here so :) |
I update the repo with a better version. The package combo is old but the exploration now works with the same code. [EDIT] i confirm that only engine 3.14.0 / Adapter 4.3.0 / Nunit 3.14.0 read the dll alone. |
Well, i have ran deeper into the engine code and found that the issue would be caused by the Assembly loading. TestEngineActivator.cs : This code is called.
But the lock is still there. Also, i see AssemblyLoadContext and CustomAssemblyLoadContext under NETCOREAPP3_1_OR_GREATER I m sure that the lock is due to this, because if i neutralize the call to LoadResult from MasterTestRunner.cs Explore
there is no lock on the assembly. Clues : DomainManager.cs :
AND DomainUnloader methods are never called.
also :
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. |
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) Lock is still there, i will now check under this version |
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 |
3.18.0-dev00058 => NUnit.Engine.NUnitEngineException: An exception occurred in the driver while loading tests. Same behavior as 3.17 => file not found lock is still there in any version, will continue to investigate on 3.17 source code |
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? |
3.17 and 3.18. |
The metadata is a build dependency but the dll is bundled with the package. We might want to change that. |
Well, you were right. 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. 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. |
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. |
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. Yes we can talk offline, I m always aware about the future of NUnit and overall Microsoft activities. |
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 In order to allow Unload() on the TestAssemblyLoadcontext and unLoad the assembly(ies), in our case the tests (=package), isCollectible must be true.
Also, the Assembly should be loaded into the context as a ByteArray, and disposable. The following code is an example of a working Assembly, collectible LoadContext without creating any lock on dll 👍
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. Now the next step is to implement the fix. I think it should be into the runner.unload() or dispose(). TestAgentRunner.cs
I don't know how the have access to TestAssemblyLoadContext and the Assembly (test) from here in order to perform the needed releasing operations. |
or for more efficiency on the resources release :
But not sure that it is usefull. |
@PatrickLeGarrec Would you like to work on doing a PR? |
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. |
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. |
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 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. 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 : In the code, i found that The vanilla code make lock : My code DO NOT make lock BUT there is a problem later :
Parameter 'name' is missing during reflection. We know by doing this, the lock go away :
but we loose information, (location and codebase)... because this is just a flow of byte in memory. To finish, i think there are very very few changes to bring to the code to make the lock away. 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. |
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. |
you can try this safe code
yes for .Net Framework it is more tricky. |
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. |
If we know why a name is missing during the reflection
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 is null So for me the difference between the vanilla code and
is nothing. |
if i compare the Assembly returned by LoadFromAssemblyPath and LoadFromStream, they are the same, no difference
|
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. |
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 :) |
@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 |
Here is the workaround to avoid the lock issue waiting for V4. Desktop UI
For information purpose. Have a nice day. |
Hi :)
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.
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 ?
The text was updated successfully, but these errors were encountered: