Link to home
Start Free TrialLog in
Avatar of learningunix
learningunix

asked on

kill thread

Is it a good practise to kill a thread in a big running application. I spawn a thread withing a class and run it in while(1) loop. In the destructor of the class instead of join() I need to kill the thread so that all the resources are released. is that bad to kill thread?
SOLUTION
Avatar of sarabande
sarabande
Flag of Luxembourg image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
>> is setting a shared bool flag to true

make sure you declare it volatile otherwise it may be optimised and cached so the changes are not see x-thread.
http://drdobbs.com/184403766
Avatar of learningunix
learningunix

ASKER

The problem is the thread sleeps every 60 seconds and then wakes up. In that case, I'll have to wait 60 secs which is not good.

Why can't I directly kill the thread.

If it is good, what is the best way to kill the thread created  by

create(&myThread, &myAttr, (void* (*)(void*)) &myLoop, (void*) this);
>> Why can't I directly kill the thread.

If you kill it whilst it's doing something important (like writing to a file) it'll cause inconsitancies.

>> The problem is the thread sleeps every 60 seconds
Why not use a timed event? A kernel primitive that blocks for a time but can be unblocked on demand. How you do this depends on your OS but boost has a condition variable you could use.

http://www.boost.org/doc/libs/1_46_1/doc/html/thread/synchronization.html#thread.synchronization.condvar_ref.condition_variable
I can definitely terminate in the main thread inside a function instead of destructor but is that recommended
There is never a good/safe way to "kill" a thread. If your code demands this you need to review your code as the design is wrong.
Currently the thread does sleep(60); not sure how can I signal this thread while its sleeping.
The idea is you remove the sleep and wait on the condition variable instead using a timed wait. If you wish to exit early you signal the condition variable. If the timed wait returns true it was due to the condition being set (ie you want it to exit) if it's return value is false it means the timed wait expired so you just do what you would have done when your sleep expired.
void  myLoop()
{
    while (1)
    {
       .....
       ....
       sleep(60);
   }
}
 
is there a way to do conditional sleep where I can wake up this thread if some condition occurs?
 
See my comment above.
@evilrix:  but I need to re-execute the loop every 60 secs. so sleep is must.
Its just at the end I need to stop this thread
you could make a loop and sleep 1 second each circle. there you can check the stop flag.

Sara

ASKER CERTIFIED SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
Will this work?

while (1)
  {
      .....
      cond_timedwait(&myWait, &myMutex, &myTime);  //myTime will be current time + 60 secs
      //If the return value is true means, it timed out and I continue the execution
       {
             //  continue the execution
        }
      // if the return value is false, it means someone else has send us the signal, I can break the while
         {
            break;
         }
}

oops, I didn't see ur previous comments while posting.
That's the idea except (assuming you plan to use boost condition variable) your boolean logic is inverted.

"timed_wait returns false if the call is returning because the time specified by abs_time was reached, true otherwise"

>> oops, I didn't see ur previous comments while posting.

:)
Here's the final code I am planning:


void cleanUp() {
   cond_signal(&myCond);
}


void myLoop() { 
  while (1)
  {
      .....
      cond_timedwait(&myWait, &myMutex, &myTime);  //myTime will be current time + 60 secs
      //If the return value is FALSE means, it timed out and I continue the execution
       {
             //  continue the execution
        }
      // if the return value is TRUE , it means someone else has send us the signal, I can break the while 
         {
            break;
         }
  }

}

Open in new window

THis

void cleanUp() {
   cond_signal(&myCond);
}


should be

void cleanUp() {
   cond_signal(&myWait);
}
That looks like the kind of thing you need to do. So when you call cleanup it will signal the condition, cause the timed wait to abort and your thread will clean up. Don't forget you need to wait for the thread to re-join though, otherwise you run the risk of terminating your program before the thread has completed. You might want to add that in the cleanup. For example (and I am not claiming this is syntactically correct.... it's just an example):

