MetricPusher race condition makes pushing final state of metrics unreliable

jomadu commented 1 year ago

Bug Report

What did you do?

What did you expect to see?

I expected to see that the final state of my metrics were published reliably when I called MetricPusher.StopAsync().

What did you see instead? Under which circumstances?

Main thread creates a metric
Main thread creates a MetricPusher with a specific intervalMilliseconds
Main thread calls MetricPusher.Start()
Main thread awaits a task that completes in the same duration as the specified intervalMilliseconds
Main thread updates the metric, and immediately attempts to stop the MetricPusher
The main thread calling MetricPusher.StopAsync() cancels the MetricPusher's cancellation token while the MetricPusher is publishing the previous state of metrics.
MetricPusher then checks the cancellation token, and seeing it cancelled, exits without publishing the updated metrics

Testing

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <TargetFramework>net6.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <ProduceReferenceAssembly>false</ProduceReferenceAssembly>
        <IsPackable>false</IsPackable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="FluentAssertions" Version="5.10.3" />
        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0" />
        <PackageReference Include="prometheus-net" Version="7.0.0" />
        <PackageReference Include="xunit" Version="2.4.1" />
        <PackageReference Include="xunit.runner.visualstudio" Version="2.4.3">
            <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
            <PrivateAssets>all</PrivateAssets>
        </PackageReference>
        <PackageReference Include="coverlet.collector" Version="3.1.0">
            <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
            <PrivateAssets>all</PrivateAssets>
        </PackageReference>
    </ItemGroup>
</Project>

using FluentAssertions;
using Prometheus;
using Xunit;

namespace Tests;

public class MetricPusherRaceConditionTests
{
    private const string PushGatewayEndpoint = "http://localhost:9091";
    private HttpClient _client;

    /*
     * Manual Setup:
     *
     * > docker run -d -p 9091:9091 prom/pushgateway --web.enable-admin-api
     * 
     */

    public MetricPusherRaceConditionTests()
    {
        _client = new HttpClient() { BaseAddress = new Uri(PushGatewayEndpoint) };
        // Wipe all stored metrics to avoid prior test state from interfering 
        using var resp = _client.PutAsync("api/v1/admin/wipe", null).GetAwaiter().GetResult();
        resp.EnsureSuccessStatusCode();

        Metrics.SuppressDefaultMetrics();
    }

    [Fact]
    public async Task GivenTaskCompletesDuringPublishingWhenMetricPusherIsStoppedThenFinalMetricStateIsntPublished()
    {
        // arrange
        using var resp = await _client.GetAsync("/metrics");
        resp.EnsureSuccessStatusCode();
        var content = await resp.Content.ReadAsStringAsync();
        content.Should().NotContain("test_counter");

        var counter = Metrics.CreateCounter("test_counter", "help");
        var metricPusher = new MetricPusher($"{PushGatewayEndpoint}/metrics", "test_job", null, intervalMilliseconds: 500);
        metricPusher.Start();

        // act
        await Task.Delay(500);
        counter.Inc();
        try
        {
            await metricPusher.StopAsync();
        }
        catch (OperationCanceledException) { }

        // assert (last state of metric shouldn't be published)
        using var resp2 = await _client.GetAsync("/metrics");
        resp2.EnsureSuccessStatusCode();
        content = await resp2.Content.ReadAsStringAsync();
        content.Should().NotContain(@"test_counter{instance="""",job=""test_job""} 1");
    }

    [Fact]
    public async Task GivenTaskCompletesNotDuringPublishingWhenMetricPusherIsStoppedThenFinalMetricStateIsPublished()
    {
        // arrange
        using var resp = await _client.GetAsync("/metrics");
        resp.EnsureSuccessStatusCode();
        var content = await resp.Content.ReadAsStringAsync();
        content.Should().NotContain("test_counter");

        var counter = Metrics.CreateCounter("test_counter", "help");
        var metricPusher = new MetricPusher($"{PushGatewayEndpoint}/metrics", "test_job", null, intervalMilliseconds: 500);
        metricPusher.Start();

        // act
        await Task.Delay(250);
        counter.Inc();
        try
        {
            await metricPusher.StopAsync();
        }
        catch (OperationCanceledException) { }

       // assert (last state of metric shouldn't be published)
        using var resp2 = await _client.GetAsync("/metrics");
        resp2.EnsureSuccessStatusCode();
        content = await resp2.Content.ReadAsStringAsync();
        content.Should().Contain(@"test_counter{instance="""",job=""test_job""} 1");
    }
}

Results for GivenTaskCompletesDuringPublishingWhenMetricPusherIsStoppedThenFinalMzetricStateIsntPublished:

