patterncsharpMinor
Usage of Interlocked.CompareExchange for deciding whether a property has been changed by another thread
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.
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
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.
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.