void cleanUp() {
   cond_signal(&myCond);
   myThread.join();
}
I guess one problem, currently I am using the same mutexLock during signalling, will that be a problem?

bool stopthread = false;
void cleanUp() {
   mutex_lock(myMutex);
     cond_signal(&myWait);
   mutex_unlock(myMutex); 

   // Instead of join can I do this
   while (!stopthread); // this will guarantee thread finishes. 
}


void myLoop() { 
  while (1)
  {
      .....
      cond_timedwait(&myWait, &myMutex, &myTime);  //myTime will be current time + 60 secs
      //If the return value is FALSE means, it timed out and I continue the execution
       {
             //  continue the execution
        }
      // if the return value is TRUE , it means someone else has send us the signal, I can break the while 
         {
            stopthread = true;    
            break;
         }
  }

}

Open in new window

Also what if the thread is in the middle of the execution inside the if loop and then cleanUp() invokes signal. in that case, the thread is not waiting on cond_timedwait() and it won't catch the signal at all ??
Why do you need a mutex to protect cond_signal in cleanup? You don't need to do that, it should already be thread safe if you are using a proper privative.


>> while (!stopthread);
This is really not the way to wait for a thread to end. Your thread library will have a proper way to join a thread. This will just chomp CPU and could even prevent the thread from ending!
>> Also what if the thread is in the middle of the execution inside the if loop and then cleanUp() invokes sig

The timed_wait will end immediately when you call it as the condition will be set. It is designed to handle this case.
ohh...i see i thought I need mutex for sending signal also.

Also what if the thread is in the middle of the execution inside the if loop and then cleanUp() invokes signal. in that case, the thread is not waiting on cond_timedwait() and it won't catch the signal at all ??
i guess this should be good
void cleanUp() {
     cond_signal(&myWait);
     join(&thread);
}


void myLoop() { 
  while (1)
  {
      .....
      cond_timedwait(&myWait, &myMutex, &myTime);  //myTime will be current time + 60 secs
      //If the return value is FALSE means, it timed out and I continue the execution
       {
             //  continue the execution
        }
      // if the return value is TRUE , it means someone else has send us the signal, I can break the while 
         {
            break;
         }
  }

}

Open in new window

Better :)

>> Also what if the thread is in the middle of the execution inside the if loop and then cleanUp() invokes signal.
See my comment above, this is fine. It will correctly handle this.

thx a lot!
You're welcome.
      If (status == 0)  // it timed out and I continue the execution
       {
             //  continue the execution
        } else  // , it means someone else has send us the signal, I can break the while
         {
            do I have to unlock the mutex ??
            break;
         }

>> do I have to unlock the mutex ??

You actually wait on a scoped lock not a mutex. The lock already has the mutex. When the thread ends the scoped lock releases the mutex.

There is an example here.
Here's what I am doing. am i releasing the lock properly?
void cleanUp() {
     cond_signal(&myWait);
     join(&thread);
}


void myLoop() { 
  while (1)
  {
      .....
      mutex_lock(&myMutex);   

      cond_timedwait(&myWait, &myMutex, &myTime);  //myTime will be current time + 60 secs
If (status == 0)  // it timed out and I continue the execution
       {
             //  continue the execution
        }
       else 
         {
            break;
         }
  mutex_unlock(&myMutex )
  }
   
}

Open in new window

int status = cond_timedwait(&myWait, &myMutex, &myTime);
I thinks so lock is not released during break;   ??
also I believe the condition should be inverted ??
cond_timedwait automatically releases the mutex, and waits on the condition variable cond. When the condition is signaled or the time expires, cond_timedwait reacquires the mutex and returns to the caller. The wait can also be interrupted by a UNIX system signal, in which case mutex is reacquired, the signal handler is called, and cond_timedwait returns EINTR.


I guess I need to unlock_mutex just before the break