Results for GivenTaskCompletesNotDuringPublishingWhenMetricPusherIsStoppedThenFinalMetricStateIsPublished:

Environment

System information:
```
Windows
```
Pushgateway version:
```
7.0.0
```

jomadu commented 1 year ago

I was also having some trouble building the test project on my own windows machine, targeting net6.0. Otherwise I would have added a set of tests in the netcore testing project to demonstrate more effectively.

Some output of me trying to build:

C:\dev\prometheus-net [master ≡]> git log -1
commit d8a4c95598683edc39138ce3451dc00f7d5d36b2 (HEAD -> master, origin/master, origin/HEAD)
Author: Sander Saares <sasaares@microsoft.com>
Date:   Thu Oct 27 02:31:01 2022

    Add .NET Standard sample project
C:\dev\prometheus-net [master ≡]> git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
C:\dev\prometheus-net [master ≡]> dotnet build
Microsoft (R) Build Engine version 17.1.1+a02f73656 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
C:\dev\prometheus-net\Tester.NetFramework.AspNet\Tester.NetFramework.AspNet.csproj(113,3): error MSB4019: The imported project "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" was not found. Confirm that the expression in the Import declaration "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" is correct, and that the file exists on disk.
C:\dev\prometheus-net\Sample.Web.NetFramework\Sample.Web.NetFramework.csproj(210,3): error MSB4019: The imported project "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" was not found. Confirm that the expression in the Import declaration "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" is correct, and that the file exists on disk.
  Sample.Grpc.Client -> C:\dev\prometheus-net\Sample.Grpc.Client\bin\Debug\net6.0\Sample.Grpc.Client.dll
C:\dev\prometheus-net\Prometheus\LabelSequence.cs(27,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(154,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0843: Auto-implemented property 'StringSequence.Length' must be fully assigned before control is returned to the caller. [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._values' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._inheritedValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._hashCode' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedInheritedArrays' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInCurrentArray' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\LabelSequence.cs(27,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(154,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0843: Auto-implemented property 'StringSequence.Length' must be fully assigned before control is returned to the caller. [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._values' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._inheritedValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._hashCode' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedInheritedArrays' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInCurrentArray' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\LabelSequence.cs(27,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedInheritedArrays' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInCurrentArray' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(154,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0843: Auto-implemented property 'StringSequence.Length' must be fully assigned before control is returned to the caller. [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._values' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._inheritedValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._hashCode' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]

Build FAILED.

C:\dev\prometheus-net\Tester.NetFramework.AspNet\Tester.NetFramework.AspNet.csproj(113,3): error MSB4019: The imported project "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" was not found. Confirm that the expression in the Import declaration "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" is correct, and that the file exists on disk.
C:\dev\prometheus-net\Sample.Web.NetFramework\Sample.Web.NetFramework.csproj(210,3): error MSB4019: The imported project "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" was not found. Confirm that the expression in the Import declaration "C:\Program Files\dotnet\sdk\6.0.203\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" is correct, and that the file exists on disk.
C:\dev\prometheus-net\Prometheus\LabelSequence.cs(27,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(154,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0843: Auto-implemented property 'StringSequence.Length' must be fully assigned before control is returned to the caller. [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._values' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._inheritedValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._hashCode' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedInheritedArrays' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInCurrentArray' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\LabelSequence.cs(27,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(154,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0843: Auto-implemented property 'StringSequence.Length' must be fully assigned before control is returned to the caller. [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._values' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._inheritedValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._hashCode' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedInheritedArrays' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInCurrentArray' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\LabelSequence.cs(27,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedInheritedArrays' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(35,16): error CS0171: Field 'StringSequence.Enumerator._completedItemsInCurrentArray' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(154,21): error CS0188: The 'this' object cannot be used before all of its fields have been assigned [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0843: Auto-implemented property 'StringSequence.Length' must be fully assigned before control is returned to the caller. [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._values' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._inheritedValues' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
C:\dev\prometheus-net\Prometheus\StringSequence.cs(123,13): error CS0171: Field 'StringSequence._hashCode' must be fully assigned before control is returned to the caller [C:\dev\prometheus-net\Prometheus\Prometheus.csproj]
    0 Warning(s)
    29 Error(s)

Time Elapsed 00:00:03.33
C:\dev\prometheus-net [master ≡]> dotnet --version
6.0.203
C:\dev\prometheus-net [master ≡]>

jomadu commented 1 year ago

Whoops ... wrong project: https://github.com/prometheus-net/prometheus-net/issues/383

prometheus / pushgateway

MetricPusher race condition makes pushing final state of metrics unreliable #506

Bug Report