Beyond "honor code", is there a difference usign a dedicated "lock object" and locking data directly?

Question

I have two threads: one that feeds updates and one that writes them to disk. Only the most recent update matters, so I don't need a PC queue.

In a nutshell:

• The feeder thread drops the latest update into a buffer, then sets a flag to indicate a new update.
• The writer thread checks the flag, and if it indicates new content, writes the buffered update to disk and disables the flag again.

I'm currently using a dedicate lock object to ensure that there's no inconsistency, and I'm wondering what differences that has from locking the flag and buffer directly. The only one I'm aware of is that a dedicated lock object requires trust that everyone who wants to manipulate the flag and buffer uses the lock.

Relevant code:

private object cacheStateLock = new object();
string textboxContents;
bool hasNewContents;

private void MainTextbox_TextChanged(object sender, TextChangedEventArgs e)
{
    lock (cacheStateLock)
    {
        textboxContents = MainTextbox.Text;
        hasNewContents = true;
    }
}

private void WriteCache() // running continually in a thread
{
    string toWrite;

    while (true)
    {
        lock (cacheStateLock)
        {
            if (!hasNewContents)
                continue;

            toWrite = textboxContents;
            hasNewContents = false;
        }

        File.WriteAllText(cacheFilePath, toWrite);
    }
}

Wichman

By buffer do you mean textboxContents? Are you asking if it's valid to lock on textboxContents instead of cacheStateLock? If so - no - it's entirely invalid as the value of textboxContents is constantly changing – Durango

Well, for me lock means the particular piece of code (written inside the lock) is locked by one thread at a time ,not allowing other thread to manipulate the data inside. – Lucienlucienne

What Rob is getting at, which I think is also the answer to your question, is that you can't lock a variable, only an object. A value type, such as your boolean flag, doesn't have a monitor (the metadata required for locking). And while the string variable holds an object that can have an associated monitor, locking only works when everyone acquires the same lock, and your string variable is constantly being reassigned to new string objects, each of which would be a different lock. – Skipp

its not very apparent from your code but if you expose your data and/or pass it to other code there's no guarantee it won't try to lock on it too, with dedicated private lock you exclude this possibility – Heterography

Thanks, all. That answers my question. – Wichman

Comments not for answers. Would @BenVoigt or others consider writing an answer?? – Unanimity

Have you looked at using ReactiveExtensions .Sample() monad? Lock free for the win. – Sapindaceous

Answer 1

First of all, if you're trying to use the bool flag in such manner, you should mark it as volatile (which isn't recommended at all, yet better than your code).

Second thing to note is that lock statement is a sintax sugar for a Monitor class methods, so even if you would be able to provide a value type for it (which is a compile error, by the way), two different threads will get their own version of the flag, making the lock useless. So you must provide a reference type for lock statement.

Third thing is that strings are immutable in the C# so it's theoretically possible for some method to store an old reference to the string and do the lock in a wrong way. Also a string could became a null from MainTextbox.Text in your case, which will throw in runtime, comparing with a private object which wouldn't ever change (you should mark it as readonly by the way).

So, introduction of a dedicated object for synchronization is an easiest and natural way to separate locking from actual logic.

As for your initial code, it has a problem, as MainTextbox_TextChanged could override the text which wasn't being written down. You can introduce some additional synchronization logic or use some library here. @Aron suggested the Rx here, I personally prefer the TPL Dataflow, it doesn't matter.

You can add the BroadcastBlock linked to ActionBlock<string>(WriteCache), which will remove the infinite loop from WriteCache method and the lock from both of your methods:

var broadcast = new BroadcastBlock<string>(s => s);
var consumer = new ActionBlock<string>(s => WriteCache(s));
broadcast.LinkTo(consumer);

// fire and forget
private async void MainTextbox_TextChanged(object sender, TextChangedEventArgs e)
{
    await broadcast.SendAsync(MainTextbox.Text);
}

// running continually in a thread without a loop
private void WriteCache(string toWrite)
{
    File.WriteAllText(cacheFilePath, toWrite);
}

Faxon