From c3704a944bff45b728a6d48a2082e1b24aac4aae Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Fri, 19 Sep 2025 16:38:26 +0100 Subject: [PATCH 1/7] fix: resolve concurrency issues in Metrics library - Replace LINQ FirstOrDefault with thread-safe iteration in Metrics.cs and MetricDirective.cs - Change CustomMetadata from Dictionary to ConcurrentDictionary in Metadata.cs - Add comprehensive concurrency tests with proper cleanup - Fix IndexOutOfRangeException and InvalidOperationException in multi-threaded scenarios - Maintain backward compatibility and performance --- .../AWS.Lambda.Powertools.Metrics/Metrics.cs | 32 ++- .../Model/Metadata.cs | 5 +- .../Model/MetricDirective.cs | 31 ++- .../ConcurrencyIssueTest.cs | 247 ++++++++++++++++++ 4 files changed, 310 insertions(+), 5 deletions(-) create mode 100644 libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs index 9cf95ec51..9615368a8 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs @@ -199,8 +199,7 @@ void IMetrics.AddMetric(string key, double value, MetricUnit unit, MetricResolut if (metrics.Count > 0 && (metrics.Count == PowertoolsConfigurations.MaxMetrics || - metrics.FirstOrDefault(x => x.Name == key) - ?.Values.Count == PowertoolsConfigurations.MaxMetrics)) + GetExistingMetric(metrics, key)?.Values.Count == PowertoolsConfigurations.MaxMetrics)) { Instance.Flush(true); } @@ -624,6 +623,35 @@ public static void Flush(bool metricsOverflow = false) Instance.Flush(metricsOverflow); } + /// + /// Safely searches for an existing metric by name without using LINQ enumeration + /// + /// The metrics collection to search + /// The metric name to search for + /// The found metric or null if not found + private static MetricDefinition GetExistingMetric(List metrics, string key) + { + // Use a traditional for loop instead of LINQ to avoid enumeration issues + // when the collection is modified concurrently + for (int i = 0; i < metrics.Count; i++) + { + try + { + var metric = metrics[i]; + if (metric?.Name == key) + { + return metric; + } + } + catch (ArgumentOutOfRangeException) + { + // Collection was modified during iteration, return null to be safe + return null; + } + } + return null; + } + /// /// Helper method for testing purposes. Clears static instance between test execution /// diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs index 847b0dc88..08252d49f 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Text.Json.Serialization; @@ -15,7 +16,7 @@ public class Metadata public Metadata() { CloudWatchMetrics = new List { new() }; - CustomMetadata = new Dictionary(); + CustomMetadata = new ConcurrentDictionary(); } /// @@ -43,7 +44,7 @@ public Metadata() /// /// The custom metadata. [JsonIgnore] - public Dictionary CustomMetadata { get; } + public ConcurrentDictionary CustomMetadata { get; } /// /// Deletes all metrics from memory diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs index 0d300d5e8..99f2d46a2 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs @@ -160,7 +160,7 @@ public void AddMetric(string name, double value, MetricUnit unit, MetricResoluti { lock (_lockObj) { - var metric = Metrics.FirstOrDefault(m => m.Name == name); + var metric = GetExistingMetric(Metrics, name); if (metric != null) { if (metric.Values.Count < PowertoolsConfigurations.MaxMetrics) @@ -298,6 +298,35 @@ internal void AddDimensionSet(List dimensionSets) } } + /// + /// Safely searches for an existing metric by name without using LINQ enumeration + /// + /// The metrics collection to search + /// The metric name to search for + /// The found metric or null if not found + private static MetricDefinition GetExistingMetric(List metrics, string name) + { + // Use a traditional for loop instead of LINQ to avoid enumeration issues + // when the collection is modified concurrently + for (int i = 0; i < metrics.Count; i++) + { + try + { + var metric = metrics[i]; + if (metric?.Name == name) + { + return metric; + } + } + catch (ArgumentOutOfRangeException) + { + // Collection was modified during iteration, return null to be safe + return null; + } + } + return null; + } + /// /// Clears both default dimensions and dimensions lists /// diff --git a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs new file mode 100644 index 000000000..303094cf1 --- /dev/null +++ b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs @@ -0,0 +1,247 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using AWS.Lambda.Powertools.Metrics; +using Xunit; + +namespace AWS.Lambda.Powertools.Metrics.Tests +{ + public class ConcurrencyIssueTest : IDisposable + { + [Fact] + public async Task AddMetric_ConcurrentAccess_ShouldNotThrowException() + { + // Arrange + Metrics.ResetForTest(); + Metrics.SetNamespace("TestNamespace"); + var exceptions = new List(); + var tasks = new List(); + + // Act - Simulate concurrent access from multiple threads + for (int i = 0; i < 10; i++) + { + var taskId = i; + tasks.Add(Task.Run(() => + { + try + { + // Simulate multiple metrics being added concurrently + for (int j = 0; j < 100; j++) + { + Metrics.AddMetric($"TestMetric_{taskId}_{j}", 1.0, MetricUnit.Count); + Metrics.AddMetric($"Client.{taskId}", 1.0, MetricUnit.Count); + Metrics.AddMetadata($"TestMetadata_{taskId}_{j}", $"value_{j}"); + } + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + })); + } + + await Task.WhenAll(tasks); + + // Assert + foreach (var ex in exceptions) + { + Console.WriteLine($"Exception: {ex.GetType().Name}: {ex.Message}"); + if (ex.StackTrace != null) + Console.WriteLine($"Stack trace: {ex.StackTrace}"); + } + Assert.Empty(exceptions); + + // Cleanup after test + CleanupMetrics(); + } + + [Fact] + public async Task AddMetric_ConcurrentAccessWithSameKey_ShouldNotThrowException() + { + // Arrange + Metrics.ResetForTest(); + Metrics.SetNamespace("TestNamespace"); + var exceptions = new List(); + var tasks = new List(); + + // Act - Simulate the specific scenario where the same metric key is used concurrently + // Increase concurrency to try to reproduce the issue + for (int i = 0; i < 50; i++) + { + tasks.Add(Task.Run(() => + { + try + { + // This simulates the scenario where the same metric key + // (like "Client.6b70*28198e") is being added from multiple threads + for (int j = 0; j < 200; j++) + { + Metrics.AddMetric("Client.SharedKey", 1.0, MetricUnit.Count); + } + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + })); + } + + await Task.WhenAll(tasks); + + // Assert - Should not have any exceptions + foreach (var ex in exceptions) + { + Console.WriteLine($"Exception: {ex.GetType().Name}: {ex.Message}"); + if (ex.StackTrace != null) + Console.WriteLine($"Stack trace: {ex.StackTrace}"); + } + Assert.Empty(exceptions); + + // Cleanup after test + CleanupMetrics(); + } + + [Fact] + public async Task AddMetric_Batch_ShouldNotThrowException() + { + // Arrange + Metrics.ResetForTest(); + Metrics.SetNamespace("TestNamespace"); + var exceptions = new List(); + var tasks = new List(); + + for (int i = 0; i < 5; i++) + { + var batchId = i; + tasks.Add(Task.Run(async () => + { + try + { + // Simulate DataLoader batch processing + var innerTasks = new List(); + for (int j = 0; j < 10; j++) + { + var itemId = j; + innerTasks.Add(Task.Run(() => + { + // Simulate metrics being added from parallel DataLoader operations + Metrics.AddMetric($"DataLoader.InsidersStatusDataLoader", 1.0, MetricUnit.Count); + Metrics.AddMetric($"Query.insidersStatus", 1.0, MetricUnit.Count); + Metrics.AddMetric($"Client.6b70*28198e", 1.0, MetricUnit.Count); + Metrics.AddMetadata($"Query.insidersStatus.OperationName", "GetInsidersStatus"); + Metrics.AddMetadata($"Query.insidersStatus.UserId", $"user_{batchId}_{itemId}"); + })); + } + await Task.WhenAll(innerTasks); + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + })); + } + + await Task.WhenAll(tasks); + + // Assert + foreach (var ex in exceptions) + { + Console.WriteLine($"Exception: {ex.GetType().Name}: {ex.Message}"); + if (ex.StackTrace != null) + Console.WriteLine($"Stack trace: {ex.StackTrace}"); + } + Assert.Empty(exceptions); + + // Cleanup after test + CleanupMetrics(); + } + + [Fact] + public async Task AddMetric_ReproduceFirstOrDefaultIssue_ShouldNotThrowException() + { + // Arrange + Metrics.ResetForTest(); + Metrics.SetNamespace("TestNamespace"); + var exceptions = new List(); + var tasks = new List(); + + // Act - This test specifically targets the FirstOrDefault issue in line 202-203 of Metrics.cs + // metrics are added and flushed rapidly to trigger collection modification + for (int i = 0; i < 100; i++) + { + var taskId = i; + tasks.Add(Task.Run(() => + { + try + { + // Add metrics rapidly to trigger the overflow condition that calls FirstOrDefault + for (int j = 0; j < 150; j++) // This should trigger multiple flushes + { + Metrics.AddMetric($"TestMetric_{taskId}_{j}", 1.0, MetricUnit.Count); + + // Also add the same metric key to trigger the FirstOrDefault path + if (j % 10 == 0) + { + Metrics.AddMetric("SharedMetric", 1.0, MetricUnit.Count); + } + } + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + })); + } + + await Task.WhenAll(tasks); + + // Assert + foreach (var ex in exceptions) + { + Console.WriteLine($"Exception: {ex.GetType().Name}: {ex.Message}"); + if (ex.StackTrace != null) + Console.WriteLine($"Stack trace: {ex.StackTrace}"); + } + Assert.Empty(exceptions); + + // Cleanup after test + CleanupMetrics(); + } + + /// + /// Cleanup method to ensure no state leaks between tests + /// + private void CleanupMetrics() + { + try + { + // Reset the static instance to clean state + Metrics.ResetForTest(); + } + catch + { + // Ignore cleanup errors + } + } + + /// + /// IDisposable implementation for proper test cleanup + /// + public void Dispose() + { + CleanupMetrics(); + } + } +} \ No newline at end of file From 364d41d60a8567a82e3bd950d1cb594ba99791e6 Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Fri, 19 Sep 2025 16:53:13 +0100 Subject: [PATCH 2/7] improve code cov --- .../ConcurrencyIssueTest.cs | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs index 303094cf1..a2ba24eba 100644 --- a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs +++ b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs @@ -220,6 +220,67 @@ public async Task AddMetric_ReproduceFirstOrDefaultIssue_ShouldNotThrowException CleanupMetrics(); } + [Fact] + public async Task AddMetric_ConcurrentModificationDuringIteration_ShouldHandleArgumentOutOfRangeException() + { + // Arrange + Metrics.ResetForTest(); + Metrics.SetNamespace("TestNamespace"); + var exceptions = new List(); + var tasks = new List(); + + // Act - Create a scenario where collection modification happens during iteration + // This test specifically targets the ArgumentOutOfRangeException catch block in GetExistingMetric + for (int i = 0; i < 20; i++) + { + var taskId = i; + tasks.Add(Task.Run(() => + { + try + { + // Rapidly add and flush metrics to create timing conditions + // where GetExistingMetric might access an index that becomes invalid + for (int j = 0; j < 200; j++) + { + // Add metrics with the same key to trigger GetExistingMetric calls + Metrics.AddMetric("SharedMetricKey", 1.0, MetricUnit.Count); + + // Occasionally add many metrics to trigger flush (which clears the collection) + if (j % 50 == 0) + { + // Add enough metrics to trigger overflow and flush + for (int k = 0; k < 105; k++) // Exceeds MaxMetrics (100) + { + Metrics.AddMetric($"OverflowMetric_{taskId}_{k}", 1.0, MetricUnit.Count); + } + } + } + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + })); + } + + await Task.WhenAll(tasks); + + // Assert - Should not have any exceptions, even if ArgumentOutOfRangeException occurs internally + foreach (var ex in exceptions) + { + Console.WriteLine($"Exception: {ex.GetType().Name}: {ex.Message}"); + if (ex.StackTrace != null) + Console.WriteLine($"Stack trace: {ex.StackTrace}"); + } + Assert.Empty(exceptions); + + // Cleanup after test + CleanupMetrics(); + } + /// /// Cleanup method to ensure no state leaks between tests /// From 86589683a69a6f5ddf7236e1fc4a39fc01f1c9cf Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Fri, 19 Sep 2025 17:06:10 +0100 Subject: [PATCH 3/7] more coverage --- .../ConcurrencyIssueTest.cs | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs index a2ba24eba..30c8cab75 100644 --- a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs +++ b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Reflection; using System.Threading.Tasks; using AWS.Lambda.Powertools.Metrics; using Xunit; @@ -281,6 +282,61 @@ public async Task AddMetric_ConcurrentModificationDuringIteration_ShouldHandleAr CleanupMetrics(); } + [Fact] + public void GetExistingMetric_ArgumentOutOfRangeException_ShouldReturnNull() + { + // Arrange - Use reflection to test the private GetExistingMetric method directly + var getExistingMetricMethod = typeof(Metrics).GetMethod("GetExistingMetric", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); + + Assert.NotNull(getExistingMetricMethod); + + // Create a custom list that will throw ArgumentOutOfRangeException + var metricsList = new TestMetricsList(); + metricsList.Add(new MetricDefinition("TestMetric", MetricUnit.Count, new List { 1.0 }, MetricResolution.Default)); + + // Act - Call the private method via reflection, searching for a different key + // This will cause the method to iterate and hit the indexer that throws ArgumentOutOfRangeException + var result = getExistingMetricMethod.Invoke(null, new object[] { metricsList, "NonExistentMetric" }); + + // Assert - Should return null when ArgumentOutOfRangeException is caught + Assert.Null(result); + } + + /// + /// Custom list that throws ArgumentOutOfRangeException on indexer access + /// to simulate the race condition scenario + /// + private class TestMetricsList : List + { + private bool _shouldThrow = false; + + public new int Count + { + get + { + // Return 1 initially, but set flag to throw on indexer access + _shouldThrow = true; + return base.Count; + } + } + + public new MetricDefinition this[int index] + { + get + { + if (_shouldThrow) + { + // Throw ArgumentOutOfRangeException to simulate the race condition + // where collection was modified between Count check and indexer access + throw new ArgumentOutOfRangeException(nameof(index), "Simulated race condition"); + } + return base[index]; + } + set => base[index] = value; + } + } + /// /// Cleanup method to ensure no state leaks between tests /// From b92cce38aa219ddf33dc23e71f6c3892f424c241 Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Fri, 19 Sep 2025 19:32:14 +0100 Subject: [PATCH 4/7] trying again to test the exception logic --- .../ConcurrencyIssueTest.cs | 105 +++++++++++++----- 1 file changed, 75 insertions(+), 30 deletions(-) diff --git a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs index 30c8cab75..b64e0d362 100644 --- a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs +++ b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ConcurrencyIssueTest.cs @@ -291,49 +291,94 @@ public void GetExistingMetric_ArgumentOutOfRangeException_ShouldReturnNull() Assert.NotNull(getExistingMetricMethod); - // Create a custom list that will throw ArgumentOutOfRangeException - var metricsList = new TestMetricsList(); - metricsList.Add(new MetricDefinition("TestMetric", MetricUnit.Count, new List { 1.0 }, MetricResolution.Default)); + // Create a list that will throw ArgumentOutOfRangeException on indexer access + // This directly tests the catch (ArgumentOutOfRangeException) block in GetExistingMetric + var metricsList = new ThrowingList(); - // Act - Call the private method via reflection, searching for a different key - // This will cause the method to iterate and hit the indexer that throws ArgumentOutOfRangeException - var result = getExistingMetricMethod.Invoke(null, new object[] { metricsList, "NonExistentMetric" }); + // Act - Call the private method via reflection + var result = getExistingMetricMethod.Invoke(null, new object[] { metricsList, "TestMetric" }); // Assert - Should return null when ArgumentOutOfRangeException is caught Assert.Null(result); + + // Additional verification - ensure our ThrowingList actually throws + Assert.Equal(1, metricsList.Count); // Should return 1 + Assert.Throws(() => _ = metricsList[0]); // Should throw } - /// - /// Custom list that throws ArgumentOutOfRangeException on indexer access - /// to simulate the race condition scenario - /// - private class TestMetricsList : List + [Fact] + public async Task AddMetric_ExtremeRaceCondition_ShouldCoverArgumentOutOfRangeException() { - private bool _shouldThrow = false; + // This test is designed to create the exact timing conditions that would + // trigger ArgumentOutOfRangeException in GetExistingMetric during real usage - public new int Count - { - get - { - // Return 1 initially, but set flag to throw on indexer access - _shouldThrow = true; - return base.Count; - } - } + // Arrange + Metrics.ResetForTest(); + Metrics.SetNamespace("TestNamespace"); + var exceptions = new List(); + var tasks = new List(); - public new MetricDefinition this[int index] + // Act - Create extreme race conditions with very tight timing + for (int i = 0; i < 50; i++) { - get + var taskId = i; + tasks.Add(Task.Run(async () => { - if (_shouldThrow) + try { - // Throw ArgumentOutOfRangeException to simulate the race condition - // where collection was modified between Count check and indexer access - throw new ArgumentOutOfRangeException(nameof(index), "Simulated race condition"); + for (int j = 0; j < 500; j++) + { + // Add the same metric key repeatedly to trigger GetExistingMetric + Metrics.AddMetric("RaceConditionMetric", 1.0, MetricUnit.Count); + + // Create timing pressure with very short delays + if (j % 25 == 0) + { + await Task.Delay(1); // Tiny delay to create timing windows + + // Force flush by adding 100+ metrics + for (int k = 0; k < 101; k++) + { + Metrics.AddMetric($"FlushForce_{taskId}_{k}", 1.0, MetricUnit.Count); + } + } + } } - return base[index]; - } - set => base[index] = value; + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + })); + } + + await Task.WhenAll(tasks); + + // Assert - Should not have any unhandled exceptions + foreach (var ex in exceptions) + { + Console.WriteLine($"Exception: {ex.GetType().Name}: {ex.Message}"); + } + Assert.Empty(exceptions); + + // Cleanup + CleanupMetrics(); + } + + /// + /// Custom list that always throws ArgumentOutOfRangeException on indexer access + /// to directly test the exception handling path in GetExistingMetric + /// + private class ThrowingList : List + { + public new int Count => 1; // Return 1 so the for loop condition (i < metrics.Count) passes + + public new MetricDefinition this[int index] + { + get => throw new ArgumentOutOfRangeException(nameof(index), "Simulated concurrent modification"); + set => throw new ArgumentOutOfRangeException(nameof(index), "Simulated concurrent modification"); } } From 90fbcf48774034b5ffa8e3f4dc4cfbb49349007e Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Mon, 22 Sep 2025 08:52:02 +0100 Subject: [PATCH 5/7] fix: enhance thread safety in Metrics library to prevent concurrent modification issues --- .../AWS.Lambda.Powertools.Metrics/Metrics.cs | 34 ++- .../Model/DimensionSet.cs | 13 +- .../Model/Metadata.cs | 16 +- .../Model/MetricDefinition.cs | 5 +- .../Model/MetricDirective.cs | 206 ++++++++++---- .../Model/RootNode.cs | 83 +++++- .../SerializationThreadingTests.cs | 261 ++++++++++++++++++ .../ThreadingTests.cs | 117 ++++++++ 8 files changed, 662 insertions(+), 73 deletions(-) create mode 100644 libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/SerializationThreadingTests.cs create mode 100644 libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ThreadingTests.cs diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs index 9615368a8..adc981598 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Metrics.cs @@ -535,9 +535,17 @@ private Dictionary ListToDictionary(List dimension var dictionary = new Dictionary(); try { - return dimensions != null - ? new Dictionary(dimensions.SelectMany(x => x.Dimensions)) - : dictionary; + if (dimensions != null) + { + foreach (var dimensionSet in dimensions) + { + foreach (var kvp in dimensionSet.Dimensions) + { + dictionary[kvp.Key] = kvp.Value; + } + } + } + return dictionary; } catch (Exception e) { @@ -633,12 +641,21 @@ private static MetricDefinition GetExistingMetric(List metrics { // Use a traditional for loop instead of LINQ to avoid enumeration issues // when the collection is modified concurrently - for (int i = 0; i < metrics.Count; i++) + if (metrics == null || string.IsNullOrEmpty(key)) + return null; + + // Create a snapshot of the count to avoid issues with concurrent modifications + var count = metrics.Count; + for (int i = 0; i < count; i++) { try { + // Check bounds again in case collection was modified + if (i >= metrics.Count) + break; + var metric = metrics[i]; - if (metric?.Name == key) + if (metric != null && string.Equals(metric.Name, key, StringComparison.Ordinal)) { return metric; } @@ -646,7 +663,12 @@ private static MetricDefinition GetExistingMetric(List metrics catch (ArgumentOutOfRangeException) { // Collection was modified during iteration, return null to be safe - return null; + break; + } + catch (IndexOutOfRangeException) + { + // Collection was modified during iteration, return null to be safe + break; } } return null; diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/DimensionSet.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/DimensionSet.cs index 6c1e2d754..d1fdc30d9 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/DimensionSet.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/DimensionSet.cs @@ -43,5 +43,16 @@ public DimensionSet(string key, string value) /// Gets the dimension keys. /// /// The dimension keys. - public List DimensionKeys => Dimensions.Keys.ToList(); + public List DimensionKeys + { + get + { + var keys = new List(); + foreach (var key in Dimensions.Keys) + { + keys.Add(key); + } + return keys; + } + } } \ No newline at end of file diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs index 08252d49f..5d8655eb4 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/Metadata.cs @@ -51,7 +51,10 @@ public Metadata() /// internal void ClearMetrics() { - _metricDirective.Metrics.Clear(); + lock (_metricDirective._lockObj) + { + _metricDirective.Metrics.Clear(); + } CustomMetadata?.Clear(); } @@ -60,7 +63,10 @@ internal void ClearMetrics() /// internal void ClearNonDefaultDimensions() { - _metricDirective.Dimensions.Clear(); + lock (_metricDirective._lockObj) + { + _metricDirective.Dimensions.Clear(); + } } /// @@ -144,7 +150,11 @@ internal void SetDefaultDimensions(List defaultDimensionSets) /// List of metrics stored in memory internal List GetMetrics() { - return _metricDirective.Metrics; + // Return a snapshot to avoid concurrent modification issues during serialization + lock (_metricDirective._lockObj) + { + return new List(_metricDirective.Metrics); + } } /// diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDefinition.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDefinition.cs index b0804fc06..4c3638f5f 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDefinition.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDefinition.cs @@ -93,6 +93,9 @@ public MetricDefinition(string name, MetricUnit unit, List values, Metri /// Metric value to add to existing key public void AddValue(double value) { - Values.Add(value); + lock (Values) + { + Values.Add(value); + } } } \ No newline at end of file diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs index 99f2d46a2..f390a5bfb 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs @@ -112,12 +112,17 @@ public List> AllDimensionKeys var result = new List>(); var allDimKeys = new List(); + // Create snapshots to avoid concurrent modification issues + var defaultDimensionsSnapshot = new List(DefaultDimensions); + var dimensionsSnapshot = new List(Dimensions); + // Add default dimensions keys - if (DefaultDimensions.Any()) + foreach (var dimensionSet in defaultDimensionsSnapshot) { - foreach (var dimensionSet in DefaultDimensions) + var keysSnapshot = dimensionSet.DimensionKeys; + foreach (var key in keysSnapshot) { - foreach (var key in dimensionSet.DimensionKeys.Where(key => !allDimKeys.Contains(key))) + if (!allDimKeys.Contains(key)) { allDimKeys.Add(key); } @@ -125,17 +130,21 @@ public List> AllDimensionKeys } // Add all regular dimensions to the same array - foreach (var dimensionSet in Dimensions) + foreach (var dimensionSet in dimensionsSnapshot) { - foreach (var key in dimensionSet.DimensionKeys.Where(key => !allDimKeys.Contains(key))) + var keysSnapshot = dimensionSet.DimensionKeys; + foreach (var key in keysSnapshot) { - allDimKeys.Add(key); + if (!allDimKeys.Contains(key)) + { + allDimKeys.Add(key); + } } } // Add non-empty dimension arrays // When no dimensions exist, add an empty array - result.Add(allDimKeys.Any() ? allDimKeys : []); + result.Add(allDimKeys.Count > 0 ? allDimKeys : []); return result; } @@ -144,7 +153,7 @@ public List> AllDimensionKeys /// /// Shared synchronization object /// - private readonly object _lockObj = new(); + internal readonly object _lockObj = new(); /// /// Adds metric to memory @@ -156,27 +165,38 @@ public List> AllDimensionKeys /// Metrics - Cannot add more than 100 metrics at the same time. public void AddMetric(string name, double value, MetricUnit unit, MetricResolution metricResolution) { - if (Metrics.Count < PowertoolsConfigurations.MaxMetrics) + if (string.IsNullOrEmpty(name)) + return; + + lock (_lockObj) { - lock (_lockObj) + if (Metrics.Count >= PowertoolsConfigurations.MaxMetrics) + { + throw new ArgumentOutOfRangeException(nameof(Metrics), + $"Cannot add more than {PowertoolsConfigurations.MaxMetrics} metrics at the same time."); + } + + var metric = GetExistingMetric(Metrics, name); + if (metric != null) { - var metric = GetExistingMetric(Metrics, name); - if (metric != null) + try { - if (metric.Values.Count < PowertoolsConfigurations.MaxMetrics) + if (metric.Values != null && metric.Values.Count < PowertoolsConfigurations.MaxMetrics) metric.AddValue(value); else throw new ArgumentOutOfRangeException(nameof(metric), $"Cannot add more than {PowertoolsConfigurations.MaxMetrics} metric data points at the same time."); } - else + catch (NullReferenceException) + { + // If metric became null due to concurrent access, create a new one Metrics.Add(new MetricDefinition(name, unit, value, metricResolution)); + } + } + else + { + Metrics.Add(new MetricDefinition(name, unit, value, metricResolution)); } - } - else - { - throw new ArgumentOutOfRangeException(nameof(Metrics), - $"Cannot add more than {PowertoolsConfigurations.MaxMetrics} metrics at the same time."); } } @@ -205,37 +225,53 @@ internal void SetService(string service) /// Dimensions - Cannot add more than 9 dimensions at the same time. internal void AddDimension(DimensionSet dimension) { - // Check if we already have any dimensions - if (Dimensions.Count > 0) + if (dimension?.Dimensions == null) + return; + + lock (_lockObj) { - // Get the first dimension set where we now store all dimensions - var firstDimensionSet = Dimensions[0]; - - // Check the actual dimension count inside the first dimension set - if (firstDimensionSet.Dimensions.Count >= PowertoolsConfigurations.MaxDimensions) - { - throw new ArgumentOutOfRangeException(nameof(dimension), - $"Cannot add more than {PowertoolsConfigurations.MaxDimensions} dimensions at the same time."); - } - - // Add to the first dimension set instead of creating a new one - foreach (var pair in dimension.Dimensions) + // Check if we already have any dimensions + if (Dimensions.Count > 0) { - if (!firstDimensionSet.Dimensions.ContainsKey(pair.Key)) + // Get the first dimension set where we now store all dimensions + var firstDimensionSet = Dimensions[0]; + + // Null check for thread safety + if (firstDimensionSet?.Dimensions == null) { - firstDimensionSet.Dimensions.Add(pair.Key, pair.Value); + // If first dimension set is null, replace it with the new one + Dimensions[0] = dimension; + return; } - else + + // Check the actual dimension count inside the first dimension set + if (firstDimensionSet.Dimensions.Count >= PowertoolsConfigurations.MaxDimensions) { - Console.WriteLine( - $"##WARNING##: Failed to Add dimension '{pair.Key}'. Dimension already exists."); + throw new ArgumentOutOfRangeException(nameof(dimension), + $"Cannot add more than {PowertoolsConfigurations.MaxDimensions} dimensions at the same time."); + } + + // Add to the first dimension set instead of creating a new one + // Create a snapshot to avoid concurrent modification issues + var dimensionSnapshot = new Dictionary(dimension.Dimensions); + foreach (var pair in dimensionSnapshot) + { + if (!firstDimensionSet.Dimensions.ContainsKey(pair.Key)) + { + firstDimensionSet.Dimensions.Add(pair.Key, pair.Value); + } + else + { + Console.WriteLine( + $"##WARNING##: Failed to Add dimension '{pair.Key}'. Dimension already exists."); + } } } - } - else - { - // No dimensions yet, add the new one - Dimensions.Add(dimension); + else + { + // No dimensions yet, add the new one + Dimensions.Add(dimension); + } } } @@ -245,12 +281,35 @@ internal void AddDimension(DimensionSet dimension) /// Default dimensions list internal void SetDefaultDimensions(List defaultDimensions) { - if (!DefaultDimensions.Any()) + if (DefaultDimensions.Count == 0) DefaultDimensions = defaultDimensions; else + { foreach (var item in defaultDimensions) - if (!DefaultDimensions.Any(d => d.DimensionKeys.Contains(item.DimensionKeys[0]))) + { + if (item.DimensionKeys.Count == 0) + continue; + + bool exists = false; + var itemFirstKey = item.DimensionKeys[0]; + + foreach (var existing in DefaultDimensions) + { + var existingKeys = existing.DimensionKeys; + for (int i = 0; i < existingKeys.Count; i++) + { + if (existingKeys[i] == itemFirstKey) + { + exists = true; + break; + } + } + if (exists) break; + } + if (!exists) DefaultDimensions.Add(item); + } + } } /// @@ -262,13 +321,29 @@ internal Dictionary ExpandAllDimensionSets() // if a key appears multiple times, the last value will be the one that's used in the output. var dimensions = new Dictionary(); - foreach (var dimensionSet in DefaultDimensions) - foreach (var (key, value) in dimensionSet.Dimensions) - dimensions[key] = value; + // Create snapshots to avoid concurrent modification issues + var defaultDimensionsSnapshot = new List(DefaultDimensions); + var dimensionsSnapshot = new List(Dimensions); + + foreach (var dimensionSet in defaultDimensionsSnapshot) + { + if (dimensionSet?.Dimensions != null) + { + var dimensionSnapshot = new Dictionary(dimensionSet.Dimensions); + foreach (var (key, value) in dimensionSnapshot) + dimensions[key] = value; + } + } - foreach (var dimensionSet in Dimensions) - foreach (var (key, value) in dimensionSet.Dimensions) - dimensions[key] = value; + foreach (var dimensionSet in dimensionsSnapshot) + { + if (dimensionSet?.Dimensions != null) + { + var dimensionSnapshot = new Dictionary(dimensionSet.Dimensions); + foreach (var (key, value) in dimensionSnapshot) + dimensions[key] = value; + } + } return dimensions; } @@ -279,16 +354,19 @@ internal Dictionary ExpandAllDimensionSets() /// List of dimension sets to add internal void AddDimensionSet(List dimensionSets) { - if (dimensionSets == null || !dimensionSets.Any()) + if (dimensionSets == null || dimensionSets.Count == 0) return; if (Dimensions.Count + dimensionSets.Count <= PowertoolsConfigurations.MaxDimensions) { // Simply add the dimension sets without checking for existing keys // This ensures dimensions added together stay together - foreach (var dimensionSet in dimensionSets.Where(dimensionSet => dimensionSet.DimensionKeys.Any())) + foreach (var dimensionSet in dimensionSets) { - Dimensions.Add(dimensionSet); + if (dimensionSet.DimensionKeys.Count > 0) + { + Dimensions.Add(dimensionSet); + } } } else @@ -308,12 +386,21 @@ private static MetricDefinition GetExistingMetric(List metrics { // Use a traditional for loop instead of LINQ to avoid enumeration issues // when the collection is modified concurrently - for (int i = 0; i < metrics.Count; i++) + if (metrics == null || string.IsNullOrEmpty(name)) + return null; + + // Create a snapshot of the count to avoid issues with concurrent modifications + var count = metrics.Count; + for (int i = 0; i < count; i++) { try { + // Check bounds again in case collection was modified + if (i >= metrics.Count) + break; + var metric = metrics[i]; - if (metric?.Name == name) + if (metric != null && string.Equals(metric.Name, name, StringComparison.Ordinal)) { return metric; } @@ -321,7 +408,12 @@ private static MetricDefinition GetExistingMetric(List metrics catch (ArgumentOutOfRangeException) { // Collection was modified during iteration, return null to be safe - return null; + break; + } + catch (IndexOutOfRangeException) + { + // Collection was modified during iteration, return null to be safe + break; } } return null; diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs index 91ccdc9f8..82264d75e 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs @@ -14,6 +14,7 @@ */ using System.Collections.Generic; +using System.Linq; using System.Text.Json; using System.Text.Json.Serialization; @@ -42,15 +43,25 @@ public Dictionary MetricData { var targetMembers = new Dictionary(); - foreach (var dimension in AWS.ExpandAllDimensionSets()) targetMembers.Add(dimension.Key, dimension.Value); + // Create snapshots to avoid concurrent modification issues + var dimensionsSnapshot = AWS.ExpandAllDimensionSets(); + foreach (var dimension in dimensionsSnapshot) + targetMembers.Add(dimension.Key, dimension.Value); - foreach (var metricDefinition in AWS.GetMetrics()) + var metricsSnapshot = new List(AWS.GetMetrics()); + foreach (var metricDefinition in metricsSnapshot) { - var values = metricDefinition.Values; + List values; + lock (metricDefinition.Values) + { + values = new List(metricDefinition.Values); + } targetMembers.Add(metricDefinition.Name, values.Count == 1 ? values[0] : values); } - foreach (var metadata in AWS.CustomMetadata) targetMembers.TryAdd(metadata.Key, metadata.Value); + var metadataSnapshot = new Dictionary(AWS.CustomMetadata); + foreach (var metadata in metadataSnapshot) + targetMembers.TryAdd(metadata.Key, metadata.Value); return targetMembers; } @@ -65,6 +76,68 @@ public string Serialize() { if (string.IsNullOrWhiteSpace(AWS.GetNamespace())) throw new SchemaValidationException("namespace"); - return JsonSerializer.Serialize(this, typeof(RootNode), MetricsSerializationContext.Default); + // Create a complete snapshot for serialization to avoid concurrent modification issues + var snapshot = CreateSerializationSnapshot(); + return JsonSerializer.Serialize(snapshot, typeof(RootNode), MetricsSerializationContext.Default); + } + + /// + /// Creates a complete snapshot of the current state for thread-safe serialization + /// + /// A snapshot RootNode with all data copied + private RootNode CreateSerializationSnapshot() + { + var snapshot = new RootNode(); + + // Copy namespace + snapshot.AWS.SetNamespace(AWS.GetNamespace()); + + // Copy service if set + if (!string.IsNullOrEmpty(AWS.GetService())) + { + snapshot.AWS.SetService(AWS.GetService()); + } + + // Copy metrics with their values + var metricsSnapshot = AWS.GetMetrics(); + foreach (var metric in metricsSnapshot) + { + List valuesCopy; + lock (metric.Values) + { + valuesCopy = new List(metric.Values); + } + + // Add each value individually to ensure proper metric creation + foreach (var value in valuesCopy) + { + snapshot.AWS.AddMetric(metric.Name, value, metric.Unit, metric.StorageResolution); + } + } + + // Copy dimensions + var dimensionsSnapshot = AWS.ExpandAllDimensionSets(); + if (dimensionsSnapshot.Count > 0) + { + // Create dimension set with first key-value pair, then add the rest + var firstKvp = dimensionsSnapshot.First(); + var dimensionSet = new DimensionSet(firstKvp.Key, firstKvp.Value); + + // Add remaining dimensions + foreach (var kvp in dimensionsSnapshot.Skip(1)) + { + dimensionSet.Dimensions[kvp.Key] = kvp.Value; + } + snapshot.AWS.AddDimension(dimensionSet); + } + + // Copy custom metadata + var metadataSnapshot = new Dictionary(AWS.CustomMetadata); + foreach (var kvp in metadataSnapshot) + { + snapshot.AWS.AddMetadata(kvp.Key, kvp.Value); + } + + return snapshot; } } \ No newline at end of file diff --git a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/SerializationThreadingTests.cs b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/SerializationThreadingTests.cs new file mode 100644 index 000000000..13e464ddf --- /dev/null +++ b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/SerializationThreadingTests.cs @@ -0,0 +1,261 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace AWS.Lambda.Powertools.Metrics.Tests +{ + public class SerializationThreadingTests : IDisposable + { + [Fact] + public async Task Serialize_ConcurrentWithAddMetric_ShouldNotThrowArgumentOutOfRangeException() + { + var exceptions = new List(); + var exceptionsLock = new object(); + var operationCount = 0; + var serializationCount = 0; + var cancellationTokenSource = new CancellationTokenSource(); + + try + { + Metrics.ResetForTest(); + Metrics.SetNamespace("SerializationTest"); + + // Create tasks that continuously add metrics + var metricTasks = new List(); + for (int i = 0; i < 10; i++) + { + int threadId = i; + metricTasks.Add(Task.Run(async () => + { + while (!cancellationTokenSource.Token.IsCancellationRequested) + { + try + { + // Add various types of metrics to create complex scenarios + Metrics.AddMetric("SharedMetric", threadId * 1.5, MetricUnit.Count); + Metrics.AddMetric($"ThreadMetric_{threadId}", threadId, MetricUnit.Milliseconds); + Metrics.AddMetric("AnotherSharedMetric", threadId * 2.0, MetricUnit.Bytes); + + // Add dimensions occasionally to make serialization more complex + if (threadId % 3 == 0) + { + Metrics.AddDimension("ThreadId", threadId.ToString()); + Metrics.AddDimension("Operation", "Test"); + } + + Interlocked.Increment(ref operationCount); + + // Small delay to allow other threads to interleave + await Task.Delay(1, cancellationTokenSource.Token); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + lock (exceptionsLock) + { + exceptions.Add(ex); + } + return; + } + } + }, cancellationTokenSource.Token)); + } + + // Create tasks that continuously serialize metrics + var serializationTasks = new List(); + for (int i = 0; i < 5; i++) + { + serializationTasks.Add(Task.Run(async () => + { + while (!cancellationTokenSource.Token.IsCancellationRequested) + { + try + { + // This is where the bug occurred - during serialization + // while other threads were modifying the collections + // Flush() internally calls Serialize() which is where the race condition happened + Metrics.Flush(); + + Interlocked.Increment(ref serializationCount); + + // Small delay to allow metric additions to interleave + await Task.Delay(2, cancellationTokenSource.Token); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + lock (exceptionsLock) + { + exceptions.Add(ex); + } + return; + } + } + }, cancellationTokenSource.Token)); + } + + // Let the test run for a reasonable amount of time to stress test the system + await Task.Delay(2000); // 2 seconds of concurrent operations + + // Stop all tasks + cancellationTokenSource.Cancel(); + + // Wait for all tasks to complete + try + { + await Task.WhenAll(metricTasks.Concat(serializationTasks)); + } + catch (OperationCanceledException) + { + // Expected when cancellation is requested + } + } + finally + { + cancellationTokenSource.Dispose(); + } + + // Assert that no threading exceptions occurred + lock (exceptionsLock) + { + if (exceptions.Count > 0) + { + var firstException = exceptions[0]; + + // Check specifically for the types of exceptions that indicate threading issues + if (firstException is ArgumentOutOfRangeException || + firstException is InvalidOperationException || + firstException is NullReferenceException || + firstException.Message.Contains("Collection was modified") || + firstException.Message.Contains("Index was out of range") || + firstException.Message.Contains("enumeration")) + { + Assert.Fail( + $"Threading-related exception occurred during concurrent serialization: " + + $"{firstException.GetType().Name}: {firstException.Message}\n" + + $"Stack trace: {firstException.StackTrace}"); + } + else + { + // Re-throw unexpected exceptions + throw firstException; + } + } + } + + // Verify that we actually performed concurrent operations + Assert.True(operationCount > 100, + $"Expected many metric operations, but only got {operationCount}"); + Assert.True(serializationCount > 10, + $"Expected many serialization operations, but only got {serializationCount}"); + } + + [Fact] + public async Task Flush_ConcurrentWithAddMetric_ShouldNotThrowArgumentOutOfRangeException() + { + var exceptions = new List(); + var exceptionsLock = new object(); + var cancellationTokenSource = new CancellationTokenSource(); + + try + { + Metrics.ResetForTest(); + Metrics.SetNamespace("FlushTest"); + + var tasks = new List(); + + // Tasks that add metrics + for (int i = 0; i < 8; i++) + { + int threadId = i; + tasks.Add(Task.Run(async () => + { + while (!cancellationTokenSource.Token.IsCancellationRequested) + { + try + { + Metrics.AddMetric($"Metric_{threadId}", threadId, MetricUnit.Count); + await Task.Delay(5, cancellationTokenSource.Token); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + lock (exceptionsLock) + { + exceptions.Add(ex); + } + return; + } + } + }, cancellationTokenSource.Token)); + } + + // Tasks that flush metrics (which triggers serialization) + for (int i = 0; i < 3; i++) + { + tasks.Add(Task.Run(async () => + { + while (!cancellationTokenSource.Token.IsCancellationRequested) + { + try + { + Metrics.Flush(); + await Task.Delay(10, cancellationTokenSource.Token); + } + catch (Exception ex) when (!(ex is OperationCanceledException)) + { + lock (exceptionsLock) + { + exceptions.Add(ex); + } + return; + } + } + }, cancellationTokenSource.Token)); + } + + // Run for 1 second + await Task.Delay(1000); + cancellationTokenSource.Cancel(); + + try + { + await Task.WhenAll(tasks); + } + catch (OperationCanceledException) + { + // Expected + } + } + finally + { + cancellationTokenSource.Dispose(); + } + + // Check for threading exceptions + lock (exceptionsLock) + { + if (exceptions.Count > 0) + { + var firstException = exceptions[0]; + if (firstException is ArgumentOutOfRangeException || + firstException is InvalidOperationException || + firstException.Message.Contains("Index was out of range")) + { + Assert.Fail( + $"Threading exception in Flush: {firstException.GetType().Name}: {firstException.Message}"); + } + else + { + throw firstException; + } + } + } + } + + public void Dispose() + { + Metrics.ResetForTest(); + } + } +} \ No newline at end of file diff --git a/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ThreadingTests.cs b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ThreadingTests.cs new file mode 100644 index 000000000..6bd81acd2 --- /dev/null +++ b/libraries/tests/AWS.Lambda.Powertools.Metrics.Tests/ThreadingTests.cs @@ -0,0 +1,117 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace AWS.Lambda.Powertools.Metrics.Tests +{ + public class ThreadingTests : IDisposable + { + [Fact] + public async Task AddMetric_ConcurrentSameKey_VerifiesThreadSafety() + { + var exceptionThrown = false; + var exception = (Exception)null; + var operationCount = 0; + var lockObject = new object(); + + try + { + Metrics.ResetForTest(); + Metrics.SetNamespace("Test"); + + // Use many threads to stress test the system + var tasks = new Task[50]; + + for (int i = 0; i < 50; i++) + { + int threadId = i; + tasks[i] = Task.Run(() => + { + for (int j = 0; j < 100; j++) + { + try + { + // Mix of same and different keys to trigger both code paths + if (j % 3 == 0) + { + // This now uses GetExistingMetric instead of FirstOrDefault + Metrics.AddMetric("SharedKey", 1.0, MetricUnit.Count); + } + else if (j % 7 == 0) + { + // Different key to force collection modifications + Metrics.AddMetric($"Key{threadId}_{j}", 1.0, MetricUnit.Count); + } + else + { + // Back to shared key + Metrics.AddMetric("SharedKey", 2.0, MetricUnit.Count); + } + + Interlocked.Increment(ref operationCount); + + // Occasionally flush to trigger more collection modifications + if (j % 25 == 0) + { + Metrics.Flush(); + } + } + catch (Exception ex) + { + // Capture any threading-related exceptions that shouldn't occur now + if (ex is NullReferenceException || + ex is InvalidOperationException || + ex.Message.Contains("Collection was modified") || + ex.Message.Contains("enumeration")) + { + lock (lockObject) + { + if (!exceptionThrown) + { + exceptionThrown = true; + exception = ex; + } + } + return; // Exit this thread + } + throw; // Re-throw other exceptions + } + } + }); + } + + await Task.WhenAll(tasks); + } + catch (Exception ex) + { + // Capture any threading-related exceptions at the task level + if (ex is NullReferenceException || + ex is InvalidOperationException || + ex.Message.Contains("modified") || + ex.Message.Contains("enumeration")) + { + exceptionThrown = true; + exception = ex; + } + else + { + throw; // Re-throw unexpected exceptions + } + } + + // Assert that the threading bug has been fixed - no exceptions should occur + Assert.False(exceptionThrown, + $"Threading exception occurred, indicating the bug may not be fully fixed: {exception?.GetType().Name}: {exception?.Message}"); + + // Verify that we successfully performed many concurrent operations + Assert.True(operationCount > 1000, + $"Expected many successful operations, but only got {operationCount}"); + } + + public void Dispose() + { + Metrics.ResetForTest(); + } + } +} \ No newline at end of file From f96c1d9f98e79020810c7159fcaa52b253172702 Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Mon, 22 Sep 2025 20:41:36 +0100 Subject: [PATCH 6/7] add null defense mechanisms --- .../Model/RootNode.cs | 89 ++++++++++++------- 1 file changed, 58 insertions(+), 31 deletions(-) diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs index 82264d75e..8822d5922 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/RootNode.cs @@ -44,24 +44,36 @@ public Dictionary MetricData var targetMembers = new Dictionary(); // Create snapshots to avoid concurrent modification issues - var dimensionsSnapshot = AWS.ExpandAllDimensionSets(); - foreach (var dimension in dimensionsSnapshot) - targetMembers.Add(dimension.Key, dimension.Value); + var dimensionsSnapshot = AWS?.ExpandAllDimensionSets(); + if (dimensionsSnapshot != null) + { + foreach (var dimension in dimensionsSnapshot) + targetMembers.Add(dimension.Key, dimension.Value); + } - var metricsSnapshot = new List(AWS.GetMetrics()); - foreach (var metricDefinition in metricsSnapshot) + var metricsSnapshot = AWS?.GetMetrics(); + if (metricsSnapshot != null) { - List values; - lock (metricDefinition.Values) + foreach (var metricDefinition in metricsSnapshot) { - values = new List(metricDefinition.Values); + if (metricDefinition?.Values != null) + { + List values; + lock (metricDefinition.Values) + { + values = new List(metricDefinition.Values); + } + targetMembers.Add(metricDefinition.Name, values.Count == 1 ? values[0] : values); + } } - targetMembers.Add(metricDefinition.Name, values.Count == 1 ? values[0] : values); } - var metadataSnapshot = new Dictionary(AWS.CustomMetadata); - foreach (var metadata in metadataSnapshot) - targetMembers.TryAdd(metadata.Key, metadata.Value); + var metadataSnapshot = AWS?.CustomMetadata; + if (metadataSnapshot != null) + { + foreach (var metadata in metadataSnapshot) + targetMembers.TryAdd(metadata.Key, metadata.Value); + } return targetMembers; } @@ -90,34 +102,45 @@ private RootNode CreateSerializationSnapshot() var snapshot = new RootNode(); // Copy namespace - snapshot.AWS.SetNamespace(AWS.GetNamespace()); + var namespaceValue = AWS?.GetNamespace(); + if (!string.IsNullOrWhiteSpace(namespaceValue)) + { + snapshot.AWS.SetNamespace(namespaceValue); + } // Copy service if set - if (!string.IsNullOrEmpty(AWS.GetService())) + var serviceValue = AWS?.GetService(); + if (!string.IsNullOrEmpty(serviceValue)) { - snapshot.AWS.SetService(AWS.GetService()); + snapshot.AWS.SetService(serviceValue); } // Copy metrics with their values - var metricsSnapshot = AWS.GetMetrics(); - foreach (var metric in metricsSnapshot) + var metricsSnapshot = AWS?.GetMetrics(); + if (metricsSnapshot != null) { - List valuesCopy; - lock (metric.Values) + foreach (var metric in metricsSnapshot) { - valuesCopy = new List(metric.Values); - } - - // Add each value individually to ensure proper metric creation - foreach (var value in valuesCopy) - { - snapshot.AWS.AddMetric(metric.Name, value, metric.Unit, metric.StorageResolution); + if (metric?.Values != null) + { + List valuesCopy; + lock (metric.Values) + { + valuesCopy = new List(metric.Values); + } + + // Add each value individually to ensure proper metric creation + foreach (var value in valuesCopy) + { + snapshot.AWS.AddMetric(metric.Name, value, metric.Unit, metric.StorageResolution); + } + } } } // Copy dimensions - var dimensionsSnapshot = AWS.ExpandAllDimensionSets(); - if (dimensionsSnapshot.Count > 0) + var dimensionsSnapshot = AWS?.ExpandAllDimensionSets(); + if (dimensionsSnapshot != null && dimensionsSnapshot.Count > 0) { // Create dimension set with first key-value pair, then add the rest var firstKvp = dimensionsSnapshot.First(); @@ -132,10 +155,14 @@ private RootNode CreateSerializationSnapshot() } // Copy custom metadata - var metadataSnapshot = new Dictionary(AWS.CustomMetadata); - foreach (var kvp in metadataSnapshot) + var customMetadata = AWS?.CustomMetadata; + if (customMetadata != null) { - snapshot.AWS.AddMetadata(kvp.Key, kvp.Value); + var metadataSnapshot = new Dictionary(customMetadata); + foreach (var kvp in metadataSnapshot) + { + snapshot.AWS.AddMetadata(kvp.Key, kvp.Value); + } } return snapshot; From 6742670db23340deaa8dbdaf82c5b4c47d1c666a Mon Sep 17 00:00:00 2001 From: Henrique Graca <999396+hjgraca@users.noreply.github.com> Date: Mon, 22 Sep 2025 20:55:26 +0100 Subject: [PATCH 7/7] Removed catch (NullReferenceException) - --- .../Model/MetricDirective.cs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs index f390a5bfb..2cfc1570d 100644 --- a/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs +++ b/libraries/src/AWS.Lambda.Powertools.Metrics/Model/MetricDirective.cs @@ -177,24 +177,21 @@ public void AddMetric(string name, double value, MetricUnit unit, MetricResoluti } var metric = GetExistingMetric(Metrics, name); - if (metric != null) + if (metric?.Values != null) { - try + if (metric.Values.Count < PowertoolsConfigurations.MaxMetrics) { - if (metric.Values != null && metric.Values.Count < PowertoolsConfigurations.MaxMetrics) - metric.AddValue(value); - else - throw new ArgumentOutOfRangeException(nameof(metric), - $"Cannot add more than {PowertoolsConfigurations.MaxMetrics} metric data points at the same time."); + metric.AddValue(value); } - catch (NullReferenceException) + else { - // If metric became null due to concurrent access, create a new one - Metrics.Add(new MetricDefinition(name, unit, value, metricResolution)); + throw new ArgumentOutOfRangeException(nameof(metric), + $"Cannot add more than {PowertoolsConfigurations.MaxMetrics} metric data points at the same time."); } } else { + // Either no existing metric found or metric/Values became null due to concurrent access Metrics.Add(new MetricDefinition(name, unit, value, metricResolution)); } }