Skip to content
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

Fix InvalidOperationException in AvTrace and improve general thread-safety #9352

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Jul 4, 2024

Fixes #9347

Description

As I believe AvTrace should be close to thread-safe class and it seems the original intention was for it to be from the comments, especially since even running during debug will have another thread initializing a source, based on the report in issue #9347, I've decided to create a little improvement.

  • We introduce a static constructor that loads the setting from registry with true/false output (false by default) in a thread-safe manner (so that internal methods that call might call into IsWpfTracingEnabledInRegistry get what they came for)
  • Each new instance initializes based on the latest fetched/saved result in s_enabledInRegistry
  • Upon calling Refresh, we reload the setting from registry and update the value in s_enabledInRegistry accordingly
  • We then use this fetched variable down the callstack to ensure the current operation has the latest value
  • IsWpfTracingEnabledInRegistry now merely retrieves the latest value for static s_enabledInRegistry
  • s_enabledInRegistry doesn't necessarily need to be volatile but I felt like the memory fence here is worth it for consistency.
  • This should improve overall performance (very minor) and stability of the AvTrace instances/refreshes in general.
  • Also fixes a possible but unlikely race-condition in Initialize function between ShouldCreateTraceSources where a different value could have been obtained from IsWpfTracingEnabledInRegistry afterwards if changed by a different thread meanwhile; if the setting in registry was changed.

It is not perfect, but it prevents the exception problem and improves the behaviour overall.

Customer Impact

This PR fixes following behavior: Ocassionally, InvalidOperationException might be thrown when setting up WPF trace sources, note that the class might be used by multiple threads by default without forcing it; e.g. when debugging.

Regression

No.

Testing

Build, local testing, verifying setting retrieval and stability.

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners July 4, 2024 18:53
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Jul 4, 2024
@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Jul 5, 2024

While digging into this; there's one race-condition I've found along the way after fixing the linked issue, which ain't a WPF issue but imperfection/bug in the thread-safety of TraceSource (which is documented to be thread-safe).

When calling PresentationTraceSources.Refresh() right at launch, after AvTrace, it calls the System.Diagnostics.Trace.Refresh() -> TraceSource.RefreshAll() -> this method locks on the static list of TraceSource references (s_tracesources), which calls Refresh on each respective TraceSource, that goes either into Initialize -> NoConfigInit_BeforeEvent, or directly ends up in OnInitializing method after init, which enumerates through its Listeners collection, and at that point you'll end up with either InvalidOperationException or NullReferenceException from the Listeners list as the access to this collection is not synchronized under the same lock. It seems to have been introduced in .NET 7, I've created an issue on dotnet/runtime - #104457.

The easiest workaround in that case is to just call first Trace.Refresh() like second after launch (or PresentationTraceSources.Refresh() in this case).

More simply speaking, using the repro project in #9347 with my PR (or without, with multiple threads but harder to catch as it usually dies on AvTrace previously), if you call Refresh right after launch and Visual Studio subscribes to the trace source or it all happens too fast from at the same time (Refresh+Init), you'll get InvalidOperationException due to TraceListenerCollection version while enumerating or NullReferenceException from different thread that still sees null.

@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Aug 17, 2024

The dotnet/runtime issue has been now fixed for .NET 9, so this now works flawlessly with my PR.

@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Oct 7, 2024

Resolved merge conflicts 05

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
2 participants