-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Skip cleanup of eventpipe for mono to prevent crashes on shutdown #100938
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change fix the bug?
The callstack I see in the bug report is:
#20 ep_rt_mono_fini () at /__w/1/s/src/mono/mono/eventpipe/ep-rt-mono.c:892
#21 0x0000ffff935b4d68 in mini_cleanup (domain=) at /__w/1/s/src/mono/mono/mini/mini-runtime.c:5259
#22 0x0000ffff937814ac in ves_icall_System_Environment_Exit (result=42) at /__w/1/s/src/mono/mono/metadata/icall.c:6151
The call being removed in this PR is:
ep_rt_mono_fini
ep_rt_shutdown
@noahfalk I think it's just getting inlined in to |
I believe runtime/src/mono/mono/mini/mini-runtime.c Line 5272 in eb4bf70
Since I deleted the method, we should see build errors if there are still references to the function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, just wanted to double check. Thanks!
Fix will solve issue in the case where we do System.Environment.Exit where we won't make sure managed threads are full stopped when calling mini_cleanup that in turn will shutdown EventPipe while threads can still be active in EventPipe code. Under normal shutdown this won't be a problem so an alternative fix to still keep cleanup under normal situations could be to check if we are shutting down due to a System.Environement.Exit code in ep_rt_mono_fini like we already check mono_runtime_is_shutting_down, but I'm fine either way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok, but I think it's valuable to unregister the profiler callbacks.
fixes #99609