Skip to content

Conversation

@CoffeeFlux
Copy link
Contributor

@CoffeeFlux CoffeeFlux commented Mar 15, 2021

Fixes #49583

@jkotas
Copy link
Member

jkotas commented Mar 15, 2021

I think the problem is caused by Thread constructors that throw exceptions before being fully initialized. Consider:

        public Thread(ThreadStart start)
        {
            if (start == null)
            {
                // If we take this path, we will have Thread object with null WaitInfo
                throw new ArgumentNullException(nameof(start));
            }

            _startHelper = new StartHelper(start);

            Initialize();
        }

@CoffeeFlux
Copy link
Contributor Author

CoffeeFlux commented Mar 15, 2021

If the issue is the constructor, I don't understand why it would be flaky. I guess it's because most of the time the finalizer won't run before the runtime as a whole gets shut down? This should at least be easy to test.

@CoffeeFlux CoffeeFlux changed the title Add temporary null check for WaitInfo Add null check for WaitInfo Mar 15, 2021
@CoffeeFlux
Copy link
Contributor Author

CoffeeFlux commented Mar 15, 2021

Looks like you're right:

using System;
using System.Threading;

namespace HelloWorld
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            try {
                new Thread((ThreadStart)null);
            }
            catch {}
            GC.Collect();
            GC.WaitForPendingFinalizers();
            Console.WriteLine("done");
        }
    }
}

I didn't even consider that finalizers are run in C# if the constructor throws... too used to C++. I guess in that case the null check is fine, and I'll move it as suggested. Thanks!

@CoffeeFlux CoffeeFlux force-pushed the corert-threading-waitinfo-null branch from 99dd4c5 to 91f3560 Compare March 15, 2021 06:33
@CoffeeFlux CoffeeFlux requested a review from marek-safar as a code owner March 15, 2021 06:33
@CoffeeFlux
Copy link
Contributor Author

Failure is #49569

@CoffeeFlux CoffeeFlux merged commit c116047 into dotnet:main Mar 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Threading.Thread tests crashing on Mono

5 participants