0

This question follows on from the development of an issue originally posted here. The issue is with editing the properties of an object from multiple-threads.

The context of the application is as follows:

I have a System.Timers.Timer object, which does the below numbered items on every tick. I need this to be done with a timer because I may want to be able to vary the tick interval from 1ms to 60s.

On every tick event:

  1. Run a background worker to read data from a file [order of ms] (or a URL [order of seconds])
  2. When background worker finishes reading data from file, raise an Event (say Ticker_Updated_Event)
  3. In the event handler of Ticker_Updated_Event, run object code which updates a certain property (say this.MyProperty). Additional code is executed in this event handler, including calls to update the GUI [which I believe I have made thread safe (at least i hope)].

Various suggestions have been made including using shared variables and interlocking, but I feel that the quickest(?) and potentially the most robust solution in terms of keeping the rest of my code functional, would be one where I simply force Timer to wait for the code in its tick handler to finish before executing another tick. Can this be as simple as attaching a boolean expression to the start and end of the tick handler?

EDIT 1: I would like the timer to tick as soon as code has finished executing in its handler. I am interested in the performance of my application when running on 1ms tick intervals. I expect the code in the event handler to execute fairly quickly and do not see this being an issue when using it with intervals greater than say 200ms.

EDIT 2: I think I wasn't very clear in setting out the scope of my functionality. I essentially will have 2 states under which my program will run. At any run time, the program will either be:

  1. Using tick intervals >> time needed for execution of code in the handler, which will include doing http requests etc, so no issues here.

OR

  1. Using very short ticks ~1ms to read data from file and run the exact same code as in the first scope, where a loop would easily suffice but mean having to modify the code in the first scope item.
Community
  • 1
  • 1
rex
  • 3,133
  • 6
  • 35
  • 62
  • why must you use a timer if you are executing on every tick? – T McKeown Mar 04 '14 at 21:48
  • 1
    So lets say that your timer ticks ever 5 seconds, and one operation takes 2 seconds. Should the timer next tick in 3 seconds, or in 5 seconds? If the tick event takes 9 seconds should it tick again in 2 seconds, or in 5 seconds? (Or some 3rd option I didn't list?) – Servy Mar 04 '14 at 21:49
  • @TMcKeown To wait some period of time before performing the "next" operation. – Servy Mar 04 '14 at 21:49
  • i guess using timer if you are going to update the UI is required. – T McKeown Mar 04 '14 at 21:50
  • @Servy I have edited the question to answer you. – rex Mar 04 '14 at 21:57
  • @ArmenSafieh-Garabedian You still haven't answered my first comment. It's currently ambiguous which behavior you want. You've answered my second. – Servy Mar 04 '14 at 21:57
  • 2
    If you want it to tick right away, then you really *don't* need a timer. Just have the completion of the previous event start the next event. If you don't actually want to wait, don't use the timer. – Servy Mar 04 '14 at 22:00
  • It sounds like the timer is being used as a polling agent. It's a heartbeat to check for new work. So it sounds like it doesn't matter if the next beat is the difference between when it last ran and the schedule versus just waiting for the next tick. – Erik Noren Mar 04 '14 at 22:01
  • @Servy, I do however want to be able to modify the length of the tick intervals, which I guess means I would have to use Sleep() statements? – rex Mar 04 '14 at 22:02
  • @ArmenSafieh-Garabedian If you actually want to wait for some time after finishing the previous task, then a `Timer` could well be appropriate. If so, you'd simply adjust the interval, not `Sleep`. – Servy Mar 04 '14 at 22:03
  • @ErikNoren If I understand what you are saying correctly, then you are.... correct; haha. I am simply using Timer as an agent to keep my application running (as opposed to a whie(true) statement of some sort with .Sleep() statement... – rex Mar 04 '14 at 22:04
  • You're polling. Tick-> Is there any work? -> Do work -> Update UI. Sounds like you want Tick-> Am I already busy? -> Is there any work? -> Do work -> Update UI – Erik Noren Mar 04 '14 at 22:05
  • @ErikNoren, the only reason I am using a background worker for scope 2 (when reading from file) is to have the code be synonymous with scope 1 item (reading from URL). Essentially, scope 2 item utilizes an inherited class from the scope 1 item that simply overrides the background worker's request code from http request to to .ReadLine() – rex Mar 04 '14 at 22:11
  • @Servy, Yes, I do want to wait, but not always; see edit 2 :) – rex Mar 04 '14 at 22:26
  • @ErikNoren I think you solution here (http://stackoverflow.com/a/22182667/2700593) actually is fit for my problem as I think I can allow skipping Ticks as long as I make sure no other code (that relies on tick data) is executing meanwhile. The program is unfortunately to large and complex to try and get a solution with a simple SO question :( I will add an edit to my question to perhaps help you post an answer :) – rex Mar 04 '14 at 22:41

3 Answers3

1

The below will certainly skip ticks, the only other alternative I can think of would amount to an event queue solution. (Which would be more like lock() { push(); } and probably use another thread to do the reading and processing lock() { o=popornull(); } // use o

class TickHandler
{
   static object StaticSyncObject = new object();
   static bool IsBusy = false;

   private TickHandlerObject myHandler;

   static void HandleTimerElapsed(object source, ElapsedEventArgs e)
   {
      lock(StaticSyncObject)
      {
          if( IsBusy ) 
             return;
          else 
             IsBusy = true;
      }
      try {

          // ... do some work here ...  hopefully often faster than interval.

          myHandler.TickerUpdated();
                           // without any IsBusy checks
                           // without thread safety (1 thread at a time calls)
                           // Note: If you're trying to split up threads below
                           //       this, that's up to you.  Or diff question.
                           //       ie: file/URL read vs memory/GUI update

      } finally {
         lock(StaticSyncObject)
         {
            IsBusy = false;
         }
      }
   }
}

PS - Don't necessarily lose heart on the Timer notion. A good timer will have much less clock skew than the average while loop. (I'm assuming clock skew is important here since you're using a timer)

Note: Careful thread safety of memory push from the "data read" code, which could execute under this thread, and proper use of Control.Invoke and/or Control.BeginInvoke should allow you to complete your task without any other "events" needing to be fired.

Performance Note: lock may take around 50 nanoseconds with low contention: http://www.informit.com/guides/content.aspx?g=dotnet&seqNum=600. So, this code should be fine for millisecond resolution. Note: The System.Threading.Interlocked methods may drop some operations to only 6 ns instead of 50 ns, but that difference seems minor in this instance, especially given the complexity of using Interlocked. (see: http://www.dotnetperls.com/interlocked)

  • If you expect to throw away a lot of ticks, you could save some time by checking IsBusy before the lock. Otherwise you'll be incurring some time while threads are stacking up trying to get the lock. Hopefully the acquiring of the lock is quick enough compared to the interval of ticks that this won't be a problem but it's really not that much more to add a check. If this happens, you could be delaying setting the IsBusy back to false and introducing delay between executions by having so many threads trying to acquire the lock just to see if they need to work. – Erik Noren Mar 04 '14 at 22:55
  • Thanks guys, could you kindly comment on my solution as well? I am curious :) – rex Mar 04 '14 at 23:03
  • @ErikNoren That sounds a lot like double-check-locking http://en.wikipedia.org/wiki/Double-checked_locking. 1) make it work. 2) (less important) make it fast. 3) I'm pretty sure the average `lock()` contention is much much faster than 1 ms. (In fact, I've used `lock()` in debug print statements before... Notice the `lock()` is not held during "do work". It's only open long enough to protect the `IsBusy` flag check, set and clear. –  Mar 05 '14 at 14:25
  • @ebyrob Yes, I see you're not holding the lock. The concern I had was, given the way the question seems to evolve into higher orders of complexity than the original question starts with, I didn't think it was a safe assumption to think this code would only be used in the single timer callback. People who aren't 100% sure what they're doing will tend to copy and paste things around a bit compounding the risk for contention. You're right in that it's probably not likely to happen if done the way we expect it to be done. Just not sure that's a safe assumption here. – Erik Noren Mar 05 '14 at 16:36
1

Going from your previous question and these updates, it sounds like you might want to be aggressive in throwing away ticks. This is based on the comment that the code is too complex to be fully aware of where data changes are happening in multiple threads to make the changes needed to synchronize the use of shared members.

The pseudo code I posted in the previous question which was expanded by ebyrob would be a reasonable choice assuming you haven't kept hidden from us that this application is running multiple times in different application domains!

If you wanted to try to shim something into your existing solution, try something like this:

class UpdateThrottler
{
    static object lockObject = new object();
    static volatile bool isBusyFlag = false;

    static bool CanAcquire()
    {
        if (!isBusyFlag)
            lock(lockObject)
                if (!isBusyFlag) //could have changed by the time we acquired lock
                    return (isBusyFlag = true);
        return false;
    }

    static void Release()
    {
        lock(lockObject)
            isBusyFlag = false;
    }
}

Then in your code, you could do something as simple as:

Tick_Handler(...)
{
    if (UpdateThrottler.CanAcquire())
    {
        try
        {
        //Do your work
        }
        finally
        {
            UpdateThrottler.Release();
        }
    }
}

This has some flexibility in that you can call Release from another location if for some reason you aren't sure to be done working at the end of the Tick handler. This could be because you use a background worker whose callback is used to finish the remaining work or whatever. I'm not entirely sure it's a good practice but short of spending the time to understand your application in its entirety, I'm not sure what else could be done.

At this point we're getting dangerously close to reproducing the (Manual)ResetEvent object.

EDIT: After some discussion I realized it doesn't hurt to have the volatile marker and could ease some potential edge case somewhere.

Erik Noren
  • 4,279
  • 1
  • 23
  • 29
  • Thank you for taking the time to post this answer. I believe I am now out of my depth and will need to spend some time understanding what you've written. Expect an update later tonight or tomorrow :) – rex Mar 04 '14 at 23:29
  • Auto/ManualResetEvents are basically simple 2 state semaphores. You're either set or you're not. It's a bit more complex under the covers making use of different wait strategies depending on if you get blocked during a wait for a very short time or a long time. It's slightly more complicated to use than what I've written but it's safer. The drawback is it's designed to make things wait which is not what you wait - you want to bail out if you're already busy. So it's not a perfect fit for what you want but we're starting to recreate its functionality but in a poor way! :) – Erik Noren Mar 04 '14 at 23:34
  • haha trying to figure out how to use this. I made a class called UpdateThrottler and I am simply doing my work in the if (UpdateThrottler.CanAcquire()){} block? – rex Mar 04 '14 at 23:42
  • Yeah. If CanAcquire returns true, it's ok for you to do work. If it is false, something else is already working. Then you call Release when you're done and ready for the next tick to get processed. You can move the Release to wherever the last of your code finishes. Either in the Timer event where I've shown it or in the callback of your background worker if that's the last thing that runs. – Erik Noren Mar 05 '14 at 02:09
  • @ErikNoren The second example uses the very definition of double-check-locking which (last I checked) doesn't work for .Net. On the bright side, `lock()` overhead isn't nearly as bad as you might think. –  Mar 05 '14 at 14:37
  • @ebyrob What do you mean it doesn't work for .NET? There's nothing magical about it - just a fail-early approach. I wasn't sure where the code was going to end up and if the callback was being triggered by more than just the single timer so I figured, given it's a straightforward bit of code, there was no harm to reducing contention. We don't care if we can get the lock if we're not going to do work anyway. In the release handler we don't double-check because we know we want to clear the flag. I'm actually wondering if we need to take the lock at all on release. – Erik Noren Mar 05 '14 at 16:30
  • @ErikNoren see here: http://stackoverflow.com/questions/394898/double-checked-locking-in-net and more generally here http://en.wikipedia.org/wiki/Double-checked_locking Folks a lot smarter than I seem to think it's foolhardy at best. How the compiler, VM, OS, and processors work together for thread safety is a changing landscape. Pushing too hard often causes leaky abstractions. Personally, if I felt I really had to reduce the 100 nanosecond per tick cost associated with locking here, I'd work on reducing the thread count. (ie: Do away with the timer, 1 thread-loop) –  Mar 05 '14 at 16:47
  • @ebyrob They seem to be talking about the 1.1 framework which did require declaring the shared variable volatile which hasn't been necessary since .NET 2.0. Further, the bullet points say the major downside is the trouble to get it right `The pattern needs to be pretty much exactly as above...` which it is. There's a lot of soft statements which lead to an overall critique of the pattern in favor of a lazy approach which is harder to communicate to someone who doesn't understand a timer uses threads. – Erik Noren Mar 05 '14 at 16:55
  • @ebyrob Keep in mind the question you've linked has a better answer below which says they are actually fine though you have to pay attention. They're also specifically trying to implement a singleton pattern with full thread safety which is a bigger beast than synchronizing access to a boolean flag. – Erik Noren Mar 05 '14 at 16:58
  • @ErikNoren Honestly, I could probably write it to work for a 1 MS timer with no synchronization... But, it'd be poor practice, and it'd be poor teaching. The thread I linked says the double-check-lock pattern requires `volatile` which I don't see in your code, it alludes to other things I'm not even sure I understand which are required for it to work... I actually at one time wrote a C# singleton with double-check-lock until finding out it was incorrect... Believe it or not, the timer is just as likely to fail as the singleton, though probably not as often. C# singleton is merely 1 pointer –  Mar 05 '14 at 17:12
  • Yes, for Java it appears it is required and as well for .NET 1.1 - I thought I addressed that. I think you might be reading into the words and maybe not knowing what the implication is and drawing a harsher conclusion. For sure this pattern isn't optimal and I wouldn't recommend it if we were designing the asker's software but we're seeing a small snippet of code and asked to help. This is honestly the best solution I could think of which required the least amount of changes while reducing the risks of contention, collision and corruption. We can go to chat if you want to keep discussing. – Erik Noren Mar 05 '14 at 17:18
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/49079/discussion-between-ebyrob-and-erik-noren) –  Mar 05 '14 at 17:19
  • Thank you for all the comments guys; I would be interested in giving more information about the wider application in a chat if you guys have the time; please let me know. @ebyrob – rex Mar 05 '14 at 18:49
  • @ArmenSafieh-Garabedian I'm definitely thinking about it. Seems Erik may be as well, but I won't have time to get into it till maybe tonight. –  Mar 05 '14 at 19:45
