views:

282

answers:

4

I have a java object that implements Runnable. here is the code.

public class Obj implements Runnable {

    boolean shouldRun;

    private int stuff

    public Obj() {
     this.setshouldRun(true);
     stuff = 0;
    }

    public synchronized void setshouldRun(boolean shouldRun) {
     this.shouldRun = shouldRun;
    }

    public synchronized boolean getshouldRun() {
     return this.shouldRun;
    }

    @Override
    public void run() {
     while (shouldRun) {
      //do stuff
     }
}

}

here is how i use the Obj.

Obj o = new Obj();
//q is a collection of Obj
q.add(o);
new Thread(o).start();

when want to remove o, may I safely do this:

o.setshouldRun(false); //thread will finish
q.remove(o);
+3  A: 

You need to synchronize all access to shouldRun, not just write access. You can do this by adding a synchronized getter isShouldRun rather than accessing shouldRun directly on this line:

while (isShouldRun())

Alternatively you can make shouldRun volatile.

Failing to do this can lead to changes to the value of shouldRun by one thread not being noticed in the other thread.

Mark Byers
Addendum: if you declare the field `volatile`, you can remove `synchronized`.
meriton
how about this scenario: The t get pre-emptied after while while (isShouldRun()). Then isShouldRun is changed and o is removed from q. what would happen next? would the thread just die because while condition is change or the thread would resume and die after checking the condidtion?
Quincy
The thread will terminate after it checks the condition, sees that it is false, breaks out of the loop, and reaches the end of the run method.
Mark Byers
is there a way to kill the thread immediately after removing o from the q?
Quincy
There isn't a supported way to murder a thread external, was depreciated a while ago. But if you need to wait for the thread to be finished before you continue on doing normal processing, call Thread.join() and wait for that call to return. (This would imply that you keep a reference to both the Thread and the runnable instead of just the runnable)
Jason Faust
+1  A: 

As long as nothing comes after q.remove(o) that would depend on o not being in a running state, you should be ok. But remember, the way you have your code, o can continue running long long after it is removed from q, because you haven't stopped to check that it has finished running before removing it from q. You've only notified it that it should stop running once it gets back around to checking that condition.

Zak
+1  A: 

The best bet would be to define shouldRun as volatile as synchronizing is slower. Assignments are not guaranteed to propagate to other threads unless dictated by the use of volatile and synchronize.

If you have access to o's thread, you can get rid of your boolean "shouldRun" by just using the thread's native "interrupt" method:

Obj o = new Obj();
q.add(o);
Thread t = new Thread(o);
t.start();

t.interrupt();
q.remove(o);

Your runnable would then have to read:

while (!Thread.interrupted()) {
}

Instead of:

while (shouldRun) {
}
mlaw
how about this scenario:The t get pre-emptied after while (!Thread.interrupted()). Then we call t.interrrupted() and o is removed from q. what would happen next? would the thread just die because it's interrupted or the thread would resume and finish the while loop then die?
Quincy
A: 

I think using java TimerTask would be a better solution. TimerTask could be cancelled by other threads. so the TimeTask thread will exist pretty much immediately, so no extra work would be done.

Quincy