Skip to content

Skip cleanup of eventpipe for mono to prevent crashes on shutdown - #100938

Merged
davmason merged 3 commits into
dotnet:mainfrom
davmason:mono_fix
Apr 13, 2024
Merged

Skip cleanup of eventpipe for mono to prevent crashes on shutdown#100938
davmason merged 3 commits into
dotnet:mainfrom
davmason:mono_fix

Conversation

@davmason

Copy link
Copy Markdown
Contributor

fixes #99609

@davmason davmason added this to the 9.0.0 milestone Apr 11, 2024
@davmason
davmason requested a review from a team April 11, 2024 20:37
@davmason davmason self-assigned this Apr 11, 2024

@noahfalk noahfalk left a comment

Copy link
Copy Markdown
Member

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

@davmason

Copy link
Copy Markdown
Contributor Author

@noahfalk I think it's just getting inlined in to mini_cleanup. I searched src\mono and couldn't find any more references to ep_rt_mono_fini, other than one header file I am going to push a change to delete it from.

@davmason

Copy link
Copy Markdown
Contributor Author

I believe ep_rt_shutdown is getting inlined here:

mono_component_event_pipe ()->shutdown ();

Since I deleted the method, we should see build errors if there are still references to the function

@noahfalk noahfalk left a comment

Copy link
Copy Markdown
Member

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!

@lateralusX

Copy link
Copy Markdown
Member

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.

@lambdageek lambdageek left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/mono/mono/eventpipe/ep-rt-mono.c
@davmason
davmason merged commit 4e702bc into dotnet:main Apr 13, 2024
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

4 participants