Missing resiliency options

[zbalkan]

Hi,

I was trying to implement the H.Pipes thanks to the blog of Erik Engberg's blog post. This library saved me from a huge blocker.

Currently, I am trying to implement a timeout, retry and circuit breaker policy. My option was to track the time and cancel the ConnectAsync() method. I used the Microsoft documentation on CancellationTokenSource.

However, I believe this needs to be handled as if HTTP requests are handled via Polly. In that case, it can be done either by allowing Polly to handle the resiliency policies or by adding those policies into the H.Pipes. directly. In either case, it requires a TimeOut exception, so that either Polly or the pipe itself can handle the retries and possible circuit breaking.

Initially, I planned to do it myself and send a PR but I just followed the docs and moved on due to project deadlines. I hope we can see these policies in H.Pipes.

Edit: The piece of code below is based on the example, in the class NamedPipeClient.cs. It includes only timeout. I did not try to add retry yet.

        private static readonly CancellationTokenSource Cts = new();
        private readonly int _cancellationTimeout;

        public NamedPipeClient(int timeout = 500)
        {
            _cancellationTimeout = timeout;
            //...
        }

        public async Task InitializeAsync()
        {
           // This try/catch/finally might be included in the library. 
           // So ctor parameter would suffice
            try
            {
                Cts.CancelAfter(_cancellationTimeout); 
                await _client.ConnectAsync(Cts.Token);

                await _client.WriteAsync(new PipeMessage
                {
                    Action = ActionType.SendText,
                    Text = "Hello from client",
                }, Cts.Token);
            }
            catch (TaskCanceledException)
            {
                Debug.WriteLine($"Task cancelled after timeout {_cancellationTimeout}.");
                Debug.WriteLine(e.Message);
               ///...
            }
            finally
            {
               // this is tricky. If the Cts will live throughout the lifetime of the class, 
               // this should be in the Dispose method of the class, not here
                Cts.Dispose();
            }
        }

[HavenDV]

Hello.

I have studied what Polly has to offer, but unfortunately I have never used it.
At the moment, this code is used:

ReconnectionInterval = reconnectionInterval ?? TimeSpan.FromMilliseconds(100);
ReconnectionTimer = new System.Timers.Timer(ReconnectionInterval.TotalMilliseconds);
ReconnectionTimer.Elapsed += async (_, _) =>
{
    try
    {
        if (!IsConnected && !IsConnecting)
        {
            using var cancellationTokenSource = new CancellationTokenSource(ReconnectionInterval);

            try
            {
                await ConnectAsync(cancellationTokenSource.Token).ConfigureAwait(false);
            }
            catch (OperationCanceledException)
            {
            }
        }
    }
    catch (Exception exception)
    {
        ReconnectionTimer.Stop();

        OnExceptionOccurred(exception);
    }
};

the timer is started after a call to ConnectAsync or WriteAsync if AutoReconnect == true (default)

[HavenDV]

catch (TaskCanceledException)

Please study to avoid future mistakes: https://stackoverflow.com/a/34359530/4792221

[HavenDV]

Polly looks like a way to make the built-in methods much more flexible and appropriate for a particular situation. As I understand it, this can work with any client, not just HttpClient?

[HavenDV]

The goal of my libraries is to be as easy to use as possible, but deeply customizable if required.
As I understand it, I need to add a way to tell Polly about bad events by adding new exceptions or custom events. And also be able to turn off the default behavior.

[zbalkan]

catch (TaskCanceledException)

Please study to avoid future mistakes: https://stackoverflow.com/a/34359530/4792221

Well, I managed to find this one out by experiencing it, unfortunately. Thank you.

And yeah, Polly can be considered a wrapper around mostly async operations (a huge oversimplification but it has a point) and adding a few exceptions would most likely be enough to make these work together.

Edit: grammar

[HavenDV]

var retryPolicy = Policy
  .Handle<InvalidOperationException>()
  .RetryAsync(3, async (exception, count) =>
  {
      await client.ConnectAsync();
  }

await retryPolicy.ExecuteAsync(async () =>
    await client.WriteAsync(message));

For now, this code will work because WriteAsync returns InvalidOperationException if client.AutoReconnect == false and the connection to the server is not established or lost.

[HavenDV]

Or do you think it's still worth throwing a special NotConnectedException?

[zbalkan]

Or do you think it's still worth throwing a special NotConnectedException?

For a meaningful response, I do believe that it would be better to have NotConnectedException, ConnectionFailedException, or whatever you like. It's better by principle. But for practical reasons, I need to experiment on it. I am not as familiar as you to estimate the results. 😊 I will comment after some benchmarks.

But still, thank you for your work for solving such a niche problem that Windows developers are struggling with for years and in a simplified manner. And also, thank you for the quick response.

[zbalkan]

Also, adding the reconnectionInterval parameter in the sample would be better to explain the Polly sample. I saw them and they are crystal clear.

Would you consider initiating AutoReconnect in the constructor?

[HavenDV]

Would you consider initiating AutoReconnect in the constructor?

Not sure, because I consider this an optional parameter, which should be passed through:

new PipeClient<T>()
{
    AutoReconnect = false,
}

because there are already many parameters in the constructor. Perhaps you have some example where autoReconnect in the parameter list is needed?

[HavenDV]

Also, adding the reconnectionInterval parameter in the sample would be better to explain the Polly sample. I saw them and they are crystal clear.

Did not understand this moment, can you explain in more detail?

[zbalkan]

Would you consider initiating AutoReconnect in the constructor?

Not sure, because I consider this an optional parameter, which should be passed through:

new PipeClient<T>()
{
    AutoReconnect = false,
}

because there are already many parameters in the constructor. Perhaps you have some example where autoReconnect in the parameter list is needed?

Reasonable.

Also, adding the reconnectionInterval parameter in the sample would be better to explain the Polly sample. I saw them and they are crystal clear.

Did not understand this moment, can you explain in more detail?

I meant something like this.

// default reconnectionInterval is 100 ms
var timeout = TimeSpan.FromMilliseconds(1000);
await using var client = new PipeClient<MyMessage>(pipeName, reconnectionInterval: timeout);
// Set explicitly to trigger an exception
AutoReconnect = false;

var retryPolicy = Policy
  .Handle<InvalidOperationException>()
  .RetryAsync(3, async (exception, count) =>
  {
      await client.ConnectAsync();
  }

await retryPolicy.ExecuteAsync(async () =>
    await client.WriteAsync(message));

But in polly docs, I saw a combination of retry and timeout without the requirement of wrapped function timeout capability. But without an exception, we need to have a response message just like HTTP response so that we can check the success result.

var retryPolicy =
		Policy.HandleResult<MyMessage>(m => !m.IsSuccessful) // Without the exception, we can handle the response if exists
			.Or<TimeoutRejectedException>() // This exception is thrown by the timeoutPolicy
			.RetryAsync(3);

var timeoutPolicy = Policy.TimeoutAsync(1);

var message = await 
	retryPolicy.ExecuteAsync(() =>
		timeoutPolicy.ExecuteAsync(async token => 
			await client.WriteAsync(new MyMessage() { Text = "Some message" } , token), CancellationToken.None));

[HavenDV]

reconnectionInterval will not work if AutoReconnect == false. Why specify it?

[zbalkan]

My bad. The default behavior is to retry infinitely, right? Then, for instance, when the named pipe client tries to connect and it fails, it throws an InvalidOperationException. In case of high latency, there's no timeout. So, we have to implement this with Polly. Is that the case? It's my misunderstanding then.

So, except for the exception name suggestion, I guess everything is OK from my side. And there's a solid Polly example for future requirements.

Thank you.