From 74b921c63f015fad2f22d818eeea5acd0a1c6eb1 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Wed, 15 Jun 2022 16:23:20 -0700 Subject: [PATCH 1/9] initial commit --- .../Extensibility/MetricValuesBuffer.cs | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs index d1aa54ed08..22f4d8045a 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs @@ -46,6 +46,8 @@ public int NextFlushIndex set { this.nextFlushIndex = value; } } + protected abstract TValue DefaultValue { get; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public int IncWriteIndex() { @@ -79,31 +81,29 @@ public void ResetIndicesAndData() public TValue GetAndResetValue(int index) { - TValue value = this.GetAndResetValueOnce(this.values, index); - - if (this.IsInvalidValue(value)) + for (int spinCountdown = 10000; spinCountdown > 0; spinCountdown--) { -#pragma warning disable SA1129 // Do not use default value type constructor - var spinWait = new SpinWait(); -#pragma warning restore SA1129 // Do not use default value type constructor + TValue value = this.GetAndResetValueOnce(this.values, index); - value = this.GetAndResetValueOnce(this.values, index); - while (this.IsInvalidValue(value)) + if (this.IsInvalidValue(value)) { - spinWait.SpinOnce(); - - if (spinWait.Count % 100 == 0) + if (spinCountdown % 100 == 0) { - // In tests (including stress tests) we always finished wating before 100 cycles. + // In tests (including stress tests) we always finished waiting before 100 cycles. // However, this is a protection against en extreme case on a slow machine. Task.Delay(10).ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult(); } - value = this.GetAndResetValueOnce(this.values, index); + continue; + } + else + { + return value; } } - return value; + // exceeded maximum spin count + return this.DefaultValue; } protected abstract void ResetValues(TValue[] values); @@ -127,6 +127,8 @@ public MetricValuesBuffer_Double(int capacity) { } + protected override double DefaultValue => double.NaN; + [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override bool IsInvalidValue(double value) { @@ -162,6 +164,8 @@ public MetricValuesBuffer_Object(int capacity) { } + protected override object DefaultValue => null; + [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override bool IsInvalidValue(object value) { From 59a34a22707099651d3e9efccfef9867f587be7a Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Thu, 16 Jun 2022 13:36:34 -0700 Subject: [PATCH 2/9] less invasive change --- .../Extensibility/MetricValuesBuffer.cs | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs index 22f4d8045a..b0e694d8c2 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs @@ -81,29 +81,36 @@ public void ResetIndicesAndData() public TValue GetAndResetValue(int index) { - for (int spinCountdown = 10000; spinCountdown > 0; spinCountdown--) + TValue value = this.GetAndResetValueOnce(this.values, index); + + if (this.IsInvalidValue(value)) { - TValue value = this.GetAndResetValueOnce(this.values, index); +#pragma warning disable SA1129 // Do not use default value type constructor + var spinWait = new SpinWait(); +#pragma warning restore SA1129 // Do not use default value type constructor - if (this.IsInvalidValue(value)) + value = this.GetAndResetValueOnce(this.values, index); + while (this.IsInvalidValue(value)) { - if (spinCountdown % 100 == 0) + spinWait.SpinOnce(); + + if (spinWait.Count % 100 == 0) { - // In tests (including stress tests) we always finished waiting before 100 cycles. + // In tests (including stress tests) we always finished wating before 100 cycles. // However, this is a protection against en extreme case on a slow machine. Task.Delay(10).ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult(); } + else if (spinWait.Count > 10000) + { + // exceeded maximum spin count. Break out to avoid infinite loop. + return this.DefaultValue; + } - continue; - } - else - { - return value; + value = this.GetAndResetValueOnce(this.values, index); } } - // exceeded maximum spin count - return this.DefaultValue; + return value; } protected abstract void ResetValues(TValue[] values); From 891bced9ffd01d97804a897226ed73bb5c27d9a5 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Thu, 16 Jun 2022 13:37:55 -0700 Subject: [PATCH 3/9] rename --- .../Metrics/Extensibility/MetricValuesBuffer.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs index b0e694d8c2..16308e147b 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs @@ -46,7 +46,7 @@ public int NextFlushIndex set { this.nextFlushIndex = value; } } - protected abstract TValue DefaultValue { get; } + protected abstract TValue InvalidValue { get; } [MethodImpl(MethodImplOptions.AggressiveInlining)] public int IncWriteIndex() @@ -103,7 +103,7 @@ public TValue GetAndResetValue(int index) else if (spinWait.Count > 10000) { // exceeded maximum spin count. Break out to avoid infinite loop. - return this.DefaultValue; + return this.InvalidValue; } value = this.GetAndResetValueOnce(this.values, index); @@ -134,7 +134,7 @@ public MetricValuesBuffer_Double(int capacity) { } - protected override double DefaultValue => double.NaN; + protected override double InvalidValue => double.NaN; [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override bool IsInvalidValue(double value) @@ -171,7 +171,7 @@ public MetricValuesBuffer_Object(int capacity) { } - protected override object DefaultValue => null; + protected override object InvalidValue => null; [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override bool IsInvalidValue(object value) From 0817a0b016417d193174333d30f551a4bc585d71 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Thu, 16 Jun 2022 13:48:37 -0700 Subject: [PATCH 4/9] review comment and changelog --- .../Metrics/Extensibility/MetricValuesBuffer.cs | 8 +------- CHANGELOG.md | 1 + 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs index 16308e147b..763c22e5c6 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs @@ -46,8 +46,6 @@ public int NextFlushIndex set { this.nextFlushIndex = value; } } - protected abstract TValue InvalidValue { get; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] public int IncWriteIndex() { @@ -103,7 +101,7 @@ public TValue GetAndResetValue(int index) else if (spinWait.Count > 10000) { // exceeded maximum spin count. Break out to avoid infinite loop. - return this.InvalidValue; + break; } value = this.GetAndResetValueOnce(this.values, index); @@ -134,8 +132,6 @@ public MetricValuesBuffer_Double(int capacity) { } - protected override double InvalidValue => double.NaN; - [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override bool IsInvalidValue(double value) { @@ -171,8 +167,6 @@ public MetricValuesBuffer_Object(int capacity) { } - protected override object InvalidValue => null; - [MethodImpl(MethodImplOptions.AggressiveInlining)] protected override bool IsInvalidValue(object value) { diff --git a/CHANGELOG.md b/CHANGELOG.md index dbcf7ef79c..6280c9bcba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - [Extension methods to retrive specific operation details.](https://github.com/microsoft/ApplicationInsights-dotnet/issues/1350) - [Mark Instrumentation Key based APIs as Obsolete](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2560). - See also: https://docs.microsoft.com/azure/azure-monitor/app/migrate-from-instrumentation-keys-to-connection-strings +- [Fix: Deadlock when Flushing Metrics.](https://github.com/microsoft/ApplicationInsights-dotnet/pull/2612) ## Version 2.21.0-beta1 - [Support IPv6 in request headers](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2521) From b9c316666dc13a295a99d805b7382da20a3c949b Mon Sep 17 00:00:00 2001 From: Timothy Mothra Date: Thu, 16 Jun 2022 13:58:16 -0700 Subject: [PATCH 5/9] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6280c9bcba..9c9f8ab4f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - [Extension methods to retrive specific operation details.](https://github.com/microsoft/ApplicationInsights-dotnet/issues/1350) - [Mark Instrumentation Key based APIs as Obsolete](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2560). - See also: https://docs.microsoft.com/azure/azure-monitor/app/migrate-from-instrumentation-keys-to-connection-strings -- [Fix: Deadlock when Flushing Metrics.](https://github.com/microsoft/ApplicationInsights-dotnet/pull/2612) +- [Fix: Livelock in MetricValuesBuffer.](https://github.com/microsoft/ApplicationInsights-dotnet/pull/2612) ## Version 2.21.0-beta1 - [Support IPv6 in request headers](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2521) From ed1494917276a6f4578219cebf256837282537f8 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Fri, 17 Jun 2022 11:23:03 -0700 Subject: [PATCH 6/9] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c9f8ab4f8..36f4382317 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - [Mark Instrumentation Key based APIs as Obsolete](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2560). - See also: https://docs.microsoft.com/azure/azure-monitor/app/migrate-from-instrumentation-keys-to-connection-strings - [Fix: Livelock in MetricValuesBuffer.](https://github.com/microsoft/ApplicationInsights-dotnet/pull/2612) + Mitigation for TelemetryClient.Flush deadlocks ([#1186](https://github.com/microsoft/ApplicationInsights-dotnet/issues/1186)) ## Version 2.21.0-beta1 - [Support IPv6 in request headers](https://github.com/microsoft/ApplicationInsights-dotnet/issues/2521) From e0625856517931519d21a13da1558c13d9f141ef Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Fri, 17 Jun 2022 11:23:40 -0700 Subject: [PATCH 7/9] log warning --- .../Implementation/Tracing/CoreEventSource.cs | 4 ++++ .../Metrics/Extensibility/MetricValuesBuffer.cs | 7 +++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs b/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs index 8630882d95..3b4118f63b 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs @@ -672,6 +672,10 @@ public void IngestionResponseTimeEventCounter(float responseDurationInMs) [Event(75, Message = "Ingestion Service responded with redirect. {0}", Level = EventLevel.Error)] public void IngestionRedirectError(string message, string appDomainName = "Incorrect") => this.WriteEvent(75, message, this.nameProvider.Name); + [Event(76, Message = "MetricValueBuffer exceeded spin count.", Level = EventLevel.Warning)] + public void MetricValueBufferExceededSpinCount(string appDomainName = "Incorrect") => this.WriteEvent(76, this.nameProvider.Name); + + [NonEvent] public void TransmissionStatusEventFailed(Exception ex) { diff --git a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs index 763c22e5c6..f4d0a597f0 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs @@ -1,5 +1,7 @@ namespace Microsoft.ApplicationInsights.Metrics.Extensibility { + using Microsoft.ApplicationInsights.Extensibility.Implementation.Tracing; + using System; using System.Runtime.CompilerServices; using System.Threading; @@ -94,13 +96,14 @@ public TValue GetAndResetValue(int index) if (spinWait.Count % 100 == 0) { - // In tests (including stress tests) we always finished wating before 100 cycles. + // In tests (including stress tests) we always finished waiting before 100 cycles. // However, this is a protection against en extreme case on a slow machine. Task.Delay(10).ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult(); } else if (spinWait.Count > 10000) { - // exceeded maximum spin count. Break out to avoid infinite loop. + // Exceeded maximum spin count. Break out to avoid infinite loop. + CoreEventSource.Log.MetricValueBufferExceededSpinCount(); break; } From 404ef6c0e99f8d669caf40b3570aa395c0997527 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Fri, 17 Jun 2022 11:31:00 -0700 Subject: [PATCH 8/9] sort usings --- .../Metrics/Extensibility/MetricValuesBuffer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs index f4d0a597f0..389ffb5c6d 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Metrics/Extensibility/MetricValuesBuffer.cs @@ -1,12 +1,12 @@ namespace Microsoft.ApplicationInsights.Metrics.Extensibility { - using Microsoft.ApplicationInsights.Extensibility.Implementation.Tracing; - using System; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; + using Microsoft.ApplicationInsights.Extensibility.Implementation.Tracing; + #pragma warning disable SA1649 // File name must match first type name #pragma warning disable SA1402 // File may only contain a single class From 10f521c9ce337d1323e992fdff8c2a044b03c251 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Fri, 17 Jun 2022 11:37:29 -0700 Subject: [PATCH 9/9] fxcop --- .../Extensibility/Implementation/Tracing/CoreEventSource.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs b/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs index 3b4118f63b..7b0af7ae26 100644 --- a/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs +++ b/BASE/src/Microsoft.ApplicationInsights/Extensibility/Implementation/Tracing/CoreEventSource.cs @@ -675,7 +675,6 @@ public void IngestionResponseTimeEventCounter(float responseDurationInMs) [Event(76, Message = "MetricValueBuffer exceeded spin count.", Level = EventLevel.Warning)] public void MetricValueBufferExceededSpinCount(string appDomainName = "Incorrect") => this.WriteEvent(76, this.nameProvider.Name); - [NonEvent] public void TransmissionStatusEventFailed(Exception ex) {