Port some CoreRT Threading classes to Mono #47333

CoffeeFlux · 2021-01-22T13:17:00Z

Best reviewed commit by commit. In some cases, I've left comments in the commit description. I expect the most interesting commits to be the last few, in particular the annotations.

vargaz · 2021-01-22T13:37:45Z

In addition to the icalls, it should be possible to remove all the w32semaphore/event/mutex files from the netcore build. It might be possible to remove the whole w32handle code on platforms which have no processes like wasm, i.e. w32process-unix.c is the last user of that code.

CoffeeFlux · 2021-01-22T13:58:08Z

It should be yeah, but this PR is already large enough that I'd prefer to do that in a followup.

CoffeeFlux · 2021-02-08T23:29:49Z

Rebased and took out of draft since the other PRs are in. Not sure if this will build, but maybe I'll get lucky?

Still should be largely reasonable to review, and I still recommend commit-by-commit.

src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs

src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.ThreadWaitInfo.Unix.cs

jkotas · 2021-02-09T00:34:32Z

src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs

        private void Initialize()
        {
            InitInternal(this);

-            // TODO: This can go away once the mono/mono mirror is disabled
-            stack_size = _startHelper!._maxStackSize;


I think you also need to delete the stack_size field and pass the stack_size as argument instead.

I pinged Aleksey about this TODO since I don't have context for the recent Thread changes and don't want to spent half an hour looking through it all if possible. I'll update tomorrow after I talk with him.

This is currently used by the runtime.

I pinged Aleksey about this TODO since I don't have context for the recent Thread changes and don't want to spent half an hour looking through it all if possible. I'll update tomorrow after I talk with him.

I don't remember what that comment means anymore except the superficial "once we stop the mirror, the layout of MonoInternalThread can change".

But yea we so still need to pass a stack_size down into the runtime so that it's threaded down to the native thread creation functions. (otherwise it ends up initialized to 0 which I think mean "default size")

I have added this comment one month ago as I was moving more Thread.cs to be shared. It is exactly as @lambdageek says - you need to pass a stack_size down into the native thread creating function.

Hopefully I understood correctly how the stack size stuff is supposed to work?

vargaz · 2021-02-09T01:49:41Z

The runtime changes look ok to me.

marek-safar · 2021-02-09T09:34:17Z

src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.cs

@@ -409,6 +409,13 @@ private static bool SignalAndWait(WaitHandle toSignal, WaitHandle toWaitOn, int
            }
        }

+        internal static void ThrowInvalidHandleException()


internal static void ThrowInvalidHandleException() => throw new InvalidOperationException(SR.InvalidOperation_InvalidHandle) { HResult = HResults.E_HANDLE; }

marek-safar · 2021-02-09T09:42:28Z

src/libraries/System.Private.CoreLib/src/System/Threading/EventWaitHandle.Unix.cs

@@ -19,7 +19,7 @@ private void CreateEventCore(bool initialState, EventResetMode mode, string name
            createdNew = true;
        }

-        private static OpenExistingResult OpenExistingWorker(string name, out EventWaitHandle result)
+        private static OpenExistingResult OpenExistingWorker(string name, out EventWaitHandle? result)


This might be better to move to the caller. It seems to do always do some error checking which is Windows specific

I think we have to match the Windows equivalent here, since otherwise you end up with a bunch of weird platform-specific code at a higher level? Maybe I'm misunderstanding.

I was referring to code patterns like

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/EventWaitHandle.cs

Lines 32 to 45 in 1d9e50c

public static EventWaitHandle OpenExisting(string name)

{

switch (OpenExistingWorker(name, out EventWaitHandle? result))

{

case OpenExistingResult.NameNotFound:

throw new WaitHandleCannotBeOpenedException();

case OpenExistingResult.NameInvalid:

throw new WaitHandleCannotBeOpenedException(SR.Format(SR.Threading_WaitHandleCannotBeOpenedException_InvalidHandle, name));

case OpenExistingResult.PathNotFound:

throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, name));

default:

Debug.Assert(result != null, "result should be non-null on success");

return result;

}

. The API is windows only and on Unix will have never reached error handling.

src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.WaitableObject.Unix.cs

src/mono/System.Private.CoreLib/src/System/Threading/LowLevelLock.cs

