From 451afdad8b54de4a187f8a8b77a77c25b2912312 Mon Sep 17 00:00:00 2001 From: Daniel Ramos Date: Mon, 10 Oct 2022 17:58:47 -0700 Subject: [PATCH 1/3] Initial fix for null reference exception --- .../Diagnostics/Metrics/MetricsEventSource.cs | 79 +++++++++---------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs index 403ba0bb09548..84e8b6d373d75 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs @@ -60,12 +60,7 @@ public static class Keywords public const EventKeywords InstrumentPublishing = (EventKeywords)0x4; } - private CommandHandler _handler; - - private MetricsEventSource() - { - _handler = new CommandHandler(); - } + private Lazy _handler = new Lazy(); /// /// Used to send ad-hoc diagnostics to humans. @@ -189,7 +184,7 @@ protected override void OnEventCommand(EventCommandEventArgs command) { lock (this) { - _handler.OnEventCommand(command); + _handler.Value.OnEventCommand(command, this); } } @@ -202,7 +197,7 @@ private sealed class CommandHandler private AggregationManager? _aggregationManager; private string _sessionId = ""; - public void OnEventCommand(EventCommandEventArgs command) + public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource parent) { try { @@ -215,7 +210,7 @@ public void OnEventCommand(EventCommandEventArgs command) // This limitation shouldn't really matter because browser also doesn't support out-of-proc EventSource communication // which is the intended scenario for this EventSource. If it matters in the future AggregationManager can be // modified to have some other fallback path that works for browser. - Log.Error("", "System.Diagnostics.Metrics EventSource not supported on browser"); + parent.Error("", "System.Diagnostics.Metrics EventSource not supported on browser"); return; } #endif @@ -233,13 +228,13 @@ public void OnEventCommand(EventCommandEventArgs command) // one other listener is active. In the future we might be able to figure out how // to infer the changes from the info we do have or add a better API but for now // I am taking the simple route and not supporting it. - Log.MultipleSessionsNotSupportedError(_sessionId); + parent.MultipleSessionsNotSupportedError(_sessionId); return; } _aggregationManager.Dispose(); _aggregationManager = null; - Log.Message($"Previous session with id {_sessionId} is stopped"); + parent.Message($"Previous session with id {_sessionId} is stopped"); } _sessionId = ""; } @@ -249,12 +244,12 @@ public void OnEventCommand(EventCommandEventArgs command) if (command.Arguments!.TryGetValue("SessionId", out string? id)) { _sessionId = id!; - Log.Message($"SessionId argument received: {_sessionId}"); + parent.Message($"SessionId argument received: {_sessionId}"); } else { _sessionId = System.Guid.NewGuid().ToString(); - Log.Message($"New session started. SessionId auto-generated: {_sessionId}"); + parent.Message($"New session started. SessionId auto-generated: {_sessionId}"); } @@ -263,21 +258,21 @@ public void OnEventCommand(EventCommandEventArgs command) double refreshIntervalSecs; if (command.Arguments!.TryGetValue("RefreshInterval", out string? refreshInterval)) { - Log.Message($"RefreshInterval argument received: {refreshInterval}"); + parent.Message($"RefreshInterval argument received: {refreshInterval}"); if (!double.TryParse(refreshInterval, out refreshIntervalSecs)) { - Log.Message($"Failed to parse RefreshInterval. Using default {defaultIntervalSecs}s."); + parent.Message($"Failed to parse RefreshInterval. Using default {defaultIntervalSecs}s."); refreshIntervalSecs = defaultIntervalSecs; } else if (refreshIntervalSecs < AggregationManager.MinCollectionTimeSecs) { - Log.Message($"RefreshInterval too small. Using minimum interval {AggregationManager.MinCollectionTimeSecs} seconds."); + parent.Message($"RefreshInterval too small. Using minimum interval {AggregationManager.MinCollectionTimeSecs} seconds."); refreshIntervalSecs = AggregationManager.MinCollectionTimeSecs; } } else { - Log.Message($"No RefreshInterval argument received. Using default {defaultIntervalSecs}s."); + parent.Message($"No RefreshInterval argument received. Using default {defaultIntervalSecs}s."); refreshIntervalSecs = defaultIntervalSecs; } @@ -285,16 +280,16 @@ public void OnEventCommand(EventCommandEventArgs command) int maxTimeSeries; if (command.Arguments!.TryGetValue("MaxTimeSeries", out string? maxTimeSeriesString)) { - Log.Message($"MaxTimeSeries argument received: {maxTimeSeriesString}"); + parent.Message($"MaxTimeSeries argument received: {maxTimeSeriesString}"); if (!int.TryParse(maxTimeSeriesString, out maxTimeSeries)) { - Log.Message($"Failed to parse MaxTimeSeries. Using default {defaultMaxTimeSeries}"); + parent.Message($"Failed to parse MaxTimeSeries. Using default {defaultMaxTimeSeries}"); maxTimeSeries = defaultMaxTimeSeries; } } else { - Log.Message($"No MaxTimeSeries argument received. Using default {defaultMaxTimeSeries}"); + parent.Message($"No MaxTimeSeries argument received. Using default {defaultMaxTimeSeries}"); maxTimeSeries = defaultMaxTimeSeries; } @@ -302,16 +297,16 @@ public void OnEventCommand(EventCommandEventArgs command) int maxHistograms; if (command.Arguments!.TryGetValue("MaxHistograms", out string? maxHistogramsString)) { - Log.Message($"MaxHistograms argument received: {maxHistogramsString}"); + parent.Message($"MaxHistograms argument received: {maxHistogramsString}"); if (!int.TryParse(maxHistogramsString, out maxHistograms)) { - Log.Message($"Failed to parse MaxHistograms. Using default {defaultMaxHistograms}"); + parent.Message($"Failed to parse MaxHistograms. Using default {defaultMaxHistograms}"); maxHistograms = defaultMaxHistograms; } } else { - Log.Message($"No MaxHistogram argument received. Using default {defaultMaxHistograms}"); + parent.Message($"No MaxHistogram argument received. Using default {defaultMaxHistograms}"); maxHistograms = defaultMaxHistograms; } @@ -320,41 +315,41 @@ public void OnEventCommand(EventCommandEventArgs command) maxTimeSeries, maxHistograms, (i, s) => TransmitMetricValue(i, s, sessionId), - (startIntervalTime, endIntervalTime) => Log.CollectionStart(sessionId, startIntervalTime, endIntervalTime), - (startIntervalTime, endIntervalTime) => Log.CollectionStop(sessionId, startIntervalTime, endIntervalTime), - i => Log.BeginInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), - i => Log.EndInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), - i => Log.InstrumentPublished(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), - () => Log.InitialInstrumentEnumerationComplete(sessionId), - e => Log.Error(sessionId, e.ToString()), - () => Log.TimeSeriesLimitReached(sessionId), - () => Log.HistogramLimitReached(sessionId), - e => Log.ObservableInstrumentCallbackError(sessionId, e.ToString())); + (startIntervalTime, endIntervalTime) => parent.CollectionStart(sessionId, startIntervalTime, endIntervalTime), + (startIntervalTime, endIntervalTime) => parent.CollectionStop(sessionId, startIntervalTime, endIntervalTime), + i => parent.BeginInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), + i => parent.EndInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), + i => parent.InstrumentPublished(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), + () => parent.InitialInstrumentEnumerationComplete(sessionId), + e => parent.Error(sessionId, e.ToString()), + () => parent.TimeSeriesLimitReached(sessionId), + () => parent.HistogramLimitReached(sessionId), + e => parent.ObservableInstrumentCallbackError(sessionId, e.ToString())); _aggregationManager.SetCollectionPeriod(TimeSpan.FromSeconds(refreshIntervalSecs)); if (command.Arguments!.TryGetValue("Metrics", out string? metricsSpecs)) { - Log.Message($"Metrics argument received: {metricsSpecs}"); - ParseSpecs(metricsSpecs); + parent.Message($"Metrics argument received: {metricsSpecs}"); + ParseSpecs(metricsSpecs, parent); } else { - Log.Message("No Metrics argument received"); + parent.Message("No Metrics argument received"); } _aggregationManager.Start(); } } - catch (Exception e) when (LogError(e)) + catch (Exception e) when (LogError(e, parent)) { // this will never run } } - private bool LogError(Exception e) + private bool LogError(Exception e, MetricsEventSource parent) { - Log.Error(_sessionId, e.ToString()); + parent.Error(_sessionId, e.ToString()); // this code runs as an exception filter // returning false ensures the catch handler isn't run return false; @@ -363,7 +358,7 @@ private bool LogError(Exception e) private static readonly char[] s_instrumentSeparators = new char[] { '\r', '\n', ',', ';' }; [UnsupportedOSPlatform("browser")] - private void ParseSpecs(string? metricsSpecs) + private void ParseSpecs(string? metricsSpecs, MetricsEventSource parent) { if (metricsSpecs == null) { @@ -374,11 +369,11 @@ private void ParseSpecs(string? metricsSpecs) { if (!MetricSpec.TryParse(specString, out MetricSpec spec)) { - Log.Message($"Failed to parse metric spec: {specString}"); + parent.Message($"Failed to parse metric spec: {specString}"); } else { - Log.Message($"Parsed metric: {spec}"); + parent.Message($"Parsed metric: {spec}"); if (spec.InstrumentName != null) { _aggregationManager!.Include(spec.MeterName, spec.InstrumentName); From 7677f3791708b92f61faaa210f6f7e6a1e77a462 Mon Sep 17 00:00:00 2001 From: Daniel Ramos Date: Tue, 11 Oct 2022 16:17:46 -0700 Subject: [PATCH 2/3] Took away lazy and put back private constructor --- .../src/System/Diagnostics/Metrics/MetricsEventSource.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs index 84e8b6d373d75..d2cbc7fe91e18 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs @@ -60,7 +60,9 @@ public static class Keywords public const EventKeywords InstrumentPublishing = (EventKeywords)0x4; } - private Lazy _handler = new Lazy(); + private CommandHandler _handler = new CommandHandler(); + + private MetricsEventSource(){} /// /// Used to send ad-hoc diagnostics to humans. @@ -184,7 +186,7 @@ protected override void OnEventCommand(EventCommandEventArgs command) { lock (this) { - _handler.Value.OnEventCommand(command, this); + _handler.OnEventCommand(command, this); } } From 106ab1d595536df50fb78cecbf1e42f331c0e0d5 Mon Sep 17 00:00:00 2001 From: Daniel Ramos Date: Fri, 14 Oct 2022 11:50:00 -0700 Subject: [PATCH 3/3] Added parent property and made handler thread safe --- .../Diagnostics/Metrics/MetricsEventSource.cs | 96 +++++++++++-------- 1 file changed, 58 insertions(+), 38 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs index d2cbc7fe91e18..458d1c9d34e5a 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs @@ -7,6 +7,7 @@ using System.Globalization; using System.Runtime.Versioning; using System.Text; +using System.Threading; namespace System.Diagnostics.Metrics { @@ -60,9 +61,21 @@ public static class Keywords public const EventKeywords InstrumentPublishing = (EventKeywords)0x4; } - private CommandHandler _handler = new CommandHandler(); + private CommandHandler? _handler; - private MetricsEventSource(){} + private CommandHandler Handler + { + get + { + if (_handler == null) + { + Interlocked.CompareExchange(ref _handler, new CommandHandler(this), null); + } + return _handler; + } + } + + private MetricsEventSource() { } /// /// Used to send ad-hoc diagnostics to humans. @@ -186,7 +199,7 @@ protected override void OnEventCommand(EventCommandEventArgs command) { lock (this) { - _handler.OnEventCommand(command, this); + Handler.OnEventCommand(command); } } @@ -199,7 +212,14 @@ private sealed class CommandHandler private AggregationManager? _aggregationManager; private string _sessionId = ""; - public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource parent) + public CommandHandler(MetricsEventSource parent) + { + Parent = parent; + } + + public MetricsEventSource Parent { get; private set;} + + public void OnEventCommand(EventCommandEventArgs command) { try { @@ -212,7 +232,7 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par // This limitation shouldn't really matter because browser also doesn't support out-of-proc EventSource communication // which is the intended scenario for this EventSource. If it matters in the future AggregationManager can be // modified to have some other fallback path that works for browser. - parent.Error("", "System.Diagnostics.Metrics EventSource not supported on browser"); + Parent.Error("", "System.Diagnostics.Metrics EventSource not supported on browser"); return; } #endif @@ -230,13 +250,13 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par // one other listener is active. In the future we might be able to figure out how // to infer the changes from the info we do have or add a better API but for now // I am taking the simple route and not supporting it. - parent.MultipleSessionsNotSupportedError(_sessionId); + Parent.MultipleSessionsNotSupportedError(_sessionId); return; } _aggregationManager.Dispose(); _aggregationManager = null; - parent.Message($"Previous session with id {_sessionId} is stopped"); + Parent.Message($"Previous session with id {_sessionId} is stopped"); } _sessionId = ""; } @@ -246,12 +266,12 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par if (command.Arguments!.TryGetValue("SessionId", out string? id)) { _sessionId = id!; - parent.Message($"SessionId argument received: {_sessionId}"); + Parent.Message($"SessionId argument received: {_sessionId}"); } else { _sessionId = System.Guid.NewGuid().ToString(); - parent.Message($"New session started. SessionId auto-generated: {_sessionId}"); + Parent.Message($"New session started. SessionId auto-generated: {_sessionId}"); } @@ -260,21 +280,21 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par double refreshIntervalSecs; if (command.Arguments!.TryGetValue("RefreshInterval", out string? refreshInterval)) { - parent.Message($"RefreshInterval argument received: {refreshInterval}"); + Parent.Message($"RefreshInterval argument received: {refreshInterval}"); if (!double.TryParse(refreshInterval, out refreshIntervalSecs)) { - parent.Message($"Failed to parse RefreshInterval. Using default {defaultIntervalSecs}s."); + Parent.Message($"Failed to parse RefreshInterval. Using default {defaultIntervalSecs}s."); refreshIntervalSecs = defaultIntervalSecs; } else if (refreshIntervalSecs < AggregationManager.MinCollectionTimeSecs) { - parent.Message($"RefreshInterval too small. Using minimum interval {AggregationManager.MinCollectionTimeSecs} seconds."); + Parent.Message($"RefreshInterval too small. Using minimum interval {AggregationManager.MinCollectionTimeSecs} seconds."); refreshIntervalSecs = AggregationManager.MinCollectionTimeSecs; } } else { - parent.Message($"No RefreshInterval argument received. Using default {defaultIntervalSecs}s."); + Parent.Message($"No RefreshInterval argument received. Using default {defaultIntervalSecs}s."); refreshIntervalSecs = defaultIntervalSecs; } @@ -282,16 +302,16 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par int maxTimeSeries; if (command.Arguments!.TryGetValue("MaxTimeSeries", out string? maxTimeSeriesString)) { - parent.Message($"MaxTimeSeries argument received: {maxTimeSeriesString}"); + Parent.Message($"MaxTimeSeries argument received: {maxTimeSeriesString}"); if (!int.TryParse(maxTimeSeriesString, out maxTimeSeries)) { - parent.Message($"Failed to parse MaxTimeSeries. Using default {defaultMaxTimeSeries}"); + Parent.Message($"Failed to parse MaxTimeSeries. Using default {defaultMaxTimeSeries}"); maxTimeSeries = defaultMaxTimeSeries; } } else { - parent.Message($"No MaxTimeSeries argument received. Using default {defaultMaxTimeSeries}"); + Parent.Message($"No MaxTimeSeries argument received. Using default {defaultMaxTimeSeries}"); maxTimeSeries = defaultMaxTimeSeries; } @@ -299,16 +319,16 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par int maxHistograms; if (command.Arguments!.TryGetValue("MaxHistograms", out string? maxHistogramsString)) { - parent.Message($"MaxHistograms argument received: {maxHistogramsString}"); + Parent.Message($"MaxHistograms argument received: {maxHistogramsString}"); if (!int.TryParse(maxHistogramsString, out maxHistograms)) { - parent.Message($"Failed to parse MaxHistograms. Using default {defaultMaxHistograms}"); + Parent.Message($"Failed to parse MaxHistograms. Using default {defaultMaxHistograms}"); maxHistograms = defaultMaxHistograms; } } else { - parent.Message($"No MaxHistogram argument received. Using default {defaultMaxHistograms}"); + Parent.Message($"No MaxHistogram argument received. Using default {defaultMaxHistograms}"); maxHistograms = defaultMaxHistograms; } @@ -317,41 +337,41 @@ public void OnEventCommand(EventCommandEventArgs command, MetricsEventSource par maxTimeSeries, maxHistograms, (i, s) => TransmitMetricValue(i, s, sessionId), - (startIntervalTime, endIntervalTime) => parent.CollectionStart(sessionId, startIntervalTime, endIntervalTime), - (startIntervalTime, endIntervalTime) => parent.CollectionStop(sessionId, startIntervalTime, endIntervalTime), - i => parent.BeginInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), - i => parent.EndInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), - i => parent.InstrumentPublished(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), - () => parent.InitialInstrumentEnumerationComplete(sessionId), - e => parent.Error(sessionId, e.ToString()), - () => parent.TimeSeriesLimitReached(sessionId), - () => parent.HistogramLimitReached(sessionId), - e => parent.ObservableInstrumentCallbackError(sessionId, e.ToString())); + (startIntervalTime, endIntervalTime) => Parent.CollectionStart(sessionId, startIntervalTime, endIntervalTime), + (startIntervalTime, endIntervalTime) => Parent.CollectionStop(sessionId, startIntervalTime, endIntervalTime), + i => Parent.BeginInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), + i => Parent.EndInstrumentReporting(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), + i => Parent.InstrumentPublished(sessionId, i.Meter.Name, i.Meter.Version, i.Name, i.GetType().Name, i.Unit, i.Description), + () => Parent.InitialInstrumentEnumerationComplete(sessionId), + e => Parent.Error(sessionId, e.ToString()), + () => Parent.TimeSeriesLimitReached(sessionId), + () => Parent.HistogramLimitReached(sessionId), + e => Parent.ObservableInstrumentCallbackError(sessionId, e.ToString())); _aggregationManager.SetCollectionPeriod(TimeSpan.FromSeconds(refreshIntervalSecs)); if (command.Arguments!.TryGetValue("Metrics", out string? metricsSpecs)) { - parent.Message($"Metrics argument received: {metricsSpecs}"); - ParseSpecs(metricsSpecs, parent); + Parent.Message($"Metrics argument received: {metricsSpecs}"); + ParseSpecs(metricsSpecs); } else { - parent.Message("No Metrics argument received"); + Parent.Message("No Metrics argument received"); } _aggregationManager.Start(); } } - catch (Exception e) when (LogError(e, parent)) + catch (Exception e) when (LogError(e)) { // this will never run } } - private bool LogError(Exception e, MetricsEventSource parent) + private bool LogError(Exception e) { - parent.Error(_sessionId, e.ToString()); + Parent.Error(_sessionId, e.ToString()); // this code runs as an exception filter // returning false ensures the catch handler isn't run return false; @@ -360,7 +380,7 @@ private bool LogError(Exception e, MetricsEventSource parent) private static readonly char[] s_instrumentSeparators = new char[] { '\r', '\n', ',', ';' }; [UnsupportedOSPlatform("browser")] - private void ParseSpecs(string? metricsSpecs, MetricsEventSource parent) + private void ParseSpecs(string? metricsSpecs) { if (metricsSpecs == null) { @@ -371,11 +391,11 @@ private void ParseSpecs(string? metricsSpecs, MetricsEventSource parent) { if (!MetricSpec.TryParse(specString, out MetricSpec spec)) { - parent.Message($"Failed to parse metric spec: {specString}"); + Parent.Message($"Failed to parse metric spec: {specString}"); } else { - parent.Message($"Parsed metric: {spec}"); + Parent.Message($"Parsed metric: {spec}"); if (spec.InstrumentName != null) { _aggregationManager!.Include(spec.MeterName, spec.InstrumentName);