Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Disable parallelism in PerfCounter tests#25401

Merged
danmoseley merged 1 commit into
dotnet:masterfrom
danmoseley:tests.notparallel
Nov 21, 2017
Merged

Disable parallelism in PerfCounter tests#25401
danmoseley merged 1 commit into
dotnet:masterfrom
danmoseley:tests.notparallel

Conversation

@danmoseley
Copy link
Copy Markdown
Member

Fixes #25400 (?)

There is at least some attempt at thread safety in the performance counters API but it seems quite that the data structures are not fully protected against eg modifying categories while concurrently enumerating them or reading counters. For example, _customCategoryTable, a HashTable, is modified here without protection against concurrent write at this same location (or in other places).

It is also possible that the results of multiple registry reads (eg for categories, instances, and data) are becoming mutually inconsistent because of intervening concurrent modifications to categories, etc.

Rather than attempt to analyze the possibilities (which we might not want to attempt to "fix" anyway) let's try running the tests serially.

@stephentoub
Copy link
Copy Markdown
Member

For example, _customCategoryTable, a HashTable, is modified here without protection against concurrent write at this same location (or in other places).

Is there an issue open for that? You're right about this being busted, and the data structure could easily end up getting corrupted.

Rather than attempt to analyze the possibilities (which we might not want to attempt to "fix" anyway) let's try running the tests serially.

Assuming the issue is the lack of thread-safety in the implementation, running the tests serially is a reasonable short-term workaround, but the real fix is obviously to fix the implementation. It's also possible there are conflicts between tests, where multiple tests running in parallel are making modifications and the tests themselves aren't robust against that; for such situations, making the tests run serially might be the right answer, or else rewriting the tests to be reliable in such an environment.

@danmoseley danmoseley merged commit 6f08f5a into dotnet:master Nov 21, 2017
@danmoseley danmoseley deleted the tests.notparallel branch November 21, 2017 06:02
@danmoseley
Copy link
Copy Markdown
Member Author

@stephentoub I can open an issue, but do we expect this code to be thread safe? The docs have the usual boilerplate Any instance members are not guaranteed to be thread safe. although there are also allusions to some internal locks being taken. The locks are evidence that some attempt was made to be safe, but we probably don't want to invest in making it waterproof unless we have documented that it is.

@danmoseley
Copy link
Copy Markdown
Member Author

danmoseley commented Nov 21, 2017

The EventLog test flakiness may possibly need a similar "remedy"

@danmoseley
Copy link
Copy Markdown
Member Author

where multiple tests running in parallel are making modifications and the tests themselves aren't robust against that

This is very possible, but @adiaaida tried to avoid this, eg., by having each test use a category with its own name in it, etc..

@stephentoub
Copy link
Copy Markdown
Member

stephentoub commented Nov 21, 2017

but do we expect this code to be thread safe?

@danmosemsft, aren't these PerformanceCounterLib instances stored in a static table and published for multiple threads to all work with? At that point instance state on PerformanceCounterLib isn't really instance state, it's shared state.

@danmoseley
Copy link
Copy Markdown
Member Author

Yes, the PerformanceCounterLib you get is the only one in your process for that target machine/LCID. OK, I'll open a bug.

@karelz karelz added this to the 2.1.0 milestone Dec 4, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Category does not exist." errors in PerformanceCounter tests

3 participants