CoffeeFlux · 2021-02-09T18:16:03Z

Oh yeah, cc: @fanyang-mono @naricc, as per usual I'm changing threading things and this might have perf implications, either for TE or microbenchmarks. No need to run in advance of merging this PR, but you might see movement.

CoffeeFlux · 2021-02-10T21:04:07Z

Ignore the Windows stuff for now, I'll just boot a Windows machine and sort out whatever build issues are arising there.

src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj

src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs

src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Threading.cs

Some of these are uncovering real issues, so I think it's better to silently ignore the argument for now

This might be a bad idea and we should just call into managed to check?

This is probably not optimal from a linker perspective, but that can be dealt with later

The remaining failures are just related to the named mutexes

Yeehaw

I originally implemented Zoltan's suggestion, but this seems to work and is significantly cleaner. Pre-jitting is also an option here, so if this doesn't work try that next.

I think it would make much more sense to always throw SemaphoreFullException instead of sometimes throwing InvalidOperationException saying the semaphore is full, but this preserves the old behavior.

CoffeeFlux · 2021-03-12T23:37:50Z

I still have a cleanup-related change to push, but this is green on CI. Issues uncovered as part of the port:

#49518

#49521

#48720

I don't consider any of these a big enough deal to block a merge here. All of them would probably block CoreCLR from adopting the managed implementation, but from a Mono perspective the only regression is the interrupts on Windows, and thankfully that appears to be the easiest of the three to address.

For the mutex abandonment, I opted to do it from the Thread object's destructor. As far as I can tell this works fine and is fairly clean, but if the approach is discovered to be faulty I can switch to pre-jitting the method instead.

For the different exception types, I just catch and rethrow. I'm not particularly fond of throwing an InvalidOperationException saying the semaphore is full instead of just a SemaphoreFullException, but that's the legacy behavior.

jkotas

Nice!

CoffeeFlux requested review from vargaz, lambdageek and jkotas January 22, 2021 13:17

dotnet-issue-labeler bot added the area-VM-threading-mono label Jan 22, 2021

monojenkins mentioned this pull request Jan 22, 2021

Port some CoreRT Threading classes to Mono mono/mono#20774

Closed

runfoapp bot mentioned this pull request Jan 22, 2021

Testing out the new system jaredpar/runfo#71

Open

CoffeeFlux force-pushed the corert-threading-changes branch from b204fa1 to ea04f42 Compare February 8, 2021 23:27

CoffeeFlux marked this pull request as ready for review February 8, 2021 23:27

CoffeeFlux requested review from lateralusX, marek-safar and thaystg as code owners February 8, 2021 23:27

jkotas reviewed Feb 8, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs Outdated Show resolved Hide resolved

vargaz reviewed Feb 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.ThreadWaitInfo.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 9, 2021

View reviewed changes

CoffeeFlux requested a review from stephentoub February 9, 2021 01:19

marek-safar reviewed Feb 9, 2021

View reviewed changes

CoffeeFlux mentioned this pull request Feb 9, 2021

[mono] Remove w32 subsystem on wasm #48056

Closed

stephentoub reviewed Feb 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.WaitableObject.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/WaitSubsystem.WaitableObject.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 9, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Threading/LowLevelLock.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 11, 2021

View reviewed changes

src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj Outdated Show resolved Hide resolved

jkotas reviewed Feb 11, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 11, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/WaitHandle.Windows.cs Show resolved Hide resolved

jkotas reviewed Feb 11, 2021

View reviewed changes

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Threading.cs Outdated Show resolved Hide resolved

CoffeeFlux added 17 commits March 11, 2021 15:37

Attempt to fix wasm

b24af36

Fix ifdefs

051fd47

Switch bool to BOOL for interop

100c14a

Always create _waitInfo

1a865ce

Some exclusions

aed3aac

Re-enable the tests and disable the name check

942b575

Some of these are uncovering real issues, so I think it's better to silently ignore the argument for now

Remove thread_interrupt_requested

616188c

This might be a bad idea and we should just call into managed to check?

Correctly abandon mutexes

61de05a

This is probably not optimal from a linker perspective, but that can be dealt with later

Disable tests again

2a12e3f

The remaining failures are just related to the named mutexes

Uncomment unsupported name exception

b84385f

Excise a lot more runtime code

17ba865

