Combining Interlocked.Increment and Volatile.Write

Question

I've been looking at the source code of System.Reactive (here), and it's taken me down a rabbit hole to this place, where there is a Volatile.Read followed by Interlocked.CompareExchange, on the same variable:

if (Volatile.Read(ref _runDrainOnce) == 0
    && Interlocked.CompareExchange(ref _runDrainOnce, 1, 0) == 0)
{
    //do something
}

As I read it, the logic of this is "if runDrainOnce is 0 and, if it was zero before I changed it to 1". Is there something subtle here? Why is the first check not redundant?

(Even more mind-boggling, there is a lock and a Monitor.Pulse in the same function. Was this the result of a bet? :))

The whole function:

private void Schedule()
{
    // Schedule the suspending drain once
    if (Volatile.Read(ref _runDrainOnce) == 0
        && Interlocked.CompareExchange(ref _runDrainOnce, 1, 0) == 0)
    {
        _drainTask.Disposable = _scheduler.ScheduleLongRunning(this, DrainLongRunning);
    }

    // Indicate more work is to be done by the drain loop
    if (Interlocked.Increment(ref _wip) == 1L)
    {
        // resume the drain loop waiting on the guard
        lock (_suspendGuard)
        {
            Monitor.Pulse(_suspendGuard);
        }
    }
}

Answer 1

Entirely speculative possibility follows. In reality, I'd say that without explicit benchmarking, it probably doesn't matter, and we could simply lose the Volatile.Read test, or even just use a lock. If there was explicit benchmarking, I'd hope for a comment hinting (or linking) at that.

---

We can infer from the naming (_runDrainOnce) that we only expect this to succeed once, and if something is only going to succeed once, we really don't need the success case to be super optimal - so: having a redundant test in that success path: not a huge problem. In contrast, let's speculate that the failure scenario is called many many times, and so having it fail just doing an acquire-fenced read (without attempting to write) may be beneficial.

The Schedule code is invoked by everything - see OnCompleted, OnError, OnNext, etc - so presumably the intent is to just make sure that the scheduling gets started, as efficiently as possible - so it doesn't touch Intelocked more than necessary (once successfully, and possibly a few times indicating failure, if there is high thread competition initially)

---

You didn't explicitly ask, but the lock/Pulse is a common pattern for having an idle worker loop waiting on receiving work using Monitor; you need to wake it if it was likely idle, which is when the count was zero and is now non-zero (hence the Interlocked.Increment).