none
async/await method does not update correctly my property RRS feed

  • Question

  • Hi.

    I have a method in my form which returns the count of some specific elements from an xml.

    private int GetNumberOfMissingElements(string filePath)
    {
        int i = 0;
        XDocument xml = XDocument.Load(filePath);
        foreach (XElement xe in xml.Descendants("someElement"))
        {
            if (xe.Descendants("someSpecificChild").Count() == 0)
            {
                i++;
            }
        }
        return i;
    }

    This method I want to use it in a click event of a button, in async way on multiple files. As long as I return an integer value from this method I want to increment a property on my form, which will be displayed in a text box on my form.

    private async void CountTotalMissingElements(DataTable files)
    {
        foreach(DataRow r in files.Rows)
        {
            string filePath = r["FilePath"].ToString();
            var result = await Task.Run(() => GetNumberOfMissingElements(filePath));
            TotalMissingElements += result;
        }
    }

    And my TotalMissingElements property is:

    private int totalMissingElements; public int TotalMissingElements { get => totalMissingElements; set { totalMissingElements = value; if(txtTME.InvokeRequired) { txtTME.BeginInvoke((MethodInvoker)delegate () { txtTME.Text = totalMissingElements.ToString(); } }
    else
    {
    txtTME.Text = totalMissingElements.ToString();
    }

    }

    }


    The problem is the files are always the same, but results are different. Sometimes shows me a result, sometimes other result. 
    What I'm doing wrong?

    Regards,
    Vali


    • Edited by ValiMaties Friday, September 25, 2020 1:44 PM Spellchecking
    Friday, September 25, 2020 1:30 PM

Answers

  • Hi.

    I'm using 16.8.0 preview 3.1 (every time latest).

    BTW: Interlocked.Add() solved my problem!

    private void CountTotalMissingElements(DataTable files)
    {
        List<Task> tasks = new List<Task>();
        foreach(DataRow r in files.Rows)
        {
            string filePath = r["FilePath"].ToString();
            tasks.Add(Task.Run(() =>
            {
                var result = GetNumberOfMissingElements(filePath));
                Interlocked.Add(ref TotalMissingElements, result);
                // TotalMissingElements is now a field, and moved code from Setter to Task, so textbox will update its Text here
            }));
        }
        Task.WaitAll(tasks.ToArray());
    }
    


    Monday, September 28, 2020 6:04 AM

All replies

  • Is CountTotalMissingElements called from the UI thread?
    When yes: you shouldn't need to use BeginInvoke in TotalMissingElements.set

    But when it is called on a non-UI thread:
    totalMissingElements = value;
    can be called from multiple threads and thus its value can not be guaranteed.

    • Edited by EckiS Friday, September 25, 2020 5:45 PM
    Friday, September 25, 2020 5:45 PM
  • Hi Vali,

    Just a guess here, but it might have to do with something called "closures". Take a look at my blog post where I talk about this: https://geek-goddess-bonnie.blogspot.com/2017/03/fire-and-forget.html  I was using Task.Run() rather than async/await, but the principle is likely the same.

    With that in mind, if that is indeed the cause of your problem, the bold bits should be all you need to change to get around it:

    private async void CountTotalMissingElements(DataTable files)
    {
        foreach(DataRow r in files.Rows)
        {
            DataRow rr = r;
            string filePath = rr["FilePath"].ToString();
            var result = await Task.Run(() => GetNumberOfMissingElements(filePath));
            TotalMissingElements += result;
        }
    }



    ~~Bonnie DeWitt [C# MVP]

    http://geek-goddess-bonnie.blogspot.com

    Saturday, September 26, 2020 8:38 PM
    Moderator
  • Bonnie: wasn't the behavior changed with C# 5, so this should no longer be needed?
    Closing over the loop variable considered harmful, part two

    UPDATE: We are taking the breaking change. In C# 5, the loop variable of a foreach will be logically inside the loop, and therefore closures will close over a fresh copy of the variable each time
    Sunday, September 27, 2020 1:57 AM
  • EckiS --- yeah, I saw that too. Googling, I see that C# 5 released with Visual Studio 2012. 

    We don't know what version of Visual Studio Vali is using ... I should have asked.

    So, Vali --- what version of Visual Studio are you using?    =0)


    ~~Bonnie DeWitt [C# MVP]

    http://geek-goddess-bonnie.blogspot.com

    Sunday, September 27, 2020 5:57 PM
    Moderator
  • Hi.

    I'm using 16.8.0 preview 3.1 (every time latest).

    BTW: Interlocked.Add() solved my problem!

    private void CountTotalMissingElements(DataTable files)
    {
        List<Task> tasks = new List<Task>();
        foreach(DataRow r in files.Rows)
        {
            string filePath = r["FilePath"].ToString();
            tasks.Add(Task.Run(() =>
            {
                var result = GetNumberOfMissingElements(filePath));
                Interlocked.Add(ref TotalMissingElements, result);
                // TotalMissingElements is now a field, and moved code from Setter to Task, so textbox will update its Text here
            }));
        }
        Task.WaitAll(tasks.ToArray());
    }
    


    Monday, September 28, 2020 6:04 AM
  • I'm glad you solved your problem, Vali!  That's great!   =0)

    ~~Bonnie DeWitt [C# MVP]

    http://geek-goddess-bonnie.blogspot.com

    Monday, September 28, 2020 3:13 PM
    Moderator