Correct way to use the Interlocked class for multithreading in .NET

Question

I have a counter, which counts the currently processed large reports

private int processedLargeReports;

and I'm generating and starting five threads, where each thread accesses this method:

public bool GenerateReport(EstimatedReportSize reportSize)
{
    var currentDateTime = DateTimeFactory.Instance.DateTimeNow;
    bool allowLargeReports = (this.processedLargeReports < Settings.Default.LargeReportLimit);
    var reportOrderNextInQueue = this.ReportOrderLogic.GetNextReportOrderAndLock(
        currentDateTime.AddHours(
        this.timeoutValueInHoursBeforeReleaseLock), 
        reportSize, 
        CorrelationIdForPickingReport, 
        allowLargeReports);

    if (reportOrderNextInQueue.IsProcessing)
    {
        Interlocked.Increment(ref this.processedLargeReports);                
    }

    var currentReport = this.GetReportToBeWorked(reportOrderNextInQueue);

    var works = this.WorkTheReport(reportOrderNextInQueue, currentReport, currentDateTime);
    if (reportOrderNextInQueue.IsProcessing)
    {
        Interlocked.Decrement(ref this.processedLargeReports);                
    }
    return works;           
}

the "reportOrderNextInQueue" variable gets a reportorder from the database and checks whether the report order is either "Normal" or "Large" (this is achieved by defining the bool IsProcessing property of reportOrderNextInQueue variable). In case of a large report, the system then Interlock Increments the processedLargeReport int and processes the large report. Once the large report is processed, the system Interlock Decrements the value.

The whole idea is that I'll only allow a single report to be processed at a time, so once a thread is processing a large report, the other threads should not be able to access a large report in the database. The bool allowLargeReport variable checks whether the processedLargeReports int and is above the limit or not.

I'm curious whether this is the proper implementation, since I cannot test it before Monday. I'm not sure whether I have to use the InterLocked class or just define the processedLargeReports variable as a volatile member.

You probably want a `lock` rather than an increment. As an academic exercise see Eric Lippert's Article on [How Locking Works in C#](http://blog.coverity.com/2014/02/12/how-does-locking-work/#.Vf2XYJeBKDk) for some more in depth talk about what its doing in general. — theB, Sep 19 '15 at 17:23
The link from @theB no longer works. The original blog can be found at https://ericlippert.com/2014/02/12/how-does-a-lock-work/ but is mainly just broken links to that same original site. — shapeshifter42, Mar 16 '20 at 11:59

Evk · Answer 1 · 2015-09-19T18:15:37.710

Say you have 5 threads starting to run code above, and LargeReportLimit is 1. They will all read processedLargeReports as 0, allowLargeReports will be true for them, and they will start processing 5 items simultaneously, despite your limit is 1. So I don't really see how this code achieves you goal, if I understand it correctly.

To expand it a bit: you read processedLargeReports and then act on it (use it to check if you should allow report to be processed). You act like this variable cannot be changed between read and act, but that is not true. Any number of threads can do anything with processedLargeReports in between you read and act on variable, because you have no locking. Interlocked in this case will only ensure that processedLargeReports will always get to 0 after all threads finished processing all tasks, but that is all.

If you need to limit concurrent access to some resourse - just use appropriate tool for this: Semaphore or SemaphoreSlim classes. Create semaphore which allows LargeReportLimit threads in. Before processing report, Wait on your semaphore. This will block if number of concrurrent threads processing report is reached. When processing is done, release your semaphore to allow waiting threads to get in. No need to use Interlocked class here.

Ivan Stoev · Accepted Answer · 2015-09-21T08:06:20.853

volatile does not provide thread safety. As usual with multithreading you need some synchronization - it could be based on Interlocked, lock or any other synchronization primitive and depends on your needs. You have chosen Interlocked - fine, but you have a race condition. You read the processedLargeReports field outside of any synchronization block and making a decision based on that value. But it could immediately change after you read it, so the whole logic will not work. The correct way would be to always do Interlocked.Increment and base your logic on the returned value. Something like this:

First, let use better name for the field

private int processingLargeReports;

and then

