Conversation
…-up) Replace Lazy<T>/lock pattern with simple null-coalescing singleton. Rename Configure() to Initialize(), make ResetForTesting() public so NuGet consumers can use it in their tests.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
| /// Resets the singleton so tests can exercise <see cref="Initialize"/> on a fresh instance. | ||
| /// </summary> | ||
| internal static void ResetForTesting() | ||
| public static void ResetForTesting() |
There was a problem hiding this comment.
This should be internal
nikhilNava
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the ETW event source singleton API in Observability.Runtime to be easier for external (NuGet) consumers to configure/reset, including a rename from Configure() to Initialize() and making the test reset hook public.
Changes:
- Replace the
Lazy<T>+ lock singleton pattern with a null-coalescing singleton (_instance ??= ...). - Rename
Configure()toInitialize()(with a default parameter) and update test coverage accordingly. - Make
ResetForTesting()public and add a new test asserting double-initialization fails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Observability/Runtime/Etw/EtwEventSource.cs | Reworks singleton implementation and exposes new public initialization/reset APIs. |
| src/Tests/Microsoft.Agents.A365.Observability.Runtime.Tests/Etw/EtwEventSourceConfigureTests.cs | Updates tests for Initialize() and adds coverage for calling it twice. |
| public static void ResetForTesting() | ||
| { | ||
| lock (_lock) | ||
| { | ||
| if (_lazy.IsValueCreated) | ||
| { | ||
| _lazy.Value.Dispose(); | ||
| } | ||
|
|
||
| _throwOnEventWriteErrors = false; | ||
| _lazy = new Lazy<EtwEventSource>(CreateInstance); | ||
| } | ||
| _instance?.Dispose(); | ||
| _instance = null; | ||
| } |
There was a problem hiding this comment.
ResetForTesting() disposes and nulls _instance without synchronization. Since this is now public API, it can be called concurrently with Log/event writes or Initialize, potentially disposing an EventSource in use. Consider making it thread-safe (e.g., Interlocked.Exchange + dispose the returned instance) and/or documenting that it must only be used in single-threaded test setup/teardown.
| } | ||
| } | ||
| } | ||
| public static EtwEventSource Log => _instance ??= new EtwEventSource(); |
There was a problem hiding this comment.
Log => _instance ??= new EtwEventSource() is not thread-safe. If multiple threads access Log concurrently during startup, more than one EtwEventSource instance can be constructed; EventSource construction can throw when a duplicate name is registered, and even if it doesn't, the extra instance will be leaked/never disposed. Consider using Lazy<EtwEventSource> (thread-safe mode), LazyInitializer.EnsureInitialized, or an Interlocked.CompareExchange-based pattern to guarantee single construction.
| if (_instance != null) | ||
| { | ||
| if (_lazy.IsValueCreated) | ||
| { | ||
| throw new InvalidOperationException( | ||
| "EtwEventSource has already been created. Configure() must be called before the first access of Log."); | ||
| } | ||
|
|
||
| _throwOnEventWriteErrors = throwOnEventWriteErrors; | ||
| throw new InvalidOperationException( | ||
| "EtwEventSource has already been initialized. Initialize() must be called before the first access of Log."); | ||
| } |
There was a problem hiding this comment.
Initialize is also not synchronized with Log/other Initialize calls. Two threads can both observe _instance == null and race to create different EtwEventSource instances, or Log can create the default instance while another thread is in Initialize, causing surprising InvalidOperationExceptions. Add synchronization (lock/Interlocked) so initialization and first access are atomic and deterministic.
| public static void Initialize(bool throwOnEventWriteErrors = false) | ||
| { |
There was a problem hiding this comment.
Renaming/removing the public Configure(...) API in favor of Initialize(...) is a breaking change for existing NuGet consumers. If backward compatibility is required, consider keeping Configure(bool) as an [Obsolete] forwarding wrapper to Initialize(bool) for at least one release.
Flip Initialize() so throw-on-errors is on by default; consumers pass suppressThrowOnEventWriteErrors: true to opt out. ResetForTesting() reverted to internal visibility.
Replace Lazy/lock pattern with simple null-coalescing singleton. Rename Configure() to Initialize(), make ResetForTesting() public so NuGet consumers can use it in their tests.