Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid buffer race conditions in CGroups #5129

Merged
merged 2 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

45 changes: 45 additions & 0 deletions src/Shared/BufferWriterPool/ReturnableBufferWriter.cs
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
{
Expand Down Expand Up @@ -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);
Expand All @@ -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()
{
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down