dotnet / runtime

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

System.Diagnostics.Tests.ProcessTests.MaxWorkingSet_GetNotStarted wrong exception code on BSD-ish systems #105422

Open gwr opened 1 month ago

gwr commented 1 month ago

Description

In the SunOS port I'm working on (See https://github.com/dotnet/runtime/pull/105403) I'm sharing this file src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs with FreeBSD, OSX, etc. During my testing I discovered a bug and have a fix to suggest.

The bug is that for a new Process object (for which there is no Unix process ID started yet) the calls to GetWorkingSetLimits and SetWorkingSetLimits are meant to throw System.InvalidOperationException but they throw System.PlatformNotSupportedException instead. Here's the test output:

System.Diagnostics.Tests.ProcessTests.MaxWorkingSet_GetNotStarted_ThrowsInvalidOperationException [FAIL]
Assert.Throws() Failure: Exception type was not an exact match
Expected: typeof(System.InvalidOperationException)
Actual:   typeof(System.PlatformNotSupportedException)
---- System.PlatformNotSupportedException : Getting or setting the working set limits on other processes is not supported on this platform.

Same for MinWorkingSet

System.Diagnostics.Tests.ProcessTests.MinWorkingSet_GetNotStarted_ThrowsInvalidOperationException [FAIL]
Assert.Throws() Failure: Exception type was not an exact match
Expected: typeof(System.InvalidOperationException)
Actual:   typeof(System.PlatformNotSupportedException)
---- System.PlatformNotSupportedException : Getting or setting the working set limits on other processes is not supported on this platform.

Reproduction Steps

Run the System.Diagnostics.Process tests and look for these failures:

System.Diagnostics.Tests.ProcessTests.MaxWorkingSet_GetNotStarted_ThrowsInvalidOperationException [FAIL]
System.Diagnostics.Tests.ProcessTests.MinWorkingSet_GetNotStarted_ThrowsInvalidOperationException [FAIL]

Expected behavior

These should pass.

Actual behavior

They fail as shown above.

Regression?

no

Known Workarounds

Disable those tests, or add this suggested fix:

commit d3d827b51af5446e207f2004463bdf426a35fdd0
Author: Gordon Ross <gordon.w.ross@gmail.com>
Date:   Sat Jul 20 15:55:40 2024 -0400

    Fix MaxWorkingSet_GetNotStarted_ThrowsInvalidOperationException

diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs
index eeac5579c18..97c0b27c730 100644
--- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs
+++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.BSD.cs
@@ -57,6 +57,8 @@ private static IntPtr ProcessorAffinityCore
         /// </summary>
         private void GetWorkingSetLimits(out IntPtr minWorkingSet, out IntPtr maxWorkingSet)
         {
+            EnsureState(State.HaveNonExitedId);
+
             // We can only do this for the current process on OS X
             if (_processId != Environment.ProcessId)
                 throw new PlatformNotSupportedException(SR.OsxExternalProcessWorkingSetNotSupported);
@@ -86,6 +88,8 @@ private void GetWorkingSetLimits(out IntPtr minWorkingSet, out IntPtr maxWorking
         /// <param name="resultingMax">The resulting maximum working set limit after any changes applied.</param>
         private void SetWorkingSetLimitsCore(IntPtr? newMin, IntPtr? newMax, out IntPtr resultingMin, out IntPtr resultingMax)
         {
+            EnsureState(State.HaveNonExitedId);
+
             // We can only do this for the current process on OS X
             if (_processId != Environment.ProcessId)
                 throw new PlatformNotSupportedException(SR.OsxExternalProcessWorkingSetNotSupported);

Configuration

Sorry for the minimal detail here, but the file involved currently affects: FreeBSD, OSX (and when I'm further along, SunOS).

Other information

No response

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process See info in area-owners.md if you want to be subscribed.

Thefrank commented 1 month ago

Is this not getting skipped correctly?

https://github.com/dotnet/runtime/blob/676189ad5e60ce6ed3337120f29470bff38bad8d/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs#L660

gwr commented 1 month ago

Is this not getting skipped correctly?

https://github.com/dotnet/runtime/blob/676189ad5e60ce6ed3337120f29470bff38bad8d/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs#L660

Maybe not? Or maybe I exposed it for the SunOS port. Anyway, with the fix, this passes for me, and should also on FreeBSD and OSX (because they all share that Process.BSD.cs file)