public bool GenerateReport(EstimatedReportSize reportSize)
{
    var currentDateTime = DateTimeFactory.Instance.DateTimeNow;
    bool allowLargeReports = 
       (Interlocked.Increment(ref this.processingLargeReports) <= Settings.Default.LargeReportLimit);
    if (!allowLargeReports)
        Interlocked.Decrement(ref this.processingLargeReports);
    var reportOrderNextInQueue = this.ReportOrderLogic.GetNextReportOrderAndLock(
        currentDateTime.AddHours(
        this.timeoutValueInHoursBeforeReleaseLock), 
        reportSize, 
        CorrelationIdForPickingReport, 
        allowLargeReports);
    if (allowLargeReports && !reportOrderNextInQueue.IsProcessing)
        Interlocked.Decrement(ref this.processingLargeReports);

    var currentReport = this.GetReportToBeWorked(reportOrderNextInQueue);

    var works = this.WorkTheReport(reportOrderNextInQueue, currentReport, currentDateTime);
    if (allowLargeReports && reportOrderNextInQueue.IsProcessing)
        Interlocked.Decrement(ref this.processingLargeReports);
    return works;           
}

Note that this also contains race conditions, but holds your LargeReportLimit constraint.

EDIT: Now when I'm thinking, since your processing is based on both Allow and Is Large Report, Interlocked is not a good choice, better use Monitor based approach like:

private int processingLargeReports;
private object processingLargeReportsLock = new object();

private void AcquireProcessingLargeReportsLock(ref bool lockTaken)
{
    Monitor.Enter(this.processingLargeReportsLock, ref lockTaken); 
}

private void ReleaseProcessingLargeReportsLock(ref bool lockTaken)
{
    if (!lockTaken) return;
    Monitor.Exit(this.processingLargeReportsLock);
    lockTaken = false;
}

public bool GenerateReport(EstimatedReportSize reportSize)
{
    bool lockTaken = false;
    try
    {
        this.AcquireProcessingLargeReportsLock(ref lockTaken); 
        bool allowLargeReports = (this.processingLargeReports < Settings.Default.LargeReportLimit);
        if (!allowLargeReports)
        {
            this.ReleaseProcessingLargeReportsLock(ref lockTaken);
        }
        var currentDateTime = DateTimeFactory.Instance.DateTimeNow;
        var reportOrderNextInQueue = this.ReportOrderLogic.GetNextReportOrderAndLock(
            currentDateTime.AddHours(
            this.timeoutValueInHoursBeforeReleaseLock), 
            reportSize, 
            CorrelationIdForPickingReport, 
            allowLargeReports);
        if (reportOrderNextInQueue.IsProcessing)
        {
            this.processingLargeReports++;
            this.ReleaseProcessingLargeReportsLock(ref lockTaken);
        }            
        var currentReport = this.GetReportToBeWorked(reportOrderNextInQueue);
        var works = this.WorkTheReport(reportOrderNextInQueue, currentReport, currentDateTime);
        if (reportOrderNextInQueue.IsProcessing)
        {
            this.AcquireProcessingLargeReportsLock(ref lockTaken); 
            this.processingLargeReports--;
        }            
        return works;
    }
    finally
    {
        this.ReleaseProcessingLargeReportsLock(ref lockTaken);
    }           
}

Thanks for the suggestion. Why is it more beneficial to use Monitor class instead of Locking in this case ? — Osman Esen, Sep 22 '15 at 06:55
Well, C# `lock` construct is just a syntactic sugar around `Monitor`, so essentially using any of them is considered a Locking approach. In this particular case, we need to be able to early release the lock in some cases (`!allowLargeReports` branch) while keeping it and releasing it later in other cases. The `lock` statement does not provide such a flexibility (in fact does not allow that), hence I used the direct approach. — Ivan Stoev, Sep 22 '15 at 07:13
Strongly speaking I needed that only for the "increment" part and could have used `lock` for the "decrement" part, but since I already needed a `finally` block, there was not benefit of doing that. — Ivan Stoev, Sep 22 '15 at 07:21
It seems like different tasks does not share processingLargeReports int variable. Even though it is incremented to 1 by Task 1, Task 2 still sees the value as 0, when it accessed the method — Osman Esen, Oct 05 '15 at 09:42
@OsmanEsen Hmm, that's strange. Are you sure the Task 1 haven't already finished and decremented the shared field? If that's not case, try marking the field as volatile (`private volatile int processingLargeReports;`) — Ivan Stoev, Oct 05 '15 at 10:12
Thanks for the answer. I have tried using volatile, but Task 2 still sees the value as 0. I have even removed the decrementation part, but the problem is still there — Osman Esen, Oct 05 '15 at 10:44
I'm using tasks insted of threads. Does it make any difference ? — Osman Esen, Oct 05 '15 at 10:48
@OsmanEsen: It doesn't matter as soon as they use one and the same instance of the class containing the code above. Are you sure you are not calling `GenerateReport` method on a different instances of your class? — Ivan Stoev, Oct 05 '15 at 11:13

Correct way to use the Interlocked class for multithreading in .NET

2 Answers2