Value of volatile variable doesn't change in multi-thread

Question

I have a winform application that runs in background with a BackgroundWorker that has an infinite loop that execute something every hour. My UI Form class is something like this:

public partial class frmAutoScript : Form
{
    private volatile bool _isDownloading = false;
    private bool IsDownloading { get { return this._isDownloading; } set { this._isDownloading = value; } }

    public frmAutoScript()
    {
        InitializeComponent();
        this.RunAutoSynchronization();
    }

    private void RunAutoSynchronization()
    {
        bool isDownloading = this.IsDownloading;

        BackgroundWorker bgwDownloader = new BackgroundWorker();
        bgwDownloader.WorkerReportsProgress = true;
        bgwDownloader.ProgressChanged += (sndr, evnt) =>
        {
            if (evnt.ProgressPercentage == 2)
                isDownloading = this.IsDownloading;
            else
            {
                this.IsDownloading = evnt.ProgressPercentage == 1;
                isDownloading = this.IsDownloading;
            }
        };
        bgwDownloader.DoWork += (sndr, evnt) =>
            {
                while (true)
                {
                    if (DateTime.Now.Hour == 16 &&
                        DateTime.Now.Minute == 0)
                    {
                        try
                        {
                            bgwDownloader.ReportProgress(2);
                            if (!isDownloading)
                            {
                                bgwDownloader.ReportProgress(1);
                                new Downloader().Download();
                            }
                            bgwDownloader.ReportProgress(0);
                        }
                        catch { }
                    }

                    System.Threading.Thread.Sleep(60000);
                }
            };
        bgwDownloader.RunWorkerAsync();
    }
}

And in that frmAutoScript, I also have a button named btnDownload that when clicked, it will download and change the value of the volatile varialbe _isDownloading. The event of the button is something like this:

private void btnDownload_Click(object sender, EventArgs e)
{
    if (IsDownloading)
        MessageBox.Show("A download is currently ongoing. Please wait for the download to finish.",
            "Force Download", MessageBoxButtons.OK, MessageBoxIcon.Exclamation);
    else
    {
        this.IsDownloading = true;
        BackgroundWorker bgwDownloader = new BackgroundWorker();
        bgwDownloader.DoWork += (sndr, evnt) =>
        {
            try
            {
                new Downloader().Download();
            }
            catch(Exception ex)
            {
                MessageBox.Show("An error occur during download. Please contact your system administrator.\n Exception: " +
                    ex.GetType().ToString() + "\nError Message:\n" + ex.Message + " Stack Trace:\n" + ex.StackTrace, "Download Error!", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
        };
        bgwDownloader.RunWorkerCompleted += (sndr, evnt) =>
        {
            this.IsDownloading = false;
        };
        bgwDownloader.RunWorkerAsync();
    }
}

But when I click the button btnDownload and the _isDownloading is set to true, and when the system time hit the 4:00 PM, the new Downloader().Download(); is executed again eventhough the _isDownloading is set to true. Why was it like this?

My code is in C#, framework 4, project is in winforms, build in Visual Studio 2010 Pro.

score 4 · Accepted Answer · answered May 28 '12 at 08:52

4

Your code is not testing against the volatile field - it is testing against isDownloading, which looks like a "local", but (because it is captured) is in fact a regular (non-volatile) field. So: either use some kind of memory barrier, or force that to be a volatile read. Or more simply: get rid of isDownloading completely, and check against the property.

Incidentally, the cache-defeating properties of volatile are not the intent of the keyword, but rather: a consequence. It'll work, but personally I'd suggest writing the code to work by intent rather than by consequence, perhaps using either a simple lock or something like Interlocked.

answered May 28 '12 at 08:52

Marc Gravell

1,026,079
266
2,566
2,900

So what is the intent of the keyword? – zmbq May 28 '12 at 08:55
@zmbq very complex, poorly understood, and relating to CPU instruction re-ordering; here's the words from ECMA 334: http://pastie.org/pastes/3981577/text . I'm pretty confident in my threading, and simply: `volatile` is not the semantic I usually use to guarantee the behaviour – Marc Gravell May 28 '12 at 08:57
I found this: http://msdn.microsoft.com/en-us/library/aa645755%28v=vs.71%29.aspx . I'm probably missing something, because I can't see why volatiles don't have the purpose everybody thinks they do. – zmbq May 28 '12 at 09:01
1

@zmbq then maybe allow Eric Lippert to add a word: see [Atomicity, volatility and immutability are different, part three](http://blogs.msdn.com/b/ericlippert/archive/2011/06/16/atomicity-volatility-and-immutability-are-different-part-three.aspx), the last paragraph starting "Frankly" – Marc Gravell May 28 '12 at 09:07
No argument there... Perhaps there should be a `synchronized` keyword that automatically puts a lock around all field access. It won't save you from deadlocks, though. – zmbq May 28 '12 at 12:45
@zmbq at the end of the day, threading is sufficiently complex that the only sensible way to approach is slowly and carefully. Extra keywords cannot in any way remove the need to stop and think. – Marc Gravell May 28 '12 at 12:59

Value of volatile variable doesn't change in multi-thread

1 Answers1

Linked