Mutex code cleanup and removal of unused types by ViktorHofer · Pull Request #14181 · dotnet/coreclr

ViktorHofer · 2017-09-26T00:04:51Z

@jkotas should I also align Interop code and move it into shared?

ViktorHofer · 2017-09-26T00:14:44Z

I just noticed that most of the cleanup was already done in corert.

jkotas · 2017-09-26T00:20:26Z

-            else
+        }
+
+        internal class MutexTryCodeHelper


If you would like, you can get rid of this too and just replace it with try/finally block

jkotas · 2017-09-26T00:22:04Z

should I also align Interop code and move it into shared?
I just noticed that most of the cleanup was already done in corert.

Ideally, both of these should be done together. I would wait with this - the synchronization/threading is one of the harder areas to unify.

jkotas · 2017-09-26T00:22:11Z

LGTM otherwise

ViktorHofer · 2017-09-26T15:11:25Z

PTAL. I aligned the code more to the corert version to simplify converging later.

jkotas · 2017-09-26T15:59:58Z

-                throw new ArgumentException(SR.Format(SR.Argument_WaitHandleNameTooLong, Path.MaxPath), nameof(name));
-            }
-#endif // PLATFORM_WINDOWS
+            VerifyNameForCreate(name);


This used to be under Windows ifdef, but it is enabled for all platforms now

On corert it's throwing when a name is provided: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Threading/Mutex.Unix.cs#L16

Should this be the same here? I'm not sure about this...

Expected - CoreRT does not support named mutexes on Unix today.

jkotas · 2017-09-26T16:01:07Z

+
 #if !PLATFORM_UNIX
-            if (System.IO.Path.MaxPath < name.Length)
+            if (Path.MaxPath < name.Length)


Should this use the VerifyNameForCreate method?

The VerifyNameForCreate method also checks if name is not null which we don't need in this case but I guess it doesn't really matter performance-wise. I will reuse it here

jkotas · 2017-09-26T17:12:41Z

-            return OpenExistingWorker(name, (MutexRights)0, out result) == OpenExistingResult.Success;
+            createdNew = errorCode != Interop.Errors.ERROR_ALREADY_EXISTS;
+            SafeWaitHandle = mutexHandle;
+            hasThreadAffinity = true;


It looks like you have deleted this in other case, but not this one.

Do you think it's safe to remove them everywhere now that I'm no longer using the RuntimeHelpers.CleanupCode method?

It think so. It is also in WaitHandle and a few FCalls - it can be removed there as well.

ViktorHofer · 2017-09-27T16:03:22Z

@jkotas should we merge that for now and I will look into other classes like SafeHandle in a separate PR?

jkotas · 2017-09-27T16:34:31Z

-                case OpenExistingResult.NameInvalid:
+                mutexHandle.SetHandleAsInvalid();
+#if PLATFORM_UNIX
+                if (errorCode == Win32Native.ERROR_FILENAME_EXCED_RANGE)


Nit: Should this rather be Interop.Errors.ERROR_FILENAME_EXCED_RANGE?

jkotas · 2017-09-27T16:35:32Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline

jkotas · 2017-09-27T16:36:03Z

should we merge that for now

ok. (once the corefx legs are green)

ViktorHofer · 2017-09-27T20:38:21Z

the output from the corefx legs is unfortunately unusable :(

jkotas · 2017-09-27T20:44:08Z

The corefx legs are failing with: 'System.Runtime.Intrinsics.X86.Avx.Insert(System.Runtime.Intrinsics.Vector256, System.Single*, System.Byte)' does not exist in the implementation

@fiigii Was it intentional that you have deleted byte index from several Avx.Insert overloads in your change #14164? If not, could you please submit PR to fix it?

fiigii · 2017-09-27T20:57:30Z

@jkotas Sorry, my bad. It was incautiously deleted by regexp.

jkotas · 2017-09-27T21:38:53Z

@ViktorHofer #14231 should fix the corefx break. We should retry after it goes in.

ViktorHofer · 2017-09-27T23:10:50Z

@dotnet-bot test this please

ViktorHofer · 2017-09-27T23:11:07Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline

ViktorHofer · 2017-09-28T00:48:00Z

Failing legs are unrelated to my changes... Some FileSystemWatcher tests are failing

ViktorHofer · 2017-09-28T12:22:53Z

@dotnet-bot test Ubuntu x64 corefx_baseline
@dotnet-bot test Ubuntu x64 Formatting

ViktorHofer · 2017-09-28T14:09:04Z

Maybe the errors are related. I need to check...

System.IO.Tests.FileSystemWatcherTests_netstandard17.EndInit_ResumesPausedEnableRaisingEvents
System.IO.Tests.FileSystemWatcherTests_netstandard17.EndInit_ResumesPausedEnableRaisingEvents(setBeforeBeginInit: True)
System.IO.Tests.FileSystemWatcherTests_netstandard17.EndInit_ResumesPausedEnableRaisingEvents(setBeforeBeginInit: False)
System.IO.Tests.File_Create_Tests.FileSystemWatcher_File_Create

jkotas · 2017-09-28T15:52:24Z

The FileSystemWatcher failures are #14154

ViktorHofer · 2017-09-29T15:33:27Z

@dotnet-bot test Ubuntu x64 corefx_baseline

ViktorHofer · 2017-09-29T15:34:28Z

@dotnet-bot test Ubuntu x64 Checked Build and Test (Jit - CoreFx)

ViktorHofer added the area-Meta-corelib label Sep 26, 2017

ViktorHofer self-assigned this Sep 26, 2017

ViktorHofer requested a review from jkotas September 26, 2017 00:04

dnfclas added the cla-already-signed label Sep 26, 2017

jkotas reviewed Sep 26, 2017

View reviewed changes

ViktorHofer force-pushed the MutexCodeCleanup branch from 8a9dc37 to b07e251 Compare September 26, 2017 23:16

Mutex code cleanup and removal of unused types

eff880d

ViktorHofer force-pushed the MutexCodeCleanup branch from b07e251 to eff880d Compare September 26, 2017 23:18

jkotas reviewed Sep 27, 2017

View reviewed changes

nit pr feedback

6626baf

ViktorHofer merged commit 97e0093 into dotnet:master Oct 2, 2017

ViktorHofer deleted the MutexCodeCleanup branch October 2, 2017 19:54

danmoseley mentioned this pull request Jan 31, 2020

Mutex name null causes FailFast in chk on Unix dotnet/runtime#24558

Closed

Conversation

ViktorHofer commented Sep 26, 2017

Uh oh!

ViktorHofer commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 26, 2017

Uh oh!

jkotas commented Sep 26, 2017

Uh oh!

ViktorHofer commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 27, 2017

Uh oh!

jkotas commented Sep 27, 2017

Uh oh!

ViktorHofer commented Sep 27, 2017

Uh oh!

jkotas commented Sep 27, 2017

Uh oh!

fiigii commented Sep 27, 2017

Uh oh!

jkotas commented Sep 27, 2017

Uh oh!

ViktorHofer commented Sep 27, 2017

Uh oh!

ViktorHofer commented Sep 27, 2017

Uh oh!

ViktorHofer commented Sep 28, 2017

Uh oh!

ViktorHofer commented Sep 28, 2017

Uh oh!

ViktorHofer commented Sep 28, 2017

Uh oh!

jkotas commented Sep 28, 2017

Uh oh!

ViktorHofer commented Sep 29, 2017

Uh oh!

ViktorHofer commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants