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

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

Open
gwr opened this issue Jul 24, 2024 · 3 comments · May be fixed by #105424
Open
Labels
area-System.Diagnostics.Process in-pr There is an active PR which will close this issue when it is merged needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@gwr
Copy link
Contributor

gwr commented Jul 24, 2024

Description

In the SunOS port I'm working on (See #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 dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 24, 2024
Copy link
Contributor

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

@Thefrank
Copy link
Contributor

Is this not getting skipped correctly?

[SkipOnPlatform(TestPlatforms.OSX | TestPlatforms.FreeBSD | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Getting MinWorkingSet is not supported on OSX, BSD, iOS, MacCatalyst, and tvOS.")]

@gwr
Copy link
Contributor Author

gwr commented Jul 25, 2024

Is this not getting skipped correctly?

[SkipOnPlatform(TestPlatforms.OSX | TestPlatforms.FreeBSD | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Getting MinWorkingSet is not supported on OSX, BSD, iOS, MacCatalyst, and tvOS.")]

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)

gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 25, 2024
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 25, 2024
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 27, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Comment out those tests for now and add notes.

Needs testing on the affected platforms and then either
update those commented out tests or just remove them.
@jeffhandley jeffhandley added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 27, 2024
@jeffhandley jeffhandley added this to the Future milestone Jul 27, 2024
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 27, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Comment out those tests for now and add notes.

Needs testing on the affected platforms and then either
update those commented out tests or just remove them.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 27, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Comment out those tests for now and add notes.

Needs testing on the affected platforms and then either
update those commented out tests or just remove them.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 27, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Comment out those tests for now and add notes.

Needs testing on the affected platforms and then either
update those commented out tests or just remove them.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 27, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Comment out those tests for now and add notes.

Needs testing on the affected platforms and then either
update those commented out tests or just remove them.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 28, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Comment out those tests for now and add notes.

Needs testing on the affected platforms and then either
update those commented out tests or just remove them.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 28, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (pretty much any port using ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

There are tests that verify the old (less correct) behavior of
the get/set WorkingSet methods from before this fix.
Still trying to get those sorted out...
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 29, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 29, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Jul 30, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 18, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 18, 2024
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 18, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 18, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 21, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 21, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 21, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 21, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 21, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
gwr added a commit to gwr/dotnet-runtime that referenced this issue Aug 22, 2024
Fixes issue dotnet#105422

On MacOS, FreeBSD, SunOS (the ports that share ProcessBSD.c)
the get/set WorkingSet methods only work on the current process.
Skip the parts of tests that operate on other processes.

Remove now redundant MacOS-speicifc tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Diagnostics.Process in-pr There is an active PR which will close this issue when it is merged needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants