Scraping an parsing jockeys data using Task.Run

Question

I started to have HUGE doubts regarding my code and I need some advice from more experienced programmers and architects.

On the button click, the application runs a method, that is running a ScrapJockeys with parameters:

if (UpdateJockeysPl) await ScrapJockeys(JPlFrom, JPlTo + 1, "jockeysPl"); //1 - 1049

Purpose of this is to scrap some data from web service and parse it with a model:

public class LoadedJockey
    {
        public string Name { get; set; }
        public string Link { get; set; }
        public List<JockeyRaceDetails> AllRaces { get; set; }
    }

When the data is parsed, object is added to ObeservableCollection<LoadedJockey> and the collection is saved. Thanks to this, and based on the data the application can do computation of Jockey performace in the past.

And now more about the method and problems with the method:

I have a method, that is triggering a for loop, repeating between 20K - 150K times. Inside the loop I need to call a service method, that execution of the method takes a lot of time. Also I wanted to have ability of canellation of the loop and everyting what is going on inside of the loop/method.

Right now I have a method with list of tasks, and inside of the loop is trigerred a Task.Run. And inside of each task I am calling awaited service method. Also each task has assigned cancellation token, like IN THE EXAMPE:

public async Task ScrapJockeys(int startIndex, int stopIndex, string dataType)
        {
            //init values and controls
            CommandStartedControlsSetup("UpdateDataCommand");
            List<Task> tasks = new List<Task>();
            int loopCounter = 0;
            ProgressBarTick("Looking for jockeys", loopCounter, stopIndex, startIndex);

            for (int i = startIndex; i < stopIndex; i++)
            {
                if (TaskCancellation == true)
                {
                    break;
                }

                int j = i;

                Task task = Task.Run(async () =>
                {
                    LoadedJockey jockey = new LoadedJockey();

                    CancellationToken.ThrowIfCancellationRequested();

                    if (dataType == "jockeysPl") jockey = await _scrapServices.ScrapSingleJockeyPlAsync(j);
                    if (dataType == "jockeysCz") jockey = await _scrapServices.ScrapSingleJockeyCzAsync(j);

                    if (jockey.Name != null)
                    {
                        lock (((ICollection)Jockeys).SyncRoot)
                        {
                            //if objects are already in the List
                            if (Jockeys.Any(h => h.Name.ToLower() == jockey.Name.ToLower()))
                            {
                                LoadedJockey doubledJockey = Jockeys.Where(h => h.Name.ToLower() == jockey.Name.ToLower()).FirstOrDefault();
                                Jockeys.Remove(doubledJockey);
                                MergeJockeysData(doubledJockey, jockey);
                            }
                            else
                            {
                                Jockeys.Add(jockey);
                            }
                        }
                    }

                    loopCounter++;

                    //saves all every 1000 records, just in case
                    if (loopCounter % 1000 == 0)
                    {
                        await _dataServices.SaveAllJockeysAsync(Jockeys.ToList());
                    }

                    ProgressBarTick("Looking for jockeys", loopCounter, stopIndex, startIndex);

                }, TokenSource.Token);

                tasks.Add(task);
            }

            try
            {
                await Task.WhenAll(tasks);
            }
            catch (OperationCanceledException)
            {
                //
            }
            finally
            {
                await _dataServices.SaveAllJockeysAsync(Jockeys.ToList()); //saves everything to JSON file

                AllControlsEnabled = true;

                CommandCompletedControlsSetup();

                VisibilityCancellingMsg = Visibility.Collapsed;
            }
        }

So my question is, is everything fine with my code? According to THIS ARTICLE:

Many async newbies start off by trying to treat asynchronous tasks the same as parallel (TPL) tasks, and this is a major misstep.

What should I use then?

And according to THIS ARTICLE:

On a busy server, this kind of implementation can kill scalability.

So how am I supposed to do it?

Please be noted, that service interface method signature is Task<LoadedJockey> ScrapSingleJockeyPlAsync(int index);

