Network helper class with retry logic on failure

Question

I wrote an helper class which allow me to request the body content from a site that is created by AJAX, for doing so I'm using Puppeteer Sharp. This is the class:

using System;
using System.IO;
using System.Net;
using System.Text;
using System.Threading.Tasks;
using PuppeteerSharp;

namespace App.Helpers
{
    public class NetworkHelper
    {    
    /// <summary>
    /// Get the html of a page waiting for an AJAX call until the selector is visible.
    /// </summary>
    /// <param name="url">Link needed for execute the request.</param>
    /// <param name="selector">Selector to wait.</param>
    /// <param name="attempts">Max attempts until request fail.</param>
    /// <returns>Contains the html of the requested page.</returns>
    public static async Task<string> LoadAndWaitForSelector(Uri url, string selector, int attempts = 5)
    {
        try
        {
            //Create new browser page
            using (Page page = await Handler.Browser.NewPageAsync())
            {                        
                //Navigate on the requested content and wait for selector
                await page.GoToAsync(url.ToString(), timeout: 0);
                await page.WaitForSelectorAsync(selector); //may throw error for timeout    
                return await page.GetContentAsync();
            }
        }
        catch (WaitTaskTimeoutException)
        {
            //there are other attempts, rerty
            if (attempts != 0)
            {
                attempts--;

                //add some delay
                await Task.Delay(10000);

                return await LoadAndWaitForSelector(url, selector, attempts);
            }
            throw;
        }
    }
}

Question: Could this class be improved more?

Also, the Browser is static, so each instance use the same browser for avoid memory leak:

public static Browser Browser { get; set; }

the method is called in the following way:

var html = await NetworkHelper.LoadAndWaitForSelector(
            new Uri("some url"), "#archive-tables");

thanks.

You don't need the static fields, you can use the same two parameters on a failed request as you're calling it within the same method and you are not modifying them so you have direct access to them. This can end up nasty if run in parallel. The threads would overwrite each other's fields. You could also add one more parameter for attemps and decrease it on each recursive call. The default value could be =5. Then there would be no static state and it would be safe to run it in parallel. — t3chb0t
– t3chb0t, Commented Aug 24, 2019 at 8:21
@t3chb0t I'm executing this method in parallel, I just removed the static fields _storedUrl and _storedSelector. Do you have any other suggestions for improve the class? Thanks. The optional parameter for attempts is a good idea — sfarzoso
– sfarzoso, Commented Aug 24, 2019 at 8:25
You are, they won't be caught and it will behave exactly as it does now where you just rethrow it. catch(Exception ex) throw is the same as not doing anything and if you use a more specific exception type then only this one will be handled. All others will be rethrown. — t3chb0t
– t3chb0t, Commented Aug 24, 2019 at 8:39
@dfhwze and I lost like 40 green internet points for that review in comments :P — t3chb0t
– t3chb0t, Commented Aug 24, 2019 at 8:44
@t3chb0t If you add answer I will accept it, I appreciate your help. Let me update the question removing the original code — sfarzoso
– sfarzoso, Commented Aug 24, 2019 at 8:45

t3chb0t · Accepted Answer · 2019-08-24 08:54:24Z

After our initial comment-review-update sequence/loop there is not much left for a review but still, one more thing. You can get rid of the if entirely when you add a when filter to the excpetion:

    catch (WaitTaskTimeoutException) when (attempts > 0)
    {
        //add some delay
        await Task.Delay(10000);

        return await LoadAndWaitForSelector(url, selector, attempts - 1);
    }

It's also better to pass a new value to the recursive call like attempts - 1 than modifying the argument with attempts--

rob.earwaker · Accepted Answer · 2019-08-24 11:13:10Z

Just wanted to add that you may want to consider using Polly instead of rolling your own retry mechanism - this is a library built specifically for this kind of retry mechanism (as well as many more complicated scenarios). Using Polly would look something like the following.

Note that I have renamed the attempts parameter to retryAttempts to make it clear that this does not define the total number of attempts but rather the total number of retry attempts after the initial one, to keep the behaviour the same as your sample code.

public class NetworkHelper
{
    public static Task<string> LoadAndWaitForSelectorAsync(
        Uri url, string selector, int retryAttempts = 5)
    {
        // Create a policy that will...
        var policy = Policy
            // Retry for any `WaitTaskTimeoutException` raised during execution.
            .Handle<WaitTaskTimeoutException>()
            .WaitAndRetryAsync(
                // Retry the specified number of times after the initial attempt.
                retryCount: retryAttempts,
                // Wait for the given duration between each retry attempt. Note that
                // this can depend on the retry number if required.
                sleepDurationProvider: retryNumber => TimeSpan.FromSeconds(10));

        // Execute the operation to get the selector within the retry policy. If the
        // final retry attempt does not succeed, the resulting `WaitTaskTimeoutException`
        // will not be handled by the policy and will be thrown.
        return policy.ExecuteAsync(() => TryGetSelectorAsync(url, selector));
    }

    private static async Task<string> TryGetSelectorAsync(Uri url, string selector)
    {
        using (Page page = await Handler.Browser.NewPageAsync())
        {
            await page.GoToAsync(url.ToString(), timeout: 0);
            await page.WaitForSelectorAsync(selector);
            return await page.GetContentAsync();
        }
    }
}

\$\begingroup\$ Nice solution too, didn't know this library \$\endgroup\$

sfarzoso
– sfarzoso

2019-08-24 12:03:32 +00:00
Commented Aug 24, 2019 at 12:03 — sfarzoso
– sfarzoso, Commented Aug 24, 2019 at 12:03

Stack Exchange Network

Network helper class with retry logic on failure

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Network helper class with retry logic on failure

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions