Add recursion / thread interaction guard in LogInfoForFatalError by trylek · Pull Request #46561 · dotnet/runtime

trylek · 2021-01-05T01:41:12Z

Based on local instrumentation I introduced while investigating
issues related to the switch-over to compile System.Private.CoreLib
using Crossgen2 I'm proposing to harden fatal exception handling
against nested and concurrent fatal error occurrences.

Thanks

Tomas

jkotas · 2021-01-05T01:50:13Z

src/coreclr/vm/eepolicy.cpp

This should be user-centric error message, without references to internal implementation details like LogInfoForFatalError

Thanks Jan for the suggestion. I have reformulated it to Repeated fatal crash, please let me know if you have additional suggestions on improving the message.

I would slightly prefer "Fatal error while logging another fatal error" or "Fatal error while logging fatal error". @jkotas what do you prefer?

I agree with @janvorli suggestion if we still need error message after addressing the feedback,

jkotas · 2021-01-05T16:02:38Z

src/coreclr/vm/eepolicy.cpp

Can we end up with Repeated fatal crash skipped to avoid infinite recursion and interaction between crashing threads. printed by this thread, and nothing printed by the primary crashing thread here?

Do we need to wait for the first thread to finish logging the error here, similar to what the stack overflow logging does?

I have further refined the logging and locking and I've created a simple unit test for the parallel crash case. I'm not sure whether there's any easy way to unit test the recursive case, I guess that would require somehow artificially crash the stack unwinder or something. Can you please take another look when you have a chance?

jkotas · 2021-01-05T16:10:23Z

It may be useful to test this with artificially induced crash to verify that it really works as intended.

AntonLapounov · 2021-01-06T02:35:44Z

src/tests/baseservices/exceptions/simple/ParallelCrash.cs

Interlocked.Increment should be used to avoid race condition.

Also I would move the increment below the string.Format call.

I have changed the increment operator to Interlocked.Increment in 4th commit but I left it above the string.Format call because I need the value returned from the method as the thread ID to print.

AntonLapounov · 2021-01-06T02:37:42Z

src/tests/baseservices/exceptions/simple/ParallelCrash.csproj

Nit: not necessary.

Copy & paste - removed in 4th commit, thanks for spotting.

AntonLapounov · 2021-01-06T02:47:50Z

src/tests/baseservices/exceptions/simple/ParallelCrash.cs

Since the threadMap array is not used anywhere, you could simplify to:

for (int threadIndex = ThreadCount; --threadIndex >= 0; ) { new Thread(CrashInParallel).Start(); }

Good point, thanks, removed in 4th commit.

janvorli · 2021-01-06T09:34:06Z

src/coreclr/vm/eepolicy.cpp

I would remove this message, it would just mess up the logged error details below.

Removed in 4th commit.

janvorli · 2021-01-06T10:05:50Z

src/coreclr/vm/eepolicy.cpp

We don't want to sleep forever. If this was the main thread, the application would hang. So we'll need some special value in the s_crashingThread or an extra static local to indicate that the logging has completed. If you prefer using the special value in s_crashingThread, then since the thread id can be anything, I would switch to using the Thread* instead (the result from GetCurrentThread) and use InterlockedCompareExchangeT. The special value can then be either some value that cannot be a pointer like "1", the original value with the lowest bit set to 1 or something along these lines.

I would also use much smaller sleep value so that the process exit is not visibly delayed once the logging is done. The stack overflow uses Sleep(50), so it would be nice to use the same wait here.

Fixed in 4th commit, thanks for the detailed explanation.

janvorli · 2021-01-06T10:07:26Z

src/coreclr/vm/eepolicy.cpp

Can you please change the message based on one of my suggestion?

Fixed in 4th commit.

janvorli · 2021-01-06T10:12:20Z

src/tests/baseservices/exceptions/simple/ParallelCrash.cs

It would be good to also have another test to exercise the case when the failfast comes from several secondary threads.

Well, as of the 3rd commit that's exactly what the test does - it hangs on the main thread and crashes all the worker threads in parallel. I have added two more variations - one to just crash the main thread and another to crash both the main thread and the worker threads. Please let me know if you believe we need to further improve the test coverage.

Based on local instrumentation I introduced while investigating issues related to the switch-over to compile System.Private.CoreLib using Crossgen2 I'm proposing to harden fatal exception handling against nested and concurrent fatal error occurrences. Thanks Tomas

(*) Replaced infinite wait in eepolicy with just a wait until the fatal error logging is done and reduced the wait quantum to 50 msecs; (*) Removed error message for the secondary threads entering the fatal error logging routine and reformulated the primary error per JanV's feedback; (*) Added the use of Interlocked.Increment and cleaned up the unit test per Anton's PR feedback; (*) Created three flavors of the test to (a) crash on the main thread, (b) crash on the worker threads, (c) both per JanV's PR feedback. Thanks Tomas

trylek added the area-VM-coreclr label Jan 5, 2021

trylek added this to the 6.0.0 milestone Jan 5, 2021

trylek requested review from AntonLapounov, janvorli and jkotas January 5, 2021 01:41

jkotas reviewed Jan 5, 2021

View reviewed changes

AntonLapounov reviewed Jan 6, 2021

View reviewed changes

AntonLapounov approved these changes Jan 6, 2021

View reviewed changes

janvorli reviewed Jan 6, 2021

View reviewed changes

trylek added 4 commits January 8, 2021 20:06

Modify bail-out message based on JanK's PR feedback

7a81a76

Locking other threads, better reporting, parallel crash unit test

b94f6bd

trylek force-pushed the EHRecursionGuard branch from bf438a2 to 122452e Compare January 8, 2021 20:46

Fix exit code for Linux / OSX

caf1de4

trylek merged commit af1e7ee into dotnet:master Jan 10, 2021

trylek deleted the EHRecursionGuard branch January 10, 2021 21:06

ghost locked as resolved and limited conversation to collaborators Feb 9, 2021

Conversation

trylek commented Jan 5, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

4 participants

jkotas commented Jan 5, 2021 •

edited

Loading