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.

Sign up for GitHub

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

Try to make ProcessNameMatchesScriptName test more reliable #46292

Closed
wants to merge 4 commits into from
Closed

Try to make ProcessNameMatchesScriptName test more reliable #46292

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10b055b
use simpler name to avoid issues with special characters
adamsitnik Dec 21, 2020
ad512a3
use full path to avoid potential alias issues
adamsitnik Dec 21, 2020
11c2197
handle potential zombie process
adamsitnik Dec 21, 2020
81d97c3
add comments
adamsitnik Dec 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,9 @@ public void ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled(bool
[PlatformSpecific(~TestPlatforms.OSX)] // On OSX, ProcessName returns the script interpreter.
public void ProcessNameMatchesScriptName()
{
string scriptName = GetTestFileName();
const string scriptName = nameof(ProcessNameMatchesScriptName);
Copy link
Member

Choose a reason for hiding this comment

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

Unless the underlying filesystem imposes additional restrictions, Linux filenames can contain any bytes except \0 and /.
.NET assumes the bytes are UTF-8.
So, I think the filename is not an issue. If it were, we should make sure GetTestFileName returns a valid filename on Linux instead of doing something specific in this test.

Copy link
Member

Choose a reason for hiding this comment

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

I agree I do'nt see a problem with GetTestFileName(), and if it has a problem, it will be affecting other tests. Either way no change is needed here.

string filename = Path.Combine(TestDirectory, scriptName);
File.WriteAllText(filename, $"#!/bin/sh\nsleep 600\n"); // sleep 10 min.
File.WriteAllText(filename, $"#!/bin/sh\n/bin/sleep 600\n"); // sleep 10 min.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for changing the to explicitly using /bin/sleep? I don't think we care how the sleep is implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not a Linux expert, but I guess that sleep 600 could become a zombie right away only if there is a custom alias for the sleep command. I don't know if it's possible, but I think that we can just use full path to be 100% sure we are calling the actual /bin/sleep.

Copy link
Member

Choose a reason for hiding this comment

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

I know on Alpine it's busybox, not an actual executable file, but it's symlinked to the same place so I guess that's OK:

LAPTOP-P6UJDVTA:~$ ls -l /bin/sleep
lrwxrwxrwx    1 root     root            12 Oct 21 09:23 /bin/sleep -> /bin/busybox

ChMod(filename, "744"); // set x-bit

using (var process = Process.Start(new ProcessStartInfo { FileName = filename }))
Expand All @@ -171,14 +171,26 @@ public void ProcessNameMatchesScriptName()
string stat = File.ReadAllText($"/proc/{process.Id}/stat");
Assert.Contains($"({scriptName.Substring(0, 15)})", stat);
string cmdline = File.ReadAllText($"/proc/{process.Id}/cmdline");
Assert.Equal($"/bin/sh\0{filename}\0", cmdline);

Assert.Equal(scriptName, process.ProcessName);
// cmdLine can sometimes be null: https://github.com/dotnet/runtime/issues/13757
// according to docs, it's when the process is a zombie: https://man7.org/linux/man-pages/man5/proc.5.html
Copy link
Member

Choose a reason for hiding this comment

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

The Assert that fails was added to understand better why ProcessName doesn't end up with the proper value. It verifies some expectations of the .NET implementation.

Can we verify the failing case has a zombie process? We can check /stat for the Z flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point, will do

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it a bit more.

Probably this test fails when the Assert happens after the script has already exited. .NET will have reaped the process, and everything under /proc is gone.

This happens when sleep doesn't work, or when the asserts are delayed for 10 min. We can safely increase the 10 min timeout. And add an assert to check !process.HasExited.

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens when sleep doesn't work

How would this be possible?

when the asserts are delayed for 10 min

this is unlikely, as we just start the process and execute asserts right away

everything under /proc is gone

Based on the CI logs it looks like not everything is gone. The previous line that asserts the content of /proc/{process.Id}/stat works just fine. Based on the available docs the only explanation I was able to find was the zombie process.

if (string.IsNullOrEmpty(cmdline))
{
Assert.True(process.HasExited, $"/proc/{process.Id}/cmdline was empty, but the process was not a zombie");
}
else
{
Assert.Equal($"/bin/sh\0{filename}\0", cmdline);
Assert.Equal(scriptName, process.ProcessName);
}
}
finally
{
process.Kill();
process.WaitForExit();
if (!process.HasExited)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add this condition?

{
process.Kill();
process.WaitForExit();
}
}
}
}
Expand Down