views:

307

answers:

5

Hi Guys, I have created a windows service which is currently having three timers. First timer is waking up every 15 sec, second timer is waking every min. and the third timer is waking everyday.

THe problem is these are spawning new threads every time and one time the threadpool gets used up completely.Is ther any to just spawn 3 threads and not spawn any more new threads.

My code looks something like this:

protected Onstart()
{
  var timer1  = new TImer();
  timer.Elapsed += Event1;
  timer1.interval = 60000;
  timer1.start();

  var timer2  = new TImer();
  timer2.Elapsed += Event2;
  timer2.interval = 60000;
  timer2.start();
}

private Event1(object,elapsedeventargs)
{
  var workerthread1 = **new thread**(workerthreadfunc1)
  workerthread1.start();
}

private Event2(object,elapsedeventargs)
{
  var workerthread2 = **new thread**(workerthreadfunc2)
  workerthread2.start();
}

So as you can see it is creating new threads which will use up all the threads in threadpool at some point and stops the windows service abruptly. Currently it is stopping and loogging a evet log with event ID 5000.

+1  A: 

Instead of spawning new threads, use the ThreadPool.

zac
THnx zac for the advice
alice7
+1  A: 

I would change the design to not include any timers. On start, create the 3 threads, and put a loop around the work done in workerthreadfunc1 and workthreadfunc2 with a thread.sleep for the appropriate amount of time (15 seconds, 1 minute, etc...) at the beginning or end of the loop. You might want to add some check at the beginning of the loop to see if someone is trying to stop the service, or if another one has already started.

marr75
+1  A: 

You could try only creating the thread if it hasn't already been created. Declare workerthread1 at the class level and do something like:

if(workerthread1 != null)
{
      workerthread1 = new thread(workerthreadfunc1);
}
msergeant
That code won't work, workerthread1 hasn't been instantiated, he could make it a variable of whatever class/module he's doing this work in, however.
marr75
Sorry. Should have read that one more time before submitting :)
msergeant
+4  A: 

Your problem is not with the ThreadPool. you're creating your own threads each time one of your timers "ticks". I don't know what workerthreadfunc1 and workerthreadfunc2 are doing but if they don't return than you'll keep creating more and more threads.

If you are using System.Timers.Timer than the Elapsed event is already on a ThreadPool thread. why not perform your desired operation there?

Moshe Levi
Thnx for the advice, i hope after using threadpool the service doesn't stop again :)
alice7
@alice: As I said, you receive the elapsed event in a thread pool's thread so you don't need to queue another work item for the pool. but I would first check if something is wrong with the thread functions. It seems like they never return.
Moshe Levi
I am currently using try and catch in each worker thread function. But looks like it is not catching the exception at all.
alice7
Can you please post the code of those functions?
Moshe Levi
+1  A: 

@marr75 @zac

both of these are good advice, but why not:

int x = 0; //last exec timestamp(ts)
int s = 0; //15 s ts
int m = 0; //min ts
int d = 0; //day ts
while(check for event()){ //e.g. stop service etc.
   if((x - s)>15){
      //code for ever 15 s here
      s = currentTime();
   }
   if ((x - m)>60){
      //code for every min here
      m = currentTime();
   }
   if ((x - d)> 86400)(
      //code for every day here
      d = currentTime();
   }
   sleep(5000); // or wait() w/e sleeps the thread for 15s
   x = currentTime();
}

this however assumes that your doing a relatively light task if your doing a heavier task like a SQL query and insert or something or other just make sure the thread kills itself when done.

You could be getting possible overflow because your 15s and min worker threads take longer than 15s and a min respectively to execute, if this is true you will be adding threads faster then they terminate eventually leading to an overflow as msergeant was trying to say, only add a new thread if your old worker thread is finished.

JERiv