3
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ How exactly are you running TimerTick? \$\endgroup\$ – svick Jul 4 '14 at 16:16
  • \$\begingroup\$ @svick Thanks for your reply! I've extended my code, please check it out. \$\endgroup\$ – Zharro Jul 7 '14 at 9:44
  • \$\begingroup\$ Your extended code won't compile. Could you post code that actually works? \$\endgroup\$ – svick Jul 7 '14 at 9:47
  • 2
    \$\begingroup\$ What I don't understand about the code: 1. Why the complicated logic around SaveInterval and 5 minutes? If the save should be done every 5 minutes, why not use that as the timer interval? 2. Why do you use CompareExchange to update _lastMinutesUpdate? Is there some other code that could change it? \$\endgroup\$ – svick Jul 7 '14 at 12:56
  • \$\begingroup\$ Are there multiple DataProcessor objects, or just one? Are there multiple Timers calling each instance's TimerTick or just one? \$\endgroup\$ – Snowbody Jul 18 '14 at 18:21
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ My interval is 200ms. Ok, I can and I actually replaced Interlocked.CompareExchange by lock, but the question was what's wrong with my code (I still can't find a place where synchronization breaks). \$\endgroup\$ – Zharro Jul 28 '14 at 13:34
  • 1
    \$\begingroup\$ You have a 200ms timer for a task that you want to happen every 5 minutes??? \$\endgroup\$ – Pierre Menard Jul 28 '14 at 16:27
  • \$\begingroup\$ Just set the interval to 5 minutes and get rid of those pointless comparisons. \$\endgroup\$ – Pierre Menard Jul 28 '14 at 16:30
  • \$\begingroup\$ One more time: I need to save values to DB every 5 minutes and send current (actual) values to all connected clients every SaveInterval ms. \$\endgroup\$ – Zharro Jul 29 '14 at 8:22
  • 1
    \$\begingroup\$ Sorry, I didn't see that in the code. Why don't you have 2 timers for 2 different jobs? \$\endgroup\$ – Pierre Menard Jul 29 '14 at 20:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.