1

I implemented a sample application to see the behavior of the retries inside a Parallel.ForEach loop. As per my observations following application not showing all the values inside the array, in the textbox (showing character number changes randomly).

I want to know what cause this behavior. Thanks

public string DisplayText = "Numbers =";

private void button1_Click(object sender, EventArgs e)
{
    int[] numbers = { 1, 2, 3, 4, 5, 6, 7, 8, 9 };
    Start(numbers);
    textBox1.Text = DisplayText;
}

private void Start(int[] numbers)
{
    Parallel.ForEach(numbers, k =>
    {
        Write(k.ToString());
    });
}

private void Write(string text)
{
    int maxRetryCount = 3;
    int retryCount = 0;
    int retryInterval = 1000;

    while (retryCount < maxRetryCount)
    {
        if(retryCount == maxRetryCount-1)
        {
            DisplayText = DisplayText + "," + text;
            break;
        }

        Thread.Sleep(retryInterval);
        retryCount++;
    }
}
Improve this question
7
  • 8
    The code isn't thread-safe and shows a very common race condition. This isn't a problem with Parallel.ForEach. The code modifies the shared DisplayText without synchronization which means the actual results are essentially random. Multiple threads read the DisplayText contents at the same time and then replace it with their own version. The last one to do so wins. You'd have to use a lock statement to ensure threads don't interfere with each other. Commented May 29, 2024 at 14:52
  • 1
    What is the real problem you want to solve? If you want to call eg 10 URLs with retries but no blocking, there are ways (and duplicate SO questions) to do so asynchronously, safely, and transparently. In ASP.NET Core, using HttpClientFactory and Polly you could write as little as var responses=await Task.WhenAll(urls.Select(url=>client.GetStringAsync(url)));. Not the best way, does the job and retries automatically Commented May 29, 2024 at 15:02
  • button1_Click(object sender, EventArgs e) is this WebForms? That's the only framework that uses events like this Commented May 29, 2024 at 16:06
  • What is your "expected result" ? Parallel.ForEach process the collection out of order. If you don't use an ordered pattern to write your result is your desired order, your result can't be ordered. The code you're sharing is not a good sample for Parallel.ForEach and for a retry mecanism. Commented May 29, 2024 at 16:19
  • @Panagiotis Kanavos no this is Windows Forms Commented May 30, 2024 at 4:19

3 Answers 3

Reset to default
3

This line:

DisplayText = DisplayText + "," + text;

...does not happen in one CPU instruction, so it can be interrupted. When it is interrupted, a thread's copy of DisplayText (stored as a reference) could become out of date, resulting in data loss.

When sharing variables between threads, you must use thread-safe data structures. In this case you could use a ConurrentQueue.

ConcurrentQueue<string> _displayText = new();
string DisplayText => string.Join(",", _displayText);

Then, instead of

DisplayText = DisplayText + "," + text;

Use

_displayText.Enqueue(text);

This data structure is designed to be thread safe and will allow multiple threads append to the queue without interfering with each other.

You could also write your own thread-safe data structure, e.g. by using lock around your updates. This is very easy to get wrong.

But the best way is to solve this sort of problem is simply to avoid any need for thread-safe data structures, by avoiding shared state. You can do this by rewriting Write as a pure function.

IEnumerable<string> Write(string text)
{
    int maxRetryCount = 3;
    int retryCount = 0;
    int retryInterval = 1000;

    while (retryCount < maxRetryCount)
    {
        if(retryCount == maxRetryCount-1)
        {
            yield return text;
            break;
        }

        Thread.Sleep(retryInterval);
        retryCount++;
    } 
}

This way you can splice together the strings after they are computed independently. Using LINQ you can do this in one line.

var displayText = string.Join
(
    ",", 
    Enumerable.Range(1, 9)
        .AsParallel()
        .SelectMany
        (
            x => Write(x.ToString())
        )
);

This latter technique is sometimes called "functional approach".

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

You might want to mention in the answer that the .AsParallel() does not produce results in the order of the original items, unless the AsOrdered operator is included. It might not be importing in this toy example, but in the OP's real-life scenario preserving the order might be desirable.
2

Showing character number changes randomly. I want to know what cause this behavior.

What causes this behavior is that the field DisplayText is accessed by multiple threads concurrently without synchronization. You are not allowed to do that. In other words if you do that, the behavior of your program becomes undefined. Microsoft accepts no responsibility when their products are used incorrectly, so if you file a bug report about this behavior, it will be ceremonially dismissed.

In case you are wondering what you can do to fix the problem, the most obvious solution is to synchronize the access to the DisplayText field. This field is called "shared state" in the jargon of multithreading. You must make sure that only one thread at a time can interact with this field. First you have to declare an object field that will function as the "locker".

private readonly object _locker = new();

It's not mandatory for the locker to be a dedicated object, used for this purpose only. It can be some other object that is completely owned by your code, and is not accessible to the outside world. But using a dedicated object is a common practice.

Then inside the Write method do this:

lock (_locker)
{
    DisplayText = DisplayText + "," + text;
}

The lock statement forces the threads to take their turn before entering the protected region. So it won't be possible for the threads to step on each other toes, and make a mess with the DisplayText field.

Improve this answer

Comments

2

The code is suffering from race conditions. The same thing would happen if multiple threads or multiple BackGroundWorker components were used, that tried to modify DisplayText from the DoWork method.

