-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[release/8.0-staging] EventPipe: Don't clean up thread list on shutdown #97188
[release/8.0-staging] EventPipe: Don't clean up thread list on shutdown #97188
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.
approved. please get a code review. we will take for consideration in 8.0.x
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.
Do we need to document anything as breaking for the mono embedding scenarios?
@lateralusX can correct me here, but I don't believe so. The worst case scenario is we leak a single lock, and the way the code is written we may have leaked it anyways, since the free only happens if |
Its fine, no need to document, but I do think we should have different ways of uninitializing EventPipe at some point in the future, one that assumes process dies and that we have less control of threads (like we have now) and another that is a more strict uninitialize assuming all threads have been stopped/killed before when EventPipe gets uninitialized, in that scenario we could do proper cleanup of all resources allocated. |
Friendly reminder that Monday February 12th is the Code Complete deadline for the March Release. Please make sure to send the email to Tactics requesting approval. |
Backport of #96936 to release/8.0-staging
/cc @davmason
Customer Impact
This was reported by an internal customer, they ported their app to 8.0 and observed that there was regularly crashes on process exit. This was tracked down to a race between shutdown and threads firing events. If
_ep_threads_lock
is accessed after it is cleaned up it will cause an AV.Regression
Testing
Customer has verified the fix
Risk
Low - we are not cleaning up some resources now, which should have no impact on the process and be relatively safe.