Skip to content

Make PortableThreadPool to check events correctly before firing them#45666

Merged
sywhang merged 1 commit intodotnet:masterfrom
sywhang:dev/suwhang/45660
Dec 7, 2020
Merged

Make PortableThreadPool to check events correctly before firing them#45666
sywhang merged 1 commit intodotnet:masterfrom
sywhang:dev/suwhang/45660

Conversation

@sywhang
Copy link
Contributor

@sywhang sywhang commented Dec 7, 2020

PortableThreadPool is firing several verbose events without checking whether they are enabled properly. Specifically they need to be checked with the proper EventLevel and EventKeywords but they were only being checked via the default IsEnabled() method which will return true whenever any event under NativeRuntimeEventSource is enabled.

This should also fix #45660.

@ghost
Copy link

ghost commented Dec 7, 2020

Hello @sywhang!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

LGTM

@sywhang
Copy link
Contributor Author

sywhang commented Dec 7, 2020

Test failure is a known issue: #45654

@sywhang sywhang merged commit 83d19de into dotnet:master Dec 7, 2020
@kouvel
Copy link
Contributor

kouvel commented Dec 7, 2020

Thanks for finding this, I have a follow-up PR in #45681, please take a look

kouvel added a commit that referenced this pull request Dec 7, 2020
* Revert "Make PortableThreadPool to check events correctly before firing them (#45666)"

This reverts commit 83d19de.

* Add keyword and verbosity checks for events

Reverted the part of e8043ff regarding `IsEnabled` checks
@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2021
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.

BinderTracingTest.Basic timeout

4 participants