Avoid buffer race conditions in CGroups (#5129)

* Correct tests decorations * Avoid buffer race conditions in CGroups Fixes #5114
dotnet · May 28, 2024 · a06edbd · a06edbd
1 parent 625bb60
commit a06edbd
Show file tree

Hide file tree

Showing 8 changed files with 314 additions and 180 deletions.
diff --git a/...crosoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs b/...crosoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV1.cs
diff --git a/...crosoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV2.cs b/...crosoft.Extensions.Diagnostics.ResourceMonitoring/Linux/LinuxUtilizationParserCgroupV2.cs
diff --git a/src/Shared/BufferWriterPool/ReturnableBufferWriter.cs b/src/Shared/BufferWriterPool/ReturnableBufferWriter.cs
@@ -0,0 +1,45 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using Microsoft.Extensions.ObjectPool;
+
+#pragma warning disable CA1716
+namespace Microsoft.Shared.Pools;
+#pragma warning restore CA1716
+
+/// <summary>
+///  Represents a buffer writer that can be automatically returned to an object pool upon dispose.
+/// </summary>
+/// <typeparam name="T">The type of the elements in the buffer.</typeparam>
+#if !SHARED_PROJECT
+[System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
+#endif
+internal readonly struct ReturnableBufferWriter<T> : IDisposable
+{
+    private readonly ObjectPool<BufferWriter<T>> _pool;
+
+    /// <summary>
+    ///  Initializes a new instance of the <see cref="ReturnableBufferWriter{T}"/> struct.
+    /// </summary>
+    /// <param name="pool">The object pool to return the buffer writer to.</param>
+    public ReturnableBufferWriter(ObjectPool<BufferWriter<T>> pool)
+    {
+        _pool = pool;
+        Buffer = pool.Get();
+    }
+
+    /// <summary>
+    ///  Gets the buffer writer.
+    /// </summary>
+    public BufferWriter<T> Buffer { get; }
+
+    /// <summary>
+    ///  Disposes the buffer writer and returns it to the object pool.
+    /// </summary>
+    public readonly void Dispose()
+    {
+        Buffer.Reset();
+        _pool.Return(Buffer);
+    }
+}
diff --git a/...braries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs b/...braries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs
@@ -24,7 +24,7 @@ namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
 public sealed class AcceptanceTest
 {
     [ConditionalFact]
-    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package.")]
+    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
     public void Adding_Linux_Resource_Utilization_Allows_To_Query_Snapshot_Provider()
     {
         using var services = new ServiceCollection()
@@ -38,7 +38,7 @@ public void Adding_Linux_Resource_Utilization_Allows_To_Query_Snapshot_Provider(
     }
 
     [ConditionalFact]
-    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package.")]
+    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
     [SuppressMessage("Minor Code Smell", "S3257:Declarations and initializations should be as concise as possible", Justification = "Broken analyzer.")]
     public void Adding_Linux_Resource_Utilization_Can_Be_Configured_With_Section()
     {
@@ -68,7 +68,7 @@ public void Adding_Linux_Resource_Utilization_Can_Be_Configured_With_Section()
     }
 
     [ConditionalFact]
-    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package.")]
+    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
     public void Adding_Linux_Resource_Utilization_Can_Be_Configured_With_Action()
     {
         var cpuRefresh = TimeSpan.FromMinutes(13);
@@ -90,7 +90,7 @@ public void Adding_Linux_Resource_Utilization_Can_Be_Configured_With_Action()
     }
 
     [ConditionalFact]
-    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package.")]
+    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
     [SuppressMessage("Minor Code Smell", "S3257:Declarations and initializations should be as concise as possible", Justification = "Broken analyzer.")]
     public void Adding_Linux_Resource_Utilization_With_Section_Registers_SnapshotProvider_Cgroupv1()
     {
@@ -139,7 +139,7 @@ public void Adding_Linux_Resource_Utilization_With_Section_Registers_SnapshotPro
     }
 
     [ConditionalFact]
-    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package.")]
+    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
     [SuppressMessage("Minor Code Smell", "S3257:Declarations and initializations should be as concise as possible", Justification = "Broken analyzer.")]
     public void Adding_Linux_Resource_Utilization_With_Section_Registers_SnapshotProvider_Cgroupv2()
     {
@@ -187,7 +187,7 @@ public void Adding_Linux_Resource_Utilization_With_Section_Registers_SnapshotPro
     }
 
     [ConditionalFact(Skip = "Flaky test, see https://github.com/dotnet/extensions/issues/3997")]
-    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package.")]
+    [OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
     public Task ResourceUtilizationTracker_Reports_The_Same_Values_As_One_Can_Observe_From_Gauges()
     {
         var cpuRefresh = TimeSpan.FromMinutes(13);

diff --git a/...ies/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/LinuxCountersTests.cs b/...ies/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/LinuxCountersTests.cs
@@ -11,7 +11,7 @@
 
 namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
 
-[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific package")]
+[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
 public sealed class LinuxCountersTests
 {
     [ConditionalFact]

diff --git a/...ests/Linux/LinuxUtilizationParserTests.cs → ...ux/LinuxUtilizationParserCgroupV1Tests.cs b/...ests/Linux/LinuxUtilizationParserTests.cs → ...ux/LinuxUtilizationParserCgroupV1Tests.cs
@@ -6,14 +6,16 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
-using Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
+using System.Threading.Tasks;
+using Microsoft.Shared.Pools;
 using Microsoft.TestUtilities;
+using Moq;
 using Xunit;
 
 namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
 
-[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Windows specific.")]
-public sealed class LinuxUtilizationParserTests
+[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
+public sealed class LinuxUtilizationParserCgroupV1Tests
 {
     [ConditionalTheory]
     [InlineData("DFIJEUWGHFWGBWEFWOMDOWKSLA")]
@@ -348,4 +350,46 @@ public void Parser_Throws_When_Cgroup_Cpu_Shares_Files_Contain_Invalid_Data(stri
         Assert.IsAssignableFrom<InvalidOperationException>(r);
         Assert.Contains("/sys/fs/cgroup/cpu/cpu.shares", r.Message);
     }
+
+    [ConditionalFact]
+    public async Task ThreadSafetyAsync()
+    {
+        var f1 = new HardcodedValueFileSystem(new Dictionary<FileInfo, string>
+        {
+            { new FileInfo("/proc/stat"), "cpu  6163 0 3853 4222848 614 0 1155 0 0 0\r\ncpu0 240 0 279 210987 59 0 927 0 0 0" },
+        });
+        var f2 = new HardcodedValueFileSystem(new Dictionary<FileInfo, string>
+        {
+            { new FileInfo("/proc/stat"), "cpu  9137 0 9296 13972503 1148 0 2786 0 0 0\r\ncpu0 297 0 431 698663 59 0 2513 0 0 0" },
+        });
+
+        int callCount = 0;
+        Mock<IFileSystem> fs = new();
+        fs.Setup(x => x.ReadFirstLine(It.IsAny<FileInfo>(), It.IsAny<BufferWriter<char>>()))
+             .Callback<FileInfo, BufferWriter<char>>((fileInfo, buffer) =>
+             {
+                 callCount++;
+                 if (callCount % 2 == 0)
+                 {
+                     f1.ReadFirstLine(fileInfo, buffer);
+                 }
+                 else
+                 {
+                     f2.ReadFirstLine(fileInfo, buffer);
+                 }
+             })
+             .Verifiable();
+
+        var p = new LinuxUtilizationParserCgroupV1(fs.Object, new FakeUserHz(100));
+
+        Task[] tasks = new Task[1_000];
+        for (int i = 0; i < tasks.Length; i++)
+        {
+            tasks[i] = Task.Run(p.GetHostCpuUsageInNanoseconds);
+        }
+
+        await Task.WhenAll(tasks);
+
+        Assert.True(true);
+    }
 }
diff --git a/...ensions.Diagnostics.ResourceMonitoring.Tests/Linux/LinuxUtilizationParserCgroupV2Tests.cs b/...ensions.Diagnostics.ResourceMonitoring.Tests/Linux/LinuxUtilizationParserCgroupV2Tests.cs
@@ -6,13 +6,16 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
+using System.Threading.Tasks;
 using Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
+using Microsoft.Shared.Pools;
 using Microsoft.TestUtilities;
+using Moq;
 using Xunit;
 
 namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
 
-[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Windows specific.")]
+[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
 public sealed class LinuxUtilizationParserCgroupV2Tests
 {
     [ConditionalTheory]
@@ -425,4 +428,46 @@ public void Parser_Throws_When_Cgroup_Cpu_Weight_Files_Contain_Invalid_Data(stri
         Assert.IsAssignableFrom<InvalidOperationException>(r);
         Assert.Contains("/sys/fs/cgroup/cpu.weight", r.Message);
     }
+
+    [ConditionalFact]
+    public async Task ThreadSafetyAsync()
+    {
+        var f1 = new HardcodedValueFileSystem(new Dictionary<FileInfo, string>
+        {
+            { new FileInfo("/proc/stat"), "cpu  6163 0 3853 4222848 614 0 1155 0 0 0\r\ncpu0 240 0 279 210987 59 0 927 0 0 0" },
+        });
+        var f2 = new HardcodedValueFileSystem(new Dictionary<FileInfo, string>
+        {
+            { new FileInfo("/proc/stat"), "cpu  9137 0 9296 13972503 1148 0 2786 0 0 0\r\ncpu0 297 0 431 698663 59 0 2513 0 0 0" },
+        });
+
+        int callCount = 0;
+        Mock<IFileSystem> fs = new();
+        fs.Setup(x => x.ReadFirstLine(It.IsAny<FileInfo>(), It.IsAny<BufferWriter<char>>()))
+             .Callback<FileInfo, BufferWriter<char>>((fileInfo, buffer) =>
+             {
+                 callCount++;
+                 if (callCount % 2 == 0)
+                 {
+                     f1.ReadFirstLine(fileInfo, buffer);
+                 }
+                 else
+                 {
+                     f2.ReadFirstLine(fileInfo, buffer);
+                 }
+             })
+             .Verifiable();
+
+        var p = new LinuxUtilizationParserCgroupV2(fs.Object, new FakeUserHz(100));
+
+        Task[] tasks = new Task[1_000];
+        for (int i = 0; i < tasks.Length; i++)
+        {
+            tasks[i] = Task.Run(p.GetHostCpuUsageInNanoseconds);
+        }
+
+        await Task.WhenAll(tasks);
+
+        Assert.True(true);
+    }
 }
diff --git a/...ries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/OSFileSystemTests.cs b/...ries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/OSFileSystemTests.cs
@@ -9,7 +9,7 @@
 
 namespace Microsoft.Extensions.Diagnostics.ResourceMonitoring.Linux.Test;
 
-[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Windows specific.")]
+[OSSkipCondition(OperatingSystems.Windows | OperatingSystems.MacOSX, SkipReason = "Linux specific tests")]
 public sealed class OSFileSystemTests
 {
     [ConditionalFact]