dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.28k stars 4.74k forks source link

`dotnet test` shouldn't crash, even if Dispose method is throwing exception during the Finalize method #108008

Open WindingWinter opened 1 month ago

WindingWinter commented 1 month ago

Description

First let me say that I'm using NUnit, but I don't think this is NUnit related. Rather it has to do with how The Finalize method works.

As we all know Finalize will call deconstructor. And according to the popular IDisposable pattern, we put Dispose method in the deconstructor to clean up the resources.

Now what if the Dispose generates an exception? The whole dotnet test will be brought down and the test cannot continue! Which means that if I my first test is already crashing in Dispose method, then my subsequent tests couldn't run if garbage collector is kicking into action.

Worse of all, this problem will come at indeterministic time for the developers( because GC works indeterministically), leaving them confused.

You might think this is not a big problem, because during manual testing we are supposed to call Dispose ourselves and we should be able to see the crash ourselves right?

But it's not always as straightforward. The issue might not be reproducible during testing time. In actual application or environment, the program can crash randomly even at the start of the constructor, due to Thread being aborted ( due to bugs in the user code), and hence the Dispose might be called in the way we don't anticipate. Like in this example, even readonly field throws Null Reference Exception because the constructor doesn't get to finish initializing!

Reproduction Steps

I can create a small sample to reproduce the issue, here's the main code

    public class AAClass
    {
        public void EmptyMethod()
        {

        }
    }
    public class ObservableObject:IDisposable
    {
        ~ObservableObject()
        {
            Dispose();
        }

        private bool _isDisposed;

        public void Dispose()
        {
            Console.WriteLine("Disposing OO...");

            if (!_isDisposed)
            {
                AAClass claAA = null;
                //this will throw a Null Reference Exception, but the issue is that
                // this will also bring down dotnet test!

                claAA.EmptyMethod();
            }

            _isDisposed = true;
        }
    }

And here's the test code:

   [TestFixture]
   public class Net80TestClasses
   {

       [Test]
       public void AATest()
       {
           ObservableObject observable = new ObservableObject();
           Assert.That(1, Is.EqualTo(2));
       }

       [Test]
       public void BBRunMe()
       {
           Thread.Sleep(20*1000);
           GC.Collect();
           GC.WaitForPendingFinalizers();  //Force GC so that Finalize will be called and crash the whole dotnet test

       }

       [Test]
       public void ZZHehe()
       {
           Thread.Sleep(20 * 1000);
          Assert.That(1, Is.EqualTo(2));

       }

       [Test]
       public void TestPass()
       {
           Assert.That(1, Is.EqualTo(1));
       }

       [Test]
       public void TestFail()
       {
           Assert.That(1, Is.EqualTo(2));
       }

       [Test]
       public void RunMe()
       {
           TestContext.WriteLine("TestLibNet80.RunMe" + RunTimeInfo.NetCoreFramework);

           TestContext.WriteLine("TestLibNet80.RunMe" + RunTimeInfo.RunTimeFramework());
       }

Expected behavior

So I would suggest that dotnet test must still be able to continue even though the Dispose method throws exception during the Garbage Collection time.

Actual behavior

And the test run will be aborted. Only 1 test is run despite that there are many of them.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

huoyaoyuan commented 1 month ago

C# language says exception from finalizer is unspecified: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/exceptions#214-how-exceptions-are-handled

Otherwise, if an exception occurs during finalizer execution, and that exception is not caught, then the behavior is unspecified.

ECMA-335 doesn't explicitly mention exception from finalizer. It hints that the exception can't be handled by others and will cause process termination.

There are other fatal errors that can't be caught by a test framework, like AccessViolation from incorrect use of unsafe code, or StackOverflow. Exception from finalizer is a similar fatal error.

huoyaoyuan commented 1 month ago

In actual application or environment, the program can crash randomly even at the start of the constructor, due to Thread being aborted ( due to bugs in the user code), and hence the Dispose might be called in the way we don't anticipate. Like in this example, even readonly field throws Null Reference Exception because the constructor doesn't get to finish initializing!

A small reproduction for this:

using System;

while (true)
{
    try
    {
        var derived = new Derived();
    }
    catch (NotImplementedException)
    {

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

abstract class Base
{
    private readonly object obj = new object();

    protected Base(int unused) { }

    ~Base()
    {
        lock (obj) // will crash!
        {
        }
    }
}

class Derived : Base
{
    private static int Throws() => throw new NotImplementedException();

    public Derived() : base(Throws()) { }
}

Exceptions before the base constructor call can produce corrupted finalizable object.