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

ProcessThreadTests.TestStartTimeProperty - Get spawned thread by ID from Process.Threads to avoid mismatch #104972

Merged
merged 6 commits into from
Jul 22, 2024
Merged

ProcessThreadTests.TestStartTimeProperty - Get spawned thread by ID from Process.Threads to avoid mismatch #104972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3092d86
Increasing the tolerance for Mono Runtime for the flaky ProcessThread…
adamsitnik Jul 16, 2024
6d17edc
select the thread based on current thread native id, remove the try c…
adamsitnik Jul 17, 2024
2cc7ecb
Revert "Increasing the tolerance for Mono Runtime for the flaky Proce…
adamsitnik Jul 17, 2024
d9350ff
fix the Linux build: gettid is not exposed in older versions of libc
adamsitnik Jul 17, 2024
0de6947
Make the test conditional
adamsitnik Jul 18, 2024
dabec4f
get SYS_gettid value specific to given architecture (and hopefully fi…
adamsitnik Jul 19, 2024
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
22 changes: 22 additions & 0 deletions src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.ComponentModel;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;
Expand Down Expand Up @@ -30,5 +31,26 @@ public void TestPriorityLevelProperty_Unix()

Assert.Throws<PlatformNotSupportedException>(() => thread.PriorityLevel = level);
}

private static int GetCurrentThreadId()
{
// The magic values come from https://github.com/torvalds/linux.
int SYS_gettid = RuntimeInformation.ProcessArchitecture switch
{
Architecture.Arm => 224,
Architecture.Arm64 => 178,
Architecture.X86 => 224,
Architecture.X64 => 186,
Architecture.S390x => 236,
Architecture.Ppc64le => 207,
Architecture.RiscV64 => 178,
_ => 178,
};

return syscall(SYS_gettid);
}

[DllImport("libc")]
private static extern int syscall(int nr);
}
}
21 changes: 10 additions & 11 deletions src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.DotNet.RemoteExecutor;
using Xunit;
using System.Threading.Tasks;
using System.Runtime.InteropServices;

namespace System.Diagnostics.Tests
{
Expand Down Expand Up @@ -107,8 +108,8 @@ public void TestStartTimeProperty_OSX()
}
}

[Fact]
[PlatformSpecific(TestPlatforms.Linux|TestPlatforms.Windows)] // OSX and FreeBSD throw PNSE from StartTime
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[PlatformSpecific(TestPlatforms.Linux | TestPlatforms.Windows)] // OSX and FreeBSD throw PNSE from StartTime
public async Task TestStartTimeProperty()
{
TimeSpan allowedWindow = TimeSpan.FromSeconds(2);
Expand Down Expand Up @@ -148,15 +149,13 @@ public async Task TestStartTimeProperty()
await Task.Factory.StartNew(() =>
{
p.Refresh();
try
{
var newest = p.Threads.Cast<ProcessThread>().OrderBy(t => t.StartTime.ToUniversalTime()).Last();
Copy link
Member

Choose a reason for hiding this comment

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

Was it wrongly assumed that Process.Threads contained the threads ordered with the most recently created as the Last() one?

Is also possible that the property does have the threads ordered but StartNew wasn't creating a new thread but reusing an existing one as Dan mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was it wrongly assumed that Process.Threads contained the threads ordered with the most recently created as the Last() one?

There are other tests running in parallel and each of them can create a new thread. Even if we disable that, the ThreadPool can still start a new thread on it's own. So to get 100% deterministic behavior we should IMO perform an ID match.

Copy link
Member Author

Choose a reason for hiding this comment

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

StartNew wasn't creating a new thread but reusing an existing one as Dan mentioned.

In theory CLR and Mono are sharing the same thread pool implementation, so I would expect TaskCreationOptions.LongRunning to always create a new thread, but I might be wrong.

@kouvel is it safe to assume that Task.Factory.StartNew(/**/, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); executed from an xUnit test runner will always spawn a new thread?

Copy link
Member

Choose a reason for hiding this comment

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

Based on this:

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ThreadPoolTaskScheduler.cs

Lines 45 to 53 in 27776e2

if (Thread.IsThreadStartSupported && (options & TaskCreationOptions.LongRunning) != 0)
{
// Run LongRunning tasks on their own dedicated thread.
new Thread(s_longRunningThreadWork)
{
IsBackground = true,
Name = ".NET Long Running Task"
}.UnsafeStart(task);
}

It looks like when thread start is supported, a new thread is created for each long-running task. The threads used for long-running tasks are not pooled currently, though they could be. Probably creating a new thread would be more explicit in intent.

Wonder if the failure is happening when thread start is not supported. In the threading tests that create new threads, the following condition is used, which appears to be the same condition as Thread.IsThreadStartSupported:
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]

Copy link
Member

@kouvel kouvel Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Although using a long-running task would avoid an unhandled exception/crash on assertion failure in the new thread. ThreadTestHelpers.CreateGuardedThread could be used for that instead, as in:

runtime/src/libraries/System.Threading.Thread/tests/ThreadTests.cs

Line 621 in 27776e2

ThreadTestHelpers.CreateGuardedThread(out waitForThread, () =>

Copy link
Member Author

Choose a reason for hiding this comment

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

@kouvel thank you!

Copy link
Member

Choose a reason for hiding this comment

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

The threads used for long-running tasks are not pooled currently, though they could be.

TIL - I should have checked the code. @kouvel is that worth an issue, as a suggestion? Basically pull a thread from the pool if available, and trigger a replacement to start. I have no idea.

Copy link
Member

Choose a reason for hiding this comment

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

What's done currently is simple and may be sufficient if most long-running work items rarely or never complete. If there's a need where work items would complete frequently enough, and are not short enough and need to be classified as long-running, then it may be worthwhile to pool those threads.

Assert.InRange(newest.StartTime.ToUniversalTime(), curTime - allowedWindow, DateTime.Now.ToUniversalTime() + allowedWindow);
}
catch (InvalidOperationException)
{
// A thread may have gone away between our getting its info and attempting to access its StartTime
}

int newThreadId = GetCurrentThreadId();

ProcessThread[] processThreads = p.Threads.Cast<ProcessThread>().ToArray();
ProcessThread newThread = Assert.Single(processThreads, thread => thread.Id == newThreadId);

Assert.InRange(newThread.StartTime.ToUniversalTime(), curTime - allowedWindow, DateTime.Now.ToUniversalTime() + allowedWindow);
Copy link
Member

Choose a reason for hiding this comment

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

Now that we get the correct thread, could we get rid of the tolerance?

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've reverted the increase of the tolerance. I think it's safer to keep the current one (I am just afraid it would make the test even more flaky)

Copy link
Member

Choose a reason for hiding this comment

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

I think with your fix the test won't be flaky at all 😆 but I might be wrong.

}, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);
}
}
Expand Down
Loading