Simple web service call with retries

Question

This code is actually pretty basic. Basically, I'm doing a large number of web calls. I currently do up to 3 Web Service calls simultaneously, one of which is below.

There were 3 errors I saw in testing: Bad Gateway, Gateway Timeout, and basically "Couldn't establish a secure connection" (not the exact error text). In each case, a single retry appears to have fixed the issue.

"Bad Gateway" and "Gateway Timeout" didn't result in an exception, but "Couldn't establish a secure connection" did.

If I get either of the first two errors, it'll wait 2 seconds and try again. If it still didn't work, it tries 7 seconds and tries again. After that, it "gives up" and just returns null. (I've never seen that happen in testing, and I've never even gotten to the second retry). The 2-second and 7-second delays are somewhat arbitrary.

The code is below:

public static async Task<Job> GetJob(string id)
{
    int attempts = 1;

    tryAgain:

    string jobJson;

    try
    {
        using (HttpClient client = HttpClientConstructor.GetHttpClient(false, "application/json"))
        {
            HttpResponseMessage response = await client.GetAsync("jobs/" + id);

            if (!response.IsSuccessStatusCode)
            {
                switch (response.StatusCode)
                {
                    case HttpStatusCode.BadGateway:
                    case HttpStatusCode.GatewayTimeout:
                        await Task.Delay(2000);
                        response = await client.GetAsync("jobs/" + id);

                        if (response.StatusCode == HttpStatusCode.BadGateway || response.StatusCode == HttpStatusCode.GatewayTimeout)
                        {
                            await Task.Delay(7000);
                            response = await client.GetAsync("jobs/" + id);
                        }
                        break;
                }

                if (!response.IsSuccessStatusCode)
                {
                    string errorMessage = "Failed I have. Reason: " + response.ReasonPhrase;
                    Console.WriteLine(errorMessage);

                    System.Diagnostics.Trace.TraceError(errorMessage);

                    // If multiple retries couldn't get it, just give up
                    return null;
                }
            }

            jobJson = await response.Content.ReadAsStringAsync();
        }

        Job job = JsonConvert.DeserializeObject<Job>(jobJson);

        return job;
    }
    catch (Exception e)
    {
        Console.WriteLine(e.ToString());

        System.Diagnostics.Trace.TraceError(e.ToString());

        // Go for two retries
        // This actually worked in my testing
        // In the rare case where I got exceptions, a retry worked
        if (attempts <= 3)
        {
            attempts++;

            // Is goto evil here?
            goto tryAgain;
        }

        return null;
    }
}

Obviously, this will be running asynchronously. As mentioned above, I will run up to 3 calls at once. It's fixed at 3 to avoid being "throttled" by rate limits on the API.

Is this a good way to solve this?

Could this code be improved?

Answer 1

In general if you have a goto statement, you should reorder your code to not need it. In this case, your code actually tries 9 times, not three, because you await client.GetAsync 3 times, and then jump back to your goto label up to 3 times.

The simple fix in this case is to remove the extra response=await... calls, enclose the entire try/except block in a loop and break or return if the call is successful. Something like:

for (int i = 0; i < 3; i++){
    try
    {
        HttpResponseMessage response = await client.GetAsync("jobs/" + id);
    }
    catch (CantConnectExcept as ex){
        await Task.Delay(2000);
        continue;
    }

    if (!response.IsSuccessStatusCode){
        await Task.Delay(2000);
        continue;
    }

    jobJson = await response.Content.ReadAsStringAsync();
    return JsonConvert.DeserializeObject<Job>(jobJson);
}
return null

It doesn't sound like waiting more seconds on later failures is important. If you find it is important, you can do something like:

List<int> waitTime = new List<int> {2000, 2000, 7000};
...
await Task.Delay(waitTime[i]);

Answer 2

Retry logic is usually a bit of a code smell. It's not very common to have a transient error that is common and resolved in the space of a few seconds of retry logic. In such a scenario it may be better to have a queue of some sort in place that can poison messages can be sent to an error store.

Answer 3

I would like to add few more suggestions to the previous answer. The idea is to set minimum delay, maximum delay and step delay, this will help us in increasing of delay progressively, irrespective of the retry count. If the requirement comes up to increase the number of retries then we don't need to change the code/logic

1. Declare the below properties, these values can be taken from configuration files, for simplicity I have declared value directly inside the class.

   private int _maximumInterval = 10000;
   private int _minimumDelay = 1000;
   private int _stepDelay = 500;
   private int _retryCount = 3;
   

2. In the function you can do the following

   public async Task.....
   {   
       int _currentDelay = _minimumDelay;
       for (int i = 0; i < _retryCount; i++)
       {
          try
          {
               HttpResponseMessage response = await client.GetAsync("jobs/" + id);
          }
          catch (Exception  ex)
          {
              await Task.Delay(_currentDelay = _currentDelay >= _maximumInterval ? _maximumInterval : (_currentDelay + _stepDelay));
              continue;
          }
   
          if (!response.IsSuccessStatusCode)
          {
               await Task.Delay(_currentDelay = _currentDelay >= _maximumInterval ? _maximumInterval : (_currentDelay + _stepDelay));
               continue;
          }
   
          jobJson = await response.Content.ReadAsStringAsync();
          return JsonConvert.DeserializeObject<Job>(jobJson);
      }
   
      return null;
   }