HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Usage of Interlocked.CompareExchange for deciding whether a property has been changed by another thread

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
compareexchangebeenhasusagepropertyinterlockeddecidingforanotherthread

Problem

I'm developing a Windows Service that receives data from somewhere and saves it to DB one time per 5 minutes. The Service is multithreaded and I actually got problems when several threads saved data at same minute (no thread sync was implemented).

Now I've implemented it and am looking for code-review and optimizations.

class DataProccessor
{
    private int _lastMinutesUpdate;
    private readonly Timer _saveTimer = new Timer(TimerTick, null, _settingsProvider.SaveInterval, Timeout.Infinite);

    private void TimerTick(object state)
    {
        var watch = new Stopwatch();
        watch.Start();
        var initialLastMinutesUpdate = _lastMinutesUpdate;
        var currentDate = DateTime.UtcNow;
        var currentDateMinutes = currentDate.Minute;
        var fiveMinUpdate = currentDateMinutes % 5 == 0 &&
                            currentDateMinutes != initialLastMinutesUpdate &&
                            initialLastMinutesUpdate == Interlocked.CompareExchange(ref _lastMinutesUpdate, currentDateMinutes, initialLastMinutesUpdate);
        if(fiveMinUpdate)
        {
            // Insert values to DataBase
        }
        // TimerTick will be called in interval from settings minus time spent to TimerTick execution.
        _saveTimer.Change(Math.Max(0, _settingsProvider.SaveInterval- watch.ElapsedMilliseconds), Timeout.Infinite);
    }
}

Solution

In your code, the only way to have two threads update that value simultaneously is if TimeTick gets invoked twice in a very short amount of time. Given a sane value for your time interval, this can't happen. And if it can, you would typically use a Monitor and quickly exit the second handler.

It seems that you are invoking this timer at a much higher rate than necessary - every 200ms - because you want to do something else that frequently, so you have code to check if you should do your 5 minute job now. There isn't a good reason to have one callback responsible for two totally different things. You should be able to solve this by having a 5 minute timer for the database job and a 200 ms timer for the broadcast, and these would each call their own callback function.

Context

StackExchange Code Review Q#55863, answer score: 2

Revisions (0)

No revisions yet.