Remove all kernel32.dll references and debug breakpoints #108647

VincentBu · 2024-10-08T07:45:36Z

This pull request aims at:

Removing all kernel32.dll references
Replacing all debug breakpoints with logging
Setting debugBreakOnTestHang to false for all configurations

dotnet-policy-service · 2024-10-08T07:46:18Z

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

Maoni0 · 2024-10-08T22:29:26Z

is the test hang just an artifact of the reporting, ie, if a test runs for a long time RF considers it hung? and is this only during the last phase where RF is trying to exit?

VincentBu · 2024-10-09T02:03:48Z

is the test hang just an artifact of the reporting, ie, if a test runs for a long time RF considers it hung? and is this only during the last phase where RF is trying to exit?

Yes. If there are tests uncompleted after the time limit reached, TestIsHungDebugBreak is called. It will block the thread for further actions (for example, attaching the process with a debugger).
Moko and Andrew suggest me to collect dumps directly then we can analyze them with windbg. This facilitates our work when running tests on Linux.

mrsharm · 2024-10-10T06:29:00Z

src/tests/GC/Stress/Framework/RFLogging.cs

@@ -79,7 +79,6 @@ private void LogWorker()
                }
                catch (IOException e)
                {
-                    ReliabilityFramework.MyDebugBreak(String.Format("LogWorker IOException:{0}", e.ToString()));


Let's throw the exception here so that we can take a dump

If the debugger break is set to true, we should set a debug break point here and if not, log and throw an exception.

mrsharm · 2024-10-10T06:31:19Z

src/tests/GC/Stress/finalization.config

@@ -12,7 +12,7 @@
            minimumTests="0" 
            minMaxTestUseCPUCount="true" 
            suppressConsoleOutputFromTests="true"
-            debugBreakOnTestHang="true">
+            debugBreakOnTestHang="false">


Remove as the default is false. (Same for the other configurations)

mrsharm · 2024-10-15T02:48:42Z

src/tests/GC/Stress/Framework/ReliabilityFramework.cs

@@ -882,20 +839,17 @@ private void TestSetShutdown(int totalTestsToRun)

            if (_curTestSet.DebugBreakOnTestHang)
            {
-                TestIsHungDebugBreak();
+                Console.WriteLine("Test hang.");


This should be via the logger and so should be the case below.

mrsharm · 2024-10-22T02:38:23Z

src/tests/GC/Stress/Framework/RFLogging.cs

@@ -120,7 +120,6 @@ private void LogWorker()
                }
                catch (IOException e)
                {
-                    ReliabilityFramework.MyDebugBreak(String.Format("LogWorker IOException:{0}", e.ToString()));


We probably want to throw an exception here still after logging?

Same as above: if debugger is present => break else, log and throw an exception.

mrsharm · 2024-10-22T02:39:19Z

src/tests/GC/Stress/Framework/ReliabilityFramework.cs

        }
        catch (OutOfMemoryException)
        {
-            // hang and let someone debug if we can't even break in...
-            Thread.CurrentThread.Join();
+            _logger.WriteToInstrumentationLog(_curTestSet, LoggingLevels.Tests, "Out of memory");


Should we throw here to pick up the dump on the scripts?

mrsharm · 2024-10-22T02:41:48Z

src/tests/GC/Stress/Framework/ReliabilityFramework.cs

        }
        catch (OutOfMemoryException)
        {
-            // hang and let someone debug if we can't even break in...
-            Thread.CurrentThread.Join();
+            _logger.WriteToInstrumentationLog(_curTestSet, LoggingLevels.Tests, "Out of memory");


Same logic here - if debugger present => break.

mrsharm · 2024-10-22T02:42:20Z

src/tests/GC/Stress/Framework/ReliabilityFramework.cs

@@ -780,7 +743,7 @@ private void TestStarter()
                    Thread.Sleep(250);	// give the CPU a bit of a rest if we don't need to start a new test.
                    if (_curTestSet.DebugBreakOnMissingTest && DateTime.Now.Subtract(_startTime) > minTimeToStartTest)
                    {
-                        NewTestsNotStartingDebugBreak();
+                        _logger.WriteToInstrumentationLog(_curTestSet, LoggingLevels.TestStarter, String.Format("New tests not starting"));


Add debug break conditional functionality.

mrsharm · 2024-10-29T02:50:24Z

src/tests/GC/Stress/Framework/RFLogging.cs

                    //Disk may be full so simply stop logging
+                    if (ReliabilityFramework._debugBreakOnTestHang)


Maybe moving this to it's own method parameterized with its own method e.g., void DebugBreakOrThrowException(string message).

mrsharm · 2024-10-29T02:50:38Z

src/tests/GC/Stress/Framework/ReliabilityFramework.cs

@@ -79,6 +79,23 @@ public interface ISingleReliabilityTest
    bool Run();			// returns true on success, false on failure.
 }

+public class MissingTestException : Exception


Can we mark this as a sealed class?

mrsharm

LGTM

VincentBu added 3 commits October 8, 2024 13:26

remove all kernel32.dll and debugbreak references

cb10c4b

log exception if run into oom

279b17c

remove some comments

5ca7365

dotnet-issue-labeler bot added the area-GC-coreclr label Oct 8, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 8, 2024

VincentBu requested a review from mrsharm October 8, 2024 07:46

build-analysis bot mentioned this pull request Oct 8, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

mrsharm reviewed Oct 10, 2024

View reviewed changes

VincentBu added 3 commits October 11, 2024 13:23

change debugBreakOnTestHang back to true

27a7df8

throw timeout exception when DebugBreakOnTestHang is false

2ff0273

throw exception if fail to write message to log file

182c58f

mrsharm reviewed Oct 15, 2024

View reviewed changes

VincentBu added 3 commits October 16, 2024 10:13

set debugBreakOnTestHang to false by default

83f33f1

write message to log in timeout scenario

73d33ac

print out debug break when debugBreakOnTestHang is true

b0f5afd

mrsharm reviewed Oct 22, 2024

View reviewed changes

VincentBu added 2 commits October 22, 2024 16:51

interrupt if debugbreak is presented, otherwise log and throw exception

a6f9b08

add _debugBreakOnTestHang field and MissingTestException class

e5a4a46

mrsharm reviewed Oct 29, 2024

View reviewed changes

VincentBu added 2 commits October 30, 2024 10:36

implement DebugBreakOrThrowException method

f13f6f3

implement GenerateExceptionMessageAndHandler method

9b11da6

mrsharm approved these changes Oct 31, 2024

View reviewed changes

cshung merged commit 302e0d4 into dotnet:main Nov 5, 2024
70 checks passed

VincentBu deleted the RemoveAllDebugBreakReference branch November 6, 2024 05:14

VincentBu mentioned this pull request Nov 6, 2024

fix for MissingTestException #109577

Merged

github-actions bot locked and limited conversation to collaborators Dec 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all kernel32.dll references and debug breakpoints #108647

Remove all kernel32.dll references and debug breakpoints #108647

VincentBu commented Oct 8, 2024

dotnet-policy-service bot commented Oct 8, 2024

Maoni0 commented Oct 8, 2024

VincentBu commented Oct 9, 2024

mrsharm Oct 10, 2024

mrsharm Oct 22, 2024

mrsharm Oct 10, 2024

mrsharm Oct 15, 2024

mrsharm Oct 22, 2024

mrsharm Oct 22, 2024

mrsharm Oct 22, 2024

mrsharm Oct 22, 2024

mrsharm Oct 22, 2024

mrsharm Oct 29, 2024

mrsharm Oct 29, 2024

mrsharm left a comment

		//Disk may be full so simply stop logging
		if (ReliabilityFramework._debugBreakOnTestHang)

Remove all kernel32.dll references and debug breakpoints #108647

Remove all kernel32.dll references and debug breakpoints #108647

Conversation

VincentBu commented Oct 8, 2024

dotnet-policy-service bot commented Oct 8, 2024

Maoni0 commented Oct 8, 2024

VincentBu commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrsharm left a comment

Choose a reason for hiding this comment