0

I am trying to update the progress bar while running a Parallel.Foreach, but during execution nothing happens. Progressbar gets updated only when the For loop end. How can I make this code work?

XAML

 <StackPanel>
        <Grid x:Name="LoadProgressGrid" Height="100"                  
                  Visibility="Visible">
            <ProgressBar x:Name="LoadProgress"
                Maximum="100"
                Minimum="1" />
            <TextBlock Margin="0,0,0,-5"
                HorizontalAlignment="Center"
                VerticalAlignment="Center"
                FontSize="16"><Run Text="Import Progress"/></TextBlock>   
        </Grid>
        <Button  Height="35" Width="100" Margin="0,10" Content="Test" Click="Test_Click"/>
    </StackPanel>

C#

 private void Test_Click(object sender, RoutedEventArgs e)
        {
              decimal current=0;  
              List<string> lst = new List<string>();

              lst.Add("Foo");
              lst.Add("Foo");
              lst.Add("Foo");
              lst.Add("Foo");

              decimal max = 100000;

              var uiFactory = new TaskFactory(TaskScheduler.FromCurrentSynchronizationContext());

              Parallel.ForEach(lst,  (data) =>
              {
                for (int i = 0; i < max; i++)
                {
                  // Use the uiFactory above:
                  // Note the need for a temporary here to avoid closure issues!
                  current = current + 1;
                  uiFactory.StartNew( () => LoadProgress.Value = (double)(current/max)*100);
                }
              });


              MessageBox.Show("Done!");
         }   
teddy2
  • 382
  • 4
  • 16
  • Are you sure the LoadProgress.Value is a decimal and not an integer? – Vyrira Dec 07 '16 at 19:41
  • 2
    Don't block the UI thread if you want the UI to actually be able to do things. – Servy Dec 07 '16 at 19:43
  • you're not of creating a temporary in order to not use a closure doesn't actually involve you creating a temporary; you just continue to use the closure – Servy Dec 07 '16 at 20:31
  • `Parallel.ForEach` is a blocking action, it won't return until the loop is finished. – ColinM Dec 07 '16 at 21:22

2 Answers2

2

As pointed out in the comments, Parallel.ForEach does not return until the loop has completed which will block the thread it runs on, and the comments by Servy in your answer also say that you are accessing and changing state from various threads without synchronization or object locking (see Race Conditions).

If you are unsure about the correct way to change state on the UI Thread then you can leave the framework do the work of capturing the context with IProgress<T>.

Regarding your answer, you can put the async keyword directly on the Test_Click event handler while keeping the return type void, but please bare in mind that void is not recommended or suggested for any other async methods and they should return a type of Task or Task<T>, read more here on why async void is a bad idea.

More to the point, here's a snippet making use of async and non-blocking code to report progress and update the progress bar, I have commented the code to be more readable.

// Add the async keyword to our event handler
private async void Button_Click(object sender, RoutedEventArgs e)
{
    //Begin our Task
    Task downloadTask = DownloadFile();

    // Await the task
    await downloadTask;
}

private async Task DownloadFile()
{
    // Capture the UI context to update our ProgressBar on the UI thread
    IProgress<int> progress = new Progress<int>(i => { LoadProgress.Value = i; });
    // Run our loop
    for (int i = 0; i < 100; i++)
    {
        int localClosure = i;
        // Simulate work
        await Task.Delay(1000);
        // Report our progress
        progress.Report((int)((double)localClosure / 100 * 100));
    }
}
Community
  • 1
  • 1
ColinM
  • 2,622
  • 17
  • 29
0

From this answer: Using Task with Parallel.Foreach in .NET 4.0 and Servy's comment I got it working.

 private  void Test_Click(object sender, RoutedEventArgs e)
        {
            test();

         }

        public async void test()
        {
            decimal current = 0;
            List<string> lst = new List<string>();

            lst.Add("Foo");
            lst.Add("Foo");
            lst.Add("Foo");
            lst.Add("Foo");

           decimal max = 10000;

          //Remove await (and async from signature) if, want to see the message box rightway.  
           await Task.Run(() => Parallel.ForEach(lst, (data) =>
            {
                for (int i = 0; i < max; i++)
                {                                        
                    current = current + 1;                    
                    Dispatcher.Invoke(new Action(() => LoadProgress.Value = (double)(current / max) * 100));
                }
            }));

            MessageBox.Show("Done!");
        }
Community
  • 1
  • 1
teddy2
  • 382
  • 4
  • 16
  • why the down vote? Is something not correct or I am doing something horribly wrong? Please let me know. – teddy2 Dec 07 '16 at 20:26
  • So you're scheduling a thread pool thread to schedule a thread pool thread to run a bunch of operations to schedule the UI (where you were from the start) thread to set a value. That's just awful design. And that's not even mentioning the invalid accessing of state from various thread pool threads in an unsynchronized manor, which is entirely unsafe. – Servy Dec 07 '16 at 20:28
  • OK. Thanks for the response. I am pretty new with C# and first attempt at tasks. Can you kindly advise the solution. Is it possible to update WPF progressbar from Parallel.ForEach? – teddy2 Dec 07 '16 at 20:33
  • Your code isn't doing anything meaningful in the first place. Providing a sensible implementation of code that sets a progress bar to a random value and then continues on isn't really a sensible thing to do to begin with. – Servy Dec 07 '16 at 20:35
  • sure. will try to post more sensible example. thanks again. – teddy2 Dec 07 '16 at 20:42