Yeehaw

Disable another test

bb3ae83

Windows build fix

696fc10

Disable InterruptTest on Windows

65801bd

Add link to issue on reentrant wait support for Windows

3472b0b

Move mutex abandoning to the managed Thread object's finalizer

def05ab

I originally implemented Zoltan's suggestion, but this seems to work and is significantly cleaner. Pre-jitting is also an option here, so if this doesn't work try that next.

Rethrow with InvalidOperationException when called from WaitHandle

891987b

I think it would make much more sense to always throw SemaphoreFullException instead of sometimes throwing InvalidOperationException saying the semaphore is full, but this preserves the old behavior.

CoffeeFlux force-pushed the corert-threading-changes branch from 59504ff to 891987b Compare March 12, 2021 05:47

CoffeeFlux added 2 commits March 12, 2021 05:44

Another ActiveIssue on Windows

a9dd5c3

Linker exclusion no longer needed

cc14788

lambdageek approved these changes Mar 13, 2021

View reviewed changes

vargaz approved these changes Mar 13, 2021

View reviewed changes

jkotas approved these changes Mar 13, 2021

View reviewed changes

marek-safar merged commit 6791d05 into dotnet:main Mar 13, 2021

jkotas mentioned this pull request Mar 13, 2021

System.Threading.Thread tests crashing on Mono #49583

Closed

CoffeeFlux mentioned this pull request Mar 13, 2021

Test on Mono are failing on lots of PRs with fail fasts #49569

Closed

runfoapp bot mentioned this pull request Mar 15, 2021

InterruptInFinallyBlockTest_SkipOnDesktopFramework failed on Linux Debug x64 Mono in CI #43953

Closed

ghost locked as resolved and limited conversation to collaborators Apr 12, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port some CoreRT Threading classes to Mono #47333

Port some CoreRT Threading classes to Mono #47333

CoffeeFlux commented Jan 22, 2021 •

edited

Loading

vargaz commented Jan 22, 2021 •

edited

Loading

CoffeeFlux commented Jan 22, 2021

CoffeeFlux commented Feb 8, 2021 •

edited

Loading

jkotas Feb 9, 2021

CoffeeFlux Feb 9, 2021

vargaz Feb 9, 2021

lambdageek Feb 9, 2021

jkotas Feb 9, 2021

CoffeeFlux Feb 10, 2021

vargaz commented Feb 9, 2021

marek-safar Feb 9, 2021

marek-safar Feb 9, 2021

CoffeeFlux Feb 10, 2021

marek-safar Feb 11, 2021

CoffeeFlux commented Feb 9, 2021

CoffeeFlux commented Feb 10, 2021

CoffeeFlux commented Mar 12, 2021 •

edited

Loading

jkotas left a comment

	public static EventWaitHandle OpenExisting(string name)
	{
	switch (OpenExistingWorker(name, out EventWaitHandle? result))
	{
	case OpenExistingResult.NameNotFound:
	throw new WaitHandleCannotBeOpenedException();
	case OpenExistingResult.NameInvalid:
	throw new WaitHandleCannotBeOpenedException(SR.Format(SR.Threading_WaitHandleCannotBeOpenedException_InvalidHandle, name));
	case OpenExistingResult.PathNotFound:
	throw new DirectoryNotFoundException(SR.Format(SR.IO_PathNotFound_Path, name));
	default:
	Debug.Assert(result != null, "result should be non-null on success");
	return result;
	}

Port some CoreRT Threading classes to Mono #47333

Port some CoreRT Threading classes to Mono #47333

Conversation

CoffeeFlux commented Jan 22, 2021 • edited Loading

vargaz commented Jan 22, 2021 • edited Loading

CoffeeFlux commented Jan 22, 2021

CoffeeFlux commented Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vargaz commented Feb 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoffeeFlux commented Feb 9, 2021

CoffeeFlux commented Feb 10, 2021

CoffeeFlux commented Mar 12, 2021 • edited Loading

jkotas left a comment

Choose a reason for hiding this comment

CoffeeFlux commented Jan 22, 2021 •

edited

Loading

vargaz commented Jan 22, 2021 •

edited

Loading

CoffeeFlux commented Feb 8, 2021 •

edited

Loading

CoffeeFlux commented Mar 12, 2021 •

edited

Loading