And also i am not 100% sure that I am using Task.Run correctly within my service class. The methods inside are wrapping the code inside await Task.Run(() =>, like IN THE EXAMPLE:

public async Task<LoadedJockey> ScrapSingleJockeyPlAsync(int index)
        {
            LoadedJockey jockey = new LoadedJockey();
            await Task.Run(() =>
            {
                int n;
                List<JockeyRaceDetails> allRaces = new List<JockeyRaceDetails>();

                for (int year = DateTime.Now.Year; year > 2013; year--)
                {
                    StringBuilder sb = new StringBuilder();
                    sb.Append("https://koniewyscigowe.pl/dzokej?d=");
                    sb.Append(index);
                    sb.Append("&sezon=");
                    sb.Append(year);
                    sb.Append("#wyniki_koni");

                    string link = sb.ToString();

                    HtmlAgilityPack.HtmlWeb web = new HtmlAgilityPack.HtmlWeb();
                    HtmlAgilityPack.HtmlDocument doc = web.Load(link);

                    //gets the name of the jockey
                    if (jockey.Name == null)
                    {
                        try
                        {
                            HtmlNode singleNode = doc.DocumentNode.SelectSingleNode("/html/body/main/section[1]/div[1]/h3");
                            string racersName = singleNode.OuterHtml.ToString();
                            if (racersName.Contains("Jeździec") && racersName.Length > 65)
                            {
                                racersName = racersName.Split('>')[1].Split(new string[] { " - " }, StringSplitOptions.None)[1].Split('<')[0].Trim(' ');
                                if (racersName.Contains(" "))
                                {
                                    char letter = racersName[0];
                                    racersName = racersName.Split(' ')[1].Trim(' ');
                                    racersName = letter + ". " + racersName;
                                }

                                racersName = MakeTitleCase(racersName);

                                jockey.Name = racersName; //jockeys name
                            }
                            else
                            {
                                racersName = "";
                            }
                        }
                        catch (Exception e)
                        {

                        }
                    }

                    //scrap races
                    HtmlNode[] tableRow = doc.DocumentNode.SelectNodes("//*[@id=\"wykaz_list\"]/tbody/tr").ToArray();
                    if (tableRow != null && tableRow.Length > 0)
                    {
                        foreach (var row in tableRow)
                        {
                            string stringTableRow = row.OuterHtml.ToString();

                            //all races
                            //if row not contains 'brak startow' and contains m (meters)
                            if (!stringTableRow.Contains("Brak danych") && (stringTableRow.Contains("&nbsp;m") || stringTableRow.Contains(" m")))
                            {
                                try
                                {
                                    JockeyRaceDetails race = new JockeyRaceDetails();

                                    string raceDate = stringTableRow.Split('>')[3].Split('<')[0].Trim(' ');
                                    string raceDistance = stringTableRow.Split('>')[8].Split(' ')[0].Trim(' ');
                                    string horsesName = stringTableRow.Split('>')[10].Split('<')[0].Trim(' ');
                                    horsesName = MakeTitleCase(horsesName);
                                    string raceScore = stringTableRow.Split('>')[12].Split('<')[0].Trim(' ');
                                    string racePlace = raceScore.Split('/')[0].Trim(' ');
                                    string raceCompetitors = raceScore.Split('/')[1].Trim(' ');

                                    race = ParseJockeyRaceData(raceDate,
                                        raceDistance,
                                        horsesName,
                                        raceScore,
                                        racePlace,
                                        raceCompetitors);

                                    allRaces.Add(race);
                                }
                                catch (Exception e)
                                {

                                }
                            }
                        }
                    }
                }
                jockey.AllRaces = allRaces; //jockeys races between 2015-2019
                jockey.Link = "https://koniewyscigowe.pl/dzokej?d=" + index; //racers link

                if (jockey.AllRaces.Count == 0)
                {
                    jockey.Name = null;
                }

            });
            return jockey;
        }

As far as I understand from the articles, this is kind of anti-pattern. But I am confused a bit. Basing on THIS SO REPLY, it should be fine...? If not, how to replace it?

Answer 1

                if (TaskCancellation == true)
                {
                    break;
                }

                    CancellationToken.ThrowIfCancellationRequested();

Some comments explaining why two different cancellation methods are necessary would be useful.

---

                    if (jockey.Name != null)
                    {
                        lock (((ICollection)Jockeys).SyncRoot)
                        {
                            //if objects are already in the List
                            if (Jockeys.Any(h => h.Name.ToLower() == jockey.Name.ToLower()))
                            {
                                LoadedJockey doubledJockey = Jockeys.Where(h => h.Name.ToLower() == jockey.Name.ToLower()).FirstOrDefault();
                                Jockeys.Remove(doubledJockey);
                                MergeJockeysData(doubledJockey, jockey);
                            }
                            else
                            {
                                Jockeys.Add(jockey);
                            }
                        }
                    }

There are at least four code smells here:

1. Using Name as a primary key. With real world data, what prevents two jockeys from having the same name?
2. Using an ICollection for something which clearly needs to be an IDictionary, given the way its contents are looked up.

3. if (collection.Any(predicate))
   {
       variable = collection.Where(predicate).FirstOrDefault();
   

   searches twice, when one search suffices:

   variable = collection.FirstOrDefault(predicate);
   if (variable != null)
   {
   

4. If MergeJockeysData doesn't modify Jockeys then data would seem to be lost, but if it does modify then the data flow is being obfuscated.

---

            try
            {
                await Task.WhenAll(tasks);
            }
            catch (OperationCanceledException)
            {
                //
            }
            finally
            {
                await _dataServices.SaveAllJockeysAsync(Jockeys.ToList()); //saves everything to JSON file

Task.WhenAll is a blunt weapon. If there could be hundreds of tasks, you may find that you get better performance by placing a cap on the number of tasks which are executed simultaneously. This requires a lazy enumerator of tasks and a loop with Task.WhenAny.

If an OperationCanceledException is thrown, does it really make sense to save the partial results?

---

                    HtmlAgilityPack.HtmlWeb web = new HtmlAgilityPack.HtmlWeb();
                    HtmlAgilityPack.HtmlDocument doc = web.Load(link);

This seems to me to be missing the point of async. It would make a lot more sense to me to first use System.Net.WebClient.DownloadStringAsync to do the IO asynchronously, and then load the string into HtmlAgilityPack for the CPU-bound part. In fact, unless memory consumption is an issue it probably makes sense to have a producer-consumer setup where multiple async tasks do IO and a single Task.Run does the parsing.