Repository navigation
Replace ConcurrentBag with List for unload handlers registry - #2010
Conversation
Replaces ConcurrentBag<EventHandler> with List<EventHandler> for module unload handler registry in Coverlet's in-process data collection. All registry accesses are now protected by locking for thread safety. Updates CoverletInProcessHandler, CoverletInProcDataCollector, ModuleTrackerTemplate, and related tests to use List. Adds Issue2009NetFrameworkRegressionTests to verify .NET Framework scenarios do not throw TypeLoadException for missing ConcurrentBag, resolving issue coverlet-coverage#2009.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #2009 by removing the ConcurrentBag<EventHandler>-based unload handler registry from the in-process coverage path (which can trigger TypeLoadException on some .NET Framework scenarios) and replacing it with a List<EventHandler> guarded by locks.
Changes:
- Replaced the AppDomain unload-handler registry from
ConcurrentBag<EventHandler>toList<EventHandler>across the in-proc collector and MTP in-process handler, adding locking + snapshotting for safe enumeration. - Updated affected unit tests to use
List<EventHandler>and to lock when mutating/reading the registry. - Added a new .NET Framework regression validation test for issue #2009.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/coverlet.MTP.validation.tests/Issue2009NetFrameworkRegressionTests.cs | Adds a Windows-only net472 validation regression test for the ConcurrentBag TypeLoadException scenario. |
| test/coverlet.MTP.tests/InProcess/CoverletInProcessHandlerTests.cs | Updates in-process handler tests to register unload handlers using List<EventHandler>. |
| test/coverlet.core.coverage.tests/Instrumentation/ModuleTrackerTemplateTests.cs | Updates tracker template tests to use a locked List<EventHandler> registry. |
| test/coverlet.collector.tests/CoverletInProcDataCollectorTests.cs | Updates in-proc collector tests to add handlers to a locked List<EventHandler> registry. |
| src/legacy/coverlet.collector/InProcDataCollection/CoverletInProcDataCollector.cs | Switches the in-proc collector registry to List<EventHandler> with lock + snapshot during flush. |
| src/coverlet.MTP/InProcess/CoverletInProcessHandler.cs | Switches the MTP in-process registry to List<EventHandler> with lock + snapshot during flush. |
| src/coverlet.core/Instrumentation/ModuleTrackerTemplate.cs | Switches tracker-side registry population to List<EventHandler> with locking. |
| EventHandler[] handlersSnapshot; | ||
| lock (registeredHandlers) | ||
| { | ||
| handlersSnapshot = [.. registeredHandlers]; |
There was a problem hiding this comment.
@Bertk I haven't profiled, but I wonder how expensive it is to make a copy each time. If ConcurrentBag<T> is unavailable in .NET Framework, could it instead use only the keys of a ConcurrentDictionary<TKey, TValue>?
There was a problem hiding this comment.
I also did not profile it but asked copilot for a general performance impact.
Short answer: likely low in practice for this specific path.
Why:
• handlersSnapshot = [.. registeredHandlers] is O(n) and allocates one array, but this runs in OnTestSessionFinishingAsync (session-end), not per hit/per test assertion.
• Typical n (instrumented assemblies) is usually small enough that one copy is negligible.
• The copy is also a correct lock-contention tradeoff: it keeps the lock short and avoids invoking handlers while holding the lock.What could hurt (if scale is extreme):
• Very large handler counts (thousands+) -> larger allocation, possible LOH allocation.
• High parallel registration while finishing -> some lock contention, but still bounded.
About ConcurrentDictionary<TKey, TValue> keys idea:
• It can remove explicit locking for add/enumeration, but it does not eliminate all costs (enumeration/snapshot >still has overhead).
• It also requires cross-cutting changes (ModuleTrackerTemplate.RegisterUnloadEvents currently assumes List in AppDomain data).Other concrete perf nits in this file:
Debug log interpolation allocates strings even when debug logging is off
• Impact: avoidable allocations in startup/finish/loop.
• Alternative: guard with logger-level checks (if available) or use non-interpolated logging pattern supported by this logger API.Per-handler reflection for assemblyName
• Impact: minor extra CPU in loop.
• Alternative: compute only when needed for logging/error paths (or cache if reused).Net: this snapshot copy is probably not a significant degradation unless you have unusually high assembly counts or frequent session-end executions.
I'm currently focused on the MTP 2.4.0 updates, so I'd like to ask for a short postponement on the performance investigation, if that's okay with everyone.
Replaces ConcurrentBag with List for module unload handler registry in Coverlet's in-process data collection. All registry accesses are now protected by locking for thread safety. Updates CoverletInProcessHandler, CoverletInProcDataCollector, ModuleTrackerTemplate, and related tests to use List. Adds Issue2009NetFrameworkRegressionTests to verify .NET Framework scenarios do not throw TypeLoadException for missing ConcurrentBag, resolving issue #2009.