Unsafe to use TestContext.CurrentContext.TestDirectory

[abelbraaksma]

When using TestContext.CurrentContext.TestDirectory several things can happen:

• CurrentContext is null
• some regular exception
• SocketException is thrown

The first two can be worked around, but the last one cannot be caught in a try/catch when calling TestDirectory. I'm not sure why, but my suspicion is that the error is thrown asynchronously.

I think that generally speaking, property gettors should either return null (if there's reason for such a case) or return a legal value, but they should not raise exceptions. Even more so when they are static, as this leads to hard-to-debug exceptions in static cctors.

This behavior is most apparent when running tests with nunit3-console.exe, where tests are loaded through TestSourceAttribute which require access to resources relative to the local directory. In such cases it makes sense to use TestContext.CurrentContext.TestDirectory, as other methods are unreliable, but unfortunately, the preferred method is unsafe and may crash your whole test-session.

Extra info:

The stacktrace of the SocketException, which is often thrown, but not always in the mentioned scenarios, is as follows:

System.Net.Sockets.SocketException (0x80004005): An existing connection was forcibly closed by the remote host

Server stack trace:
   at System.Net.Sockets.Socket.Receive(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags)
   at System.Runtime.Remoting.Channels.SocketStream.Read(Byte[] buffer, Int32 offset, Int32 size)
   at System.Runtime.Remoting.Channels.SocketHandler.ReadFromSocket(Byte[] buffer, Int32 offset, Int32 count)
   at System.Runtime.Remoting.Channels.SocketHandler.Read(Byte[] buffer, Int32 offset, Int32 count)
   at System.Runtime.Remoting.Channels.Tcp.TcpFixedLengthReadingStream.Read(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.BinaryReader.ReadBytes(Int32 count)
   at System.Runtime.Serialization.Formatters.Binary.SerializationHeaderRecord.Read(__BinaryParser input)
   at System.Runtime.Serialization.Formatters.Binary.__BinaryParser.ReadSerializationHeaderRecord()
   at System.Runtime.Serialization.Formatters.Binary.__BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(HeaderHandler handler, __BinaryParser serParser, Boolean fCheck, Boolean isCrossAppDomain, IMethodCallMessage methodCallMessage)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, HeaderHandler handler, Boolean fCheck, Boolean isCrossAppDomain, IMethodCallMessage methodCallMessage)
   at System.Runtime.Remoting.Channels.CoreChannel.DeserializeBinaryResponseMessage(Stream inputStream, IMethodCallMessage reqMsg, Boolean bStrictBinding)
   at System.Runtime.Remoting.Channels.BinaryClientFormatterSink.SyncProcessMessage(IMessage msg)

Exception rethrown at [0]:
   at System.Runtime.Remoting.Proxies.RealProxy.HandleReturnMessage(IMessage reqMsg, IMessage retMsg)
   at System.Runtime.Remoting.Proxies.RealProxy.PrivateInvoke(MessageData& msgData, Int32 type)
   at NUnit.Engine.ITestAgent.Stop()
   at NUnit.Engine.Runners.ProcessRunner.Dispose(Boolean disposing)
   at NUnit.Engine.Runners.AbstractTestRunner.Dispose()
   at NUnit.Engine.Runners.MasterTestRunner.Dispose(Boolean disposing)
   at NUnit.Engine.Runners.MasterTestRunner.Dispose()
   at NUnit.ConsoleRunner.ConsoleRunner.RunTests(TestPackage package, TestFilter filter)
   at NUnit.ConsoleRunner.Program.Main(String[] args)

[rprouse]

I am closing old uncategorized issues that have not had comments or made progress in several years. If anyone comes back with a compelling argument for these issues, we can reopen.

[abelbraaksma]

@rprouse, I understand you'd want to clean up, but if this is still a bug, it's a nasty one when you hit it. I just checked the code of the project that ran into this, which is still in development, and I noticed comments that explicitly say "don't use TestDirectory" with a reference to this issue, and a workaround using GetExecutingAssembly() (which in itself isn't reliable either, but for different reasons, so that workaround is a little bit more involved).

The way I see it: currently, combining TestSourceAttribute and TestDirectory is impossible, and leads to unpredictable and uncatchable errors. We could solve it one or two ways:

1. Remove this property completely. It is currently not stable.
2. Wrap whatever is inside this property into a try/catch and make sure it can be accessed concurrently.

My guess is that, when people see SocketException, they won't suspect reading an innocent property can cause such an out-of-bands error and they won't solve it by finding this issue and/or reporting it.

All that said, I haven't seriously tried the old-style approach for a while and maybe this bug was resolved meanwhile by other commits?

[abelbraaksma]

On a more general note: static methods, properties etc are best coded very defensively and thread-safe.

[rprouse]

@abelbraaksma can you produce a simple test case that we can use to reproduce and fix this issue? I am reopening with the confirm label.

[CharliePoole]

@rprouse @abelbraaksma I think the central issue here is that there is (or was) no TestContext when calling the property outside of test execution. Of course, the TestCaseSource method is actually called before test execution even begins, so CurrentContext is quite properly null at that point.

I said "was" above because, at a certain point, we allowed ourselves to be convinced that creating a sort of artificial TestContext at this point would be convenient for users and we did so. Seeing all the issues that has caused, I really wish we had refused and instead thrown a nice exception - rather than just the NRE that will now occur.

Obviously, you can't do that now as too much time has passed but this may be something to put on the list or NUnit 4.0.

WRT socket exceptions, I can't see how that would happen unless the loaded test assembly is accessed through a remote connection, but I don't think we ever supported that.

[abelbraaksma]

WRT socket exceptions, I can't see how that would happen unless the loaded test assembly is accessed through a remote connection

No, everything was local. My guess is that it was caused in the RPC communication layer between the main process and the test process, and perhaps some race condition (the stack trace shows a lot of System.Runtime.Remoting). But yeah, that's just an educated guess at best.

I still maintain the code that caused this at the time. I'll try to find out whether it still reproduces with recent versions of the nunit gui.

[CharliePoole]

@abelbraaksma AFAICS we haven't been talking about a GUI before this in the thread. Did I miss something?

[abelbraaksma]

This behavior is most apparent when running tests with nunit3-console.exe,

@CharliePoole, My bad, it was this one. I actually don't even know if that spawns extra processes. But I'll test with the gui as well just in case.

[CharliePoole]

Actually there is no longer an NUnit project GUI - there was one in V2. I maintain a separate GUI in a project of my own - the testcentric-gui projecct on GitHub - but I don't think it's of any help to you in this issue unless you actually want a GUI for some reason. It's probably simplest if you figure out what is causing the problem in the environment where you normally run tests.

In the case of nunint3-console, a separate process is spawned by default. Most likely something in your test is crashing that process and you are seeing the resulting remoting exception and not the original cause. If your test assembly will run in process, you may get more information. Try using the --inprocess option.

[abelbraaksma]

I've checked my commit history related to this thread (actually, parts referred to nunit/nunit-console#405, which is not available anymore, others linked to the current thread #2872).

The commit-comments further link to #1834, which describes exactly the same SocketException as this one, this comment by @ChrisMaddock shows that it was reproducible, but not debuggable (i.e.: potential race condition): #1834 (comment).

This, in turn, is then fixed in nunit/nunit-console#223.

Looking back at those commit messages I can distill first a rollback from NUnit 3.10 to 3.6, and a few commits later it appeared that some app.config were incorrectly trying to load the wrong versions through BindingRedirect. The 3.6 (and 3.7) versions are mentioned in this comment by @tom-dudley (#1834 (comment)) as susceptible to the SocketException. If anything, my rollback at the time introduced this exception. What blurs things further is that I was actually investigating a -100 exit code from NUnit, which is notoriously hard to debug (though it usually is nothing more than a SOE).

Next commits upgrade everything to 3.10 and remove any version inconsistencies throughout the projects and solutions. Nonetheless, the comment in the code remained, using an alternative way for getting TestContext.CurrentContext.TestDirectory in case I dynamically detect the NUnit console runner is attached (I use TeamCity, which uses the Console Runner). This led me to believe the bug was still present.

Today I ran all tests with this workaround disabled, and there were no crashes related to NUnit, no SocketException anymore.

Bottom line: the SocketException really did exist, had nothing to do with loading resources over the network, and was a bug with the versions of NUnit that I used at the time. The bug was resolved through nunit/nunit-console#223 and consequently disappeared after I upgraded to versions of 3.10 and up.

Apologies for the request for reopening, the bug was truly recognized and resolved (prior to my own reporting).

Try using the --inprocess option.

Good to know this if I need to investigate any future issue I might have. Thanks!

[ChrisMaddock]

Glad we're all sorted! 😄