0

I thought I would try and answer my question as well with a solution I have attempted. Please let me know if you see any potential pitfalls in this solution:

This class raises events based on its Timer object executing code:

public class Ticker{

    private TickHandlerObject myHandler; // injected dependency

    //bw is a background worker

    // Handle the Timer tick here:
    private void OnTimedEvent(object source, ElapsedEventArgs e)
    {
        if (myHandler.IsBusy == false) // check here if the handler is still running
        {
            // do work with background worker, that could take time...
        }
    }

    private void bw_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
            // raise event to be handled by myHanlder object
            Updated();
        }

    }
}

This class instaniates the ticker and handles its events.

public class TickHandlerObject{

    // This class subscribes to the Updated event of myTicker.
    private Ticker myTicker;
    myTicker.Updated += this.TickerUpdated;
    public bool IsBusy{get;set;}

    private void TickerUpdated()
    {
        // thread-safing
        this.IsBusy = true;

        // do stuff that can take longer than the tick interval

        //  thread-safing
        this.IsBusy = false;
    }
}
rex
  • 3,133
  • 6
  • 35
  • 62
  • What is `private Ticker myTicker;` ? It doesn't appear to be anything. Is `TickerUpdated` called from a Timer Tick? If so, you'll have similar problems of race conditions around the IsBusy flag without locking. – Erik Noren Mar 04 '14 at 23:04
  • Yes, this is 'psuedo' code, i have omitted the Timer code, only left its Tick handler inside Ticker class. – rex Mar 04 '14 at 23:06
  • @ErikNoren would declaring IsBusy as volatile fix that issue? since it won't be modified but only read by Ticker? – rex Mar 04 '14 at 23:14
  • I'm not sure but I'm inclined to say no. The way you have the TickerUpdated method now, you are always setting to true when you start work and false when you're done without checking if you can set to true. This would require a comparison check at some point. The time between check and set could allow two threads to see IsBusy as false, both set true and do work at the same time. It didn't do its job of letting only one through at a time. – Erik Noren Mar 04 '14 at 23:24
  • `volatile` will make sure the variable is eventually updated in each thread. (not optimized away from that) However, by itself a single volatile variable can usually only do one operation at a time like "check" or "set". In this case you must do "check and set" as one operation or there will be timing glitches. (ie: windows of opportunity for two threads to run at the same time) –  Mar 05 '14 at 15:07