Skip to content
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

[mono] More Threading changes #49637

Merged
merged 20 commits into from
Apr 6, 2021

Conversation

CoffeeFlux
Copy link
Contributor

Contains: general cleanup, domain removal, and moving to sleeping from managed

We can probably also switch to setting the priority in managed, but I'll do that separately.

I have not tested this on Windows, but I assume it will work fine.

@jkotas is there argument validation for Sleep on the native side in CoreCLR I should remove now that it's happening in managed? Or would you prefer that change be Mono-only?

@vargaz
Copy link
Contributor

vargaz commented Mar 15, 2021

The 'Libraries Test Run release mono Linux x64 Debug' lane failure looks relevant.

@marek-safar
Copy link
Contributor

@CoffeeFlux any update here?

@CoffeeFlux
Copy link
Contributor Author

@marek-safar I held off on this until after P3 and am out today and tomorrow, but I'll push my changes this Thursday.

@CoffeeFlux CoffeeFlux marked this pull request as draft April 2, 2021 15:49
@CoffeeFlux
Copy link
Contributor Author

CoffeeFlux commented Apr 2, 2021

I validated that the current strategy for mutex abandonment is indeed broken, and will add a test and a fix to this before taking it out of draft. Sample program that will succeed on CoreCLR but fail with our current approach:

using System;
using System.Threading;

public class Example
{
    private static Mutex _orphan = new Mutex();

    [MTAThread]
    public static void Main()
    {
        Thread t = new Thread(new ThreadStart(AbandonMutex));
        t.Start();
        t.Join();

        // Wait on the abandoned mutexes. The WaitOne returns
        // immediately, because its wait condition is satisfied by
        // the abandoned mutex, but on return it throws
        // AbandonedMutexException.
        try
        {
            _orphan.WaitOne();
            Console.WriteLine("WaitOne succeeded.");
        }
        catch(AbandonedMutexException ex)
        {
            Console.WriteLine("Exception on return from WaitOne." +
                "\r\n\tMessage: {0}", ex.Message);
        }
        finally
        {
            // Whether or not the exception was thrown, the current
            // thread owns the mutex, and must release it.
            //
            _orphan.ReleaseMutex();
        }
    }

    [MTAThread]
    public static void AbandonMutex()
    {
        _orphan.WaitOne();
        // Abandon the mutex by exiting without releasing it.
        Console.WriteLine("Thread exits without releasing the mutexes.");
    }
}

@jkotas
Copy link
Member

jkotas commented Apr 2, 2021

is there argument validation for Sleep on the native side in CoreCLR I should remove now that it's happening in managed?

Yes, that would be nice. Look for ThreadNative::Sleep in comsynchronizable.cpp.

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the coop attach detach functions have non-obvious callers that depend on the return type that need to be adjusted.

src/mono/mono/metadata/threads-types.h Show resolved Hide resolved
@CoffeeFlux
Copy link
Contributor Author

I think we should be good on the Mono side other than fixing abandoning and adding an associated test.

There is additional cleanup potential with #50656, but that is outside the scope of this PR.

@CoffeeFlux CoffeeFlux marked this pull request as ready for review April 2, 2021 21:22
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for cleaning this up.

I'd avoid calling into managed while holding a lock. (I'm not sure that anything would necessarily go wrong, but it seems like it wouldn't be difficult to call outside the lock.)

src/mono/mono/metadata/threads.c Outdated Show resolved Hide resolved
@CoffeeFlux
Copy link
Contributor Author

There were conflicts with Zoltan's latest domain cleanup PR, so rebased and we should be good now. Going to try and land this before #50771 but it'll likely cause conflicts there @vargaz

@CoffeeFlux
Copy link
Contributor Author

The remaining lanes look mostly CoreCLR-related, and the Windows ones seem stalled for infra-related reasons. Everything passed pre-rebase, so merging.

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.

7 participants