ProcessThreadTests.TestStartTimeProperty - Get spawned thread by ID from Process.Threads to avoid mismatch by adamsitnik · Pull Request #104972 · dotnet/runtime

adamsitnik · 2024-07-16T15:34:38Z

…Tests.TestStartTimeProperty test, fixes dotnet#103448

dotnet-policy-service · 2024-07-16T15:35:02Z

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

danmoseley · 2024-07-17T02:28:11Z

Although, I think @jozkee has a point in his comment in the issue, that tolerating such wrong timestamps is probably masking a real bug rather than just accommodating test noise.

…atch block: - it can hide LINQ issues as LINQ may throw InvalidOperationException - it's not needed for the current thread, because the code is still running so we know the thread is alive

…ssThreadTests.TestStartTimeProperty test, fixes dotnet#103448" This reverts commit 3092d86.

danmoseley · 2024-07-17T14:44:30Z

Hmm, is the likely reason for the failure that Task.Factory.StartNew(() => cannot be assumed to start a new thread, even with TaskCreationOptions.LongRunning ? If that's the case, I don't think your change will fix that problem. The fix would be to actually create a new thread (?)

src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.cs

jozkee · 2024-07-17T14:57:06Z

src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.cs

                    p.Refresh();
-                    try
-                    {
-                        var newest = p.Threads.Cast<ProcessThread>().OrderBy(t => t.StartTime.ToUniversalTime()).Last();


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.

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.

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?

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))]

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, () =>

@kouvel thank you!

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.

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.

jozkee

LGTM, assuming CI passes, thanks!

jozkee · 2024-07-18T14:58:30Z

src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.cs

+                    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);


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

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)

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

…x the build for arm64)

Increasing the tolerance for Mono Runtime for the flaky ProcessThread…

3092d86

…Tests.TestStartTimeProperty test, fixes dotnet#103448

adamsitnik added area-System.Diagnostics.Process test-enhancement Improvements of test source code runtime-mono specific to the Mono runtime labels Jul 16, 2024

adamsitnik added this to the 9.0.0 milestone Jul 16, 2024

adamsitnik requested review from danmoseley and jozkee July 16, 2024 15:34

dotnet-policy-service bot assigned adamsitnik Jul 16, 2024

build-analysis bot mentioned this pull request Jul 16, 2024

[browser] RuntimeError: memory access out of bounds #96546

Closed

lewing approved these changes Jul 16, 2024

View reviewed changes

danmoseley approved these changes Jul 17, 2024

View reviewed changes

adamsitnik added 2 commits July 17, 2024 14:42

select the thread based on current thread native id, remove the try c…

6d17edc

…atch block: - it can hide LINQ issues as LINQ may throw InvalidOperationException - it's not needed for the current thread, because the code is still running so we know the thread is alive

Revert "Increasing the tolerance for Mono Runtime for the flaky Proce…

2cc7ecb

…ssThreadTests.TestStartTimeProperty test, fixes dotnet#103448" This reverts commit 3092d86.

adamsitnik mentioned this pull request Jul 17, 2024

ProcessThreadTests.TestStartTimeProperty failed in CI #103448

Closed

jozkee reviewed Jul 17, 2024

View reviewed changes

src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.cs Outdated Show resolved Hide resolved

src/libraries/System.Diagnostics.Process/tests/ProcessThreadTests.cs Outdated Show resolved Hide resolved

jozkee reviewed Jul 17, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jul 17, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Closed

adamsitnik and others added 2 commits July 17, 2024 17:46

fix the Linux build: gettid is not exposed in older versions of libc

d9350ff

Make the test conditional

0de6947

jozkee approved these changes Jul 18, 2024

View reviewed changes

danmoseley approved these changes Jul 18, 2024

View reviewed changes

jozkee changed the title ~~Increasing the tolerance for Mono Runtime for the flaky ProcessThreadTests.TestStartTimeProperty test~~ ProcessThreadTests.TestStartTimeProperty - Get spawned thread by ID from Process.Threads to avoid mismatch Jul 19, 2024

adamsitnik force-pushed the issue103448 branch from ee31b4e to c126dd5 Compare July 19, 2024 12:33

get SYS_gettid value specific to given architecture (and hopefully fi…

dabec4f

…x the build for arm64)

adamsitnik force-pushed the issue103448 branch from c126dd5 to dabec4f Compare July 19, 2024 12:37

This was referenced Jul 19, 2024

nativeaot\\SmokeTests\\Reflection\\Reflection\\Reflection.cmd fails with Access Violation #105136

Closed

System.IO.Tests.Directory_Create_Tests.FileSystemWatcher_Directory_Create_InNestedDirectory fails with "Created event did not occur as expected" #103630

Open

build-analysis bot mentioned this pull request Jul 19, 2024

'System.Net.NameResolution.Functional.Tests' Failure #105092

Closed

This was referenced Jul 19, 2024

consider pulling long running threatpool threads from the pool #105165

Open

Delete dead Interop.Gdi32.* code #105209

Merged

adamsitnik merged commit 7c3286f into dotnet:main Jul 22, 2024

jozkee mentioned this pull request Jul 29, 2024

ProcessThreadTests.TestStartTimeProperty failure in CI #105526

Closed

github-actions bot locked and limited conversation to collaborators Aug 22, 2024

	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);
	}

Conversation

adamsitnik commented Jul 16, 2024

Uh oh!

dotnet-policy-service bot commented Jul 16, 2024

Uh oh!

danmoseley commented Jul 17, 2024

Uh oh!

danmoseley commented Jul 17, 2024

Uh oh!

Uh oh!

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!

kouvel Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jozkee left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kouvel Jul 17, 2024 •

edited

Loading