As the real platform is Winforms, I suspect DisplayText is used as a quick+dirty way to display results or log messages to a TextBox without causing a cross-thread exception.

Before explaining what's wrong, this is how the entire problem can be solved in WinForms since 2012, without blocking or explicit locking and ensure results are displayed as soon as they're available. Along with the mandatory progress and message in a status bar. All forms need a progress and status bar, it's the rules.

Async/await, Parallel.ForEachAsync and Progress

Let's assume the actual work is to retrieve the status from a bunch of sites. The progress report is the site and Status code:

record SiteUrl(string Site, string Url);
record SiteResult(string Site, HttpStatusCode Status);

In the form, progress is handled by the Progress class. Instead of generating temporary strings, the status message is appended to TextBox1. The Progress<> class ensures any progress messages are handled in the thread that created the class, in this case the UI thread. This avoids cross-thread exceptions and ensures there are no race conditions :

IProgress<SiteResult> _progress;
HttpClient _client;

public Form1()
{
    InitializeComponent();
    _progress = new Progress<SiteResult>(ReportProgress);
    _client=new HttpClient();
}

private void ReportProgress(SiteResult result)
{
    var msg = $"{result.Site}:{result.Status}";
    textBox1.AppendText("\r\n");
    textBox1.AppendText(msg);
    toolStripStatusLabel1.Text = msg;
    toolStripProgressBar1.Increment(1);
}

Instead of the blocking Parallel.ForEach, the asynchronous Parallel.ForEachAsync is used :

record SiteUrl(string Site,string Url);

private async void button1_Click(object sender, EventArgs e)
{
    var sites = new List<SiteUrl>{
            new SiteUrl("Google","https://www.google.com"),
            new SiteUrl("MS","https://www.microsoft.com"),
            new SiteUrl("FB","https://www.facebook.com"),
    };

    // Noisy stuff, extract it to keep the code clean
    ResetProgress(sites.Count);

    await Parallel.ForEachAsync(sites, async (site, ct) => ProcessSite(site, ct));

}

async Task ProcessSite(SiteUrl site,CancellationToken ct)
{
    var result = await _client.GetAsync(site.Url, ct);
    _progress.Report(new SiteResult(site.Site, result.StatusCode));
    //Do something else with the result ...
}

private void ResetProgress(int max)
{
    textBox1.Text= "Sites= ";
    toolStripStatusLabel1.Text="";
    toolStripProgressBar1.Value=0;
    toolStripProgressBar1.Maximum=max;
}

What's wrong

The original code suffers from race conditions. The threads used by Parallel.ForEach are reading the same value from DisplayText at the same time, modifying it and then replacing DisplayText with their own value. The last one to make that replacement wins.

One way to fix this is by using a lock to protect access to the field,eg :

//Lock object stored in a field
object _lockObj=new object();

...
lock(_lockObj)
{
    DisplayText = DisplayText + "," + text;
}

That's not the best option, as it blocks the worker threads while trying to update the status field.

As the real change we want is to update textbAnother option is to forward the status change to the UI thread with BeginInvoke`.

if (retryCount == maxRetryCount - 1)
{
    textBox1.BeginInvoke(() =>UpdateStatus(text));
    break;
}

...
void UpdateStatus(string text)
{
    textBox1.AppendText("," + text);
}

DisplayText is no longer needed, the worker threads aren't blocked and the status box is updated immediatelly. The code is more complex though and needs direct access to the UpdateStatus method.

Improve this answer

5 Comments

The Progress<T> is a good idea, but it has a hidden gotcha. After the completion of the await Parallel.ForEachAsync, it's not guaranteed that the reporting will be finished. That's because the Progress<T> updates the UI asynchronously, calling the BeginInvoke instead of the Invoke. In case this is a problem, one solution is to use the custom SynchronousProgress<T> instead of the build-in Progress<T>.
It will be finished eventually, but you might want to do something immediately after the await Parallel.ForEachAsync, that requires that the reporting is finished. And the Progress<T> has no functionality for enforcing this hypothetical requirement. You'll have to do hacks like await Task.Yield(); and hope for the best. Could you explain why the SynchronousProgress<T> defeats the purpose of reporting progress? If you report correctly, i.e. not too often, there will be no measurable negative effect in performance.
The whole point of IProgress is decoupling the workers from reporting, including any synchronization concerns. Throttling reduces the perceived delays with Progress too. Besides, if Send blocks, it can cause the same deadlocks Invoke can. The worker shouldn't have to care about these. In more complex applications instead of direct updates through a specific Progress<> instance, messages/events are posted to a queue and used to update the UI anyway. VS certainly works this way and at some point used DataFlow for this. A custom IProgress could post to that queue
The Invoke/Send can cause a deadlock only if you are doing something bad, i.e. blocking the UI thread, for example with Parallel.ForEachAsync().Wait(). If that's the case, simply switch back to the Progress<T>. The SynchronousProgress<T> is just an alternative, for the cases that it makes sense. I am not proposing to use the SynchronousProgress<T> everywhere. As for decoupling the workers from reporting, the SynchronousProgress<T> does it just as well as the Progress<T>. The abstraction is the same: the IProgress<T> interface.
As for the more complex applications that are not using a specific Progress<T> instance, these are irrelevant to our discussion. We are discussing about the Progress<T> here. Let's stay focused.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.