microsoft / ApplicationInsights-Kubernetes

Enrich the telemetry data for .NET applications running inside containers that are managed by Kubernetes.
Other
135 stars 57 forks source link

[Investigation] Make calling into ExecuteAsync idempotent #327

Closed xiaomi7732 closed 1 year ago

xiaomi7732 commented 1 year ago

Tag @karolz-ms. He initiated the idea.

In the current implementation, it is undesire to call StartApplicationInsightsKubernetesEnricher() when hosted services exist, because it leads to duplicated call into ExecuteAsync - 1 from the background service, 1 from the explicit call of the method above.

Look into how to make ExecuteAsync() idempotent or find out a way to allow ExecuteAsync() to be called multiple times - that will reduce the chance for client code to call StartApplicationInsightsKubernetesEnricher() by accident, and become more refactor friendly.

xiaomi7732 commented 1 year ago

Notes:

  1. Lazy for ExecuteAsync() - that the task never creates a second time: The issue is cancellation token handling. Lazy doesn't take in a cancellation token; To implement it, it may be a Lazy<Func<CancellationToken, Task>>, and then, it starts to look like an AsyncLazy library - that feels a bit heavy for this case; It is also an intention not to own tricky implementations in this library.

  2. Use an AsyncLazy library - don't need to worry about owning the complex logic. I went over some open-source libraries, and most of them are more than just AsyncLazy, and not all libraries are always dealing with all the issues. It is doable, it is probably not worth introducing the dependency for this scenario.

  3. Going back to the origin of the issue - do not run the code 2nd time, and a stupidly simple solution would be: Have a ref counter, start at 0, and drop the execution when it is increased to 1. Here's a pseudo:

int _refCount = 0;

async Task ExecuteAsync(CancenllationToken cancellationToken)
{
    if ( refCount >= 1 ) { return; } // Already executed;
    Interlocked.Increment(ref refCount);
    if ( refCount != 1 ) { return; } // Only thread got refCount == 1 survives.
    // Start to run the logic. No cancellation token passing around.
}
  1. Introduce a SemaphoreSlim(1,1), one thread grabs the only available resource and never releases it, anything coming in afterward got rejected. - seems a bit to use SemaphoreSlim by force.

I am currently leaning toward 3. I think it is simple enough and easy to understand the semantics, and it doesn't have additional dependencies and is lightweight.

xiaomi7732 commented 1 year ago

Another approach suggested by @karol-ms: just take a lock, considering this isn't any hot path

static readonly _lock = new object();
bool _executed;

async Task ExecuteAsync(CancenllationToken cancellationToken)
{
    lock(_lock)
    {
        if (_executed) { return; } // Already executed.
        _executed = true;
    }
    // The rest of the code.
}

A consideration: use a Task to replace the boolean.