Prevent UnobservedTaskException in LeaderElector

[klesta490]

We have noticed that LeaderElector can produce UnobservedTaskException, this PR aims to 'observe' this exception so UnobservedTaskException handler is not invoked.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: klesta490 / name: Vladimír Kleštinec (20d0a81, 8cdac30)

[k8s-ci-robot]

Welcome @klesta490!

It looks like this is your first PR to kubernetes-client/csharp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/csharp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (master@2899a13). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff            @@
##             master    #1420   +/-   ##
=========================================
  Coverage          ?   70.35%           
=========================================
  Files             ?       89           
  Lines             ?     2648           
  Branches          ?        0           
=========================================
  Hits              ?     1863           
  Misses            ?      785           
  Partials          ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[tg123]

you mean ex in runasync?

[klesta490]

you mean ex in runasync?

Well consequently. Exception is thrown from private async Task<bool> TryAcquireOrRenew(CancellationToken cancellationToken) that is called from private async Task AcquireAsync(CancellationToken cancellationToken) in cases when timeout is hit, noone observes exception from acq task.

This is unobserved exception we experienced

'System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[System.Boolean,k8s.
LeaderElection.LeaderElector+<TryAcquireOrRenew>d__18]' System.Net.Http.HttpRequestException: 
An error occurred while sending the request.
 ---> 
System.Net.Http.HttpProtocolException: The HTTP/2 server sent invalid data on the connection. HTTP/2 error code 'PROTOCOL_ERROR' (0x1).
 
  at System.Net.Http.Http2Connection.ThrowRequestAborted(Exception innerException)
   
  at System.Net.Http.Http2Connection.Http2Stream.CheckResponseBodyState()
   
  at System.Net.Http.Http2Connection.Http2Stream.TryEnsureHeaders()
   
  at System.Net.Http.Http2Connection.Http2Stream.ReadResponseHeadersAsync(CancellationToken cancellationToken)
   
  at System.Net.Http.Http2Connection.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   
  --- End of inner exception stack trace ---
   
  at System.Net.Http.Http2Connection.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   
  at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)

  at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   
  at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   
  at k8s.Kubernetes.SendRequestRaw(String requestContent, HttpRequestMessage httpRequest, CancellationToken cancellationToken)
   
  at k8s.AbstractKubernetes.ICoordinationV1Operations_ReadNamespacedLeaseWithHttpMessagesAsync[T](String name, String namespaceParameter, Nullable`1 pretty, IReadOnlyDictionary`2 customHeaders, CancellationToken cancellationToken)
   
  at k8s.AbstractKubernetes.k8s.ICoordinationV1Operations.ReadNamespacedLeaseWithHttpMessagesAsync(String name, String namespaceParameter, Nullable`1 pretty, IReadOnlyDictionary`2 customHeaders, CancellationToken cancellationToken)
   
  at k8s.CoordinationV1OperationsExtensions.ReadNamespacedLeaseAsync(ICoordinationV1Operations operations, String name, String namespaceParameter, Nullable`1 pretty, CancellationToken cancellationToken)
   
  at k8s.LeaderElection.ResourceLock.MetaObjectLock`1.GetAsync(CancellationToken cancellationToken)
   
  at k8s.LeaderElection.LeaderElector.TryAcquireOrRenew(CancellationToken cancellationToken)

[tg123]

not covered by

csharp/src/KubernetesClient/LeaderElection/LeaderElector.cs

Line 75 in 20d0a81

{

?

[klesta490]

No it is not covered as you can see from my PR. Or am I missing something? Who observes exception from acq task in case of timeout?

[tg123]

i mean there is a catch in renew loop (line 75)
and the only place might throw is line 54 calling AcquireAsync. the very first line in RunAsync

the idea is give user a change to catch ex when calling to RunAsync to ensure the no issue to api server

[klesta490]

i mean there is a catch in renew loop (line 75) and the only place might throw is line 54 calling AcquireAsync. the very first line in RunAsync

the idea is give user a change to catch ex when calling to RunAsync to ensure the no issue to api server

Yes, I am talking about AcquireAsync.
If you look at code of private async Task AcquireAsync(CancellationToken cancellationToken) then there is if statement on line 201 that is calling Task.WhenAny (wait for acq task or timeout task). In scenario when acq task is somehow stuck, slow etc. timeout is hit and Task.WhenAny returns timeout task instead of acq. No line of code in this case awaits acq, so there is eventually garbage collected Task that ended as faulted and has exception => UnobservedTaskException is invoked. In our application we monitor such tasks and log them for further investigation.

Point of my PR is to prevent this UnobservedTaskException because there is nothing in client code (my app) that I can do to avoid it.

[tg123]

could you paste your example code how you use the leader election?
@klesta490

[klesta490]

_leaderElector = new LeaderElector(config);

        _leaderElector.OnStartedLeading += () =>
        {
            Log.Info("Got Leader Lock");
           
        };
        _leaderElector.OnNewLeader += leader =>
        {
            Log.Info($"Leader changed to: {leader}");
           
        };
        _leaderElector.OnStoppedLeading += () =>
        {
            Log.Info("Lost Leader Lock");
           
        };
        _leaderElectorTask = Task.Run(async () =>
        {
            var token = _cts.Token;
            while (!_cts.IsCancellationRequested)
            {
                try
                {
                    await _leaderElector.RunAsync(token);
                }
                catch (OperationCanceledException) when (_cts.IsCancellationRequested)
                {
                    break;
                }
                catch (Exception ex)
                {
                    Log.Error(ex, "Error occured in leader task.");
                }

                try
                {
                    await Task.Delay(TimeSpan.FromSeconds(10), token);
                }
                catch (OperationCanceledException) { }
            }
        });

Anyway, I don't see how is this relevant, have you looked at my commit? I don't want to be rude, but are you familiar with https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskscheduler.unobservedtaskexception?view=net-7.0 ?

[tg123]

sorry, i am new to c#
could you elaborate how the unobserved exception happened
i did not get that part

[tg123]

i wonder why didnt the catch all work

[klesta490]

Here
https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/LeaderElection/LeaderElector.cs#L201 acq is Task an it is not immediatelly awaited. Then is passed as arg to Task.When any on line https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/LeaderElection/LeaderElector.cs#L203 something bad with connection happens and this timeout Task is finished first before acq. This means that Task.WhenAny returns Task.Delay instance instead of acq and this https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/LeaderElection/LeaderElector.cs#L204C47-L204C54 condition is not hit. Loop continues, acq is eventually Garbage collected. And base on application settings UnobservedTaskException is triggered https://www.tabsoverspaces.com/232922-unobserved-tasks-in-net-4-5

[tg123]

ah i see,
acq got dangling and no one await it, that caused the issue.

could you please add the ex to OnError?

[klesta490]

Sure, done

[tg123]

/LGTM

thanks for pr of the edge case

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: klesta490, tg123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [tg123]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tg123]

@klesta490 please sign cla

[tg123]

/LGTM

[klesta490]

Thanks