Skip to content

Fix Crst level violation error on SendMethodDetails event #36932

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

Merged
merged 6 commits into from
Jun 3, 2020

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented May 23, 2020

Another attempt at fixing #643 since the old PR is quite outdated and have merge conflicts to deal with.

This PR fixes AddTypeToGlobalCacheIfNotExists by pulling out the constructors of LoggedTypesFromModule and TypeLoggingInfo out of the scope of EtwTypeLogHash Crst.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about the ETW keyword check, otherwise LGTM!

// Check if ETW is enabled, and if not, bail here.
// We do this inside of the lock to ensure that we don't immediately
// re-allocate the global type hash after it has been cleaned up.
if (!ETW_TRACING_CATEGORY_ENABLED(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is safe to leave this inside the lock where it was. Did you observe a problem that was causing?

@janvorli
Copy link
Member

janvorli commented Jun 2, 2020

I've verified that this PR fixes the problem for me.

@sywhang sywhang merged commit 8deadd2 into dotnet:master Jun 3, 2020
@davmason davmason mentioned this pull request Jul 7, 2020
davmason added a commit that referenced this pull request Jul 9, 2020
#36932 had a typo that caused types to never be logged
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants