0

Ok, I was asked this multi-threading question in an interview. The question was something like this

public class Job {

    static boolean interruptTask = false;

    private static class Mytask extends Thread{
        @Override
        public void run() {
            while(!interruptTask){
                //do some time consuming thing until interruption like looping over millions of times

            }
        }
    }

    public static void main(String[] args) throws InterruptedException {
        Mytask t = new Mytask();
        t.run();
        Thread.sleep(1000);
        interruptTask=true;
        t.join();
        System.out.println("end");
    }
}

Question statement

What is wrong with this code snippet and how will you fix it.

I was able to identify the problem here. The problem in my opinion is that even when we make the interrupttask variable true in the main function, it does not interrupt the thread. The thread will keep on doing until its long time taking execution is not over.

But I can't figure out the solution for this problem? I mean, we have tools like wait,notify for thread, but I don't those are somethings we can use here. What should be the approach to resolve this particular issue in this scenario?

Can anyone help me figure out the solution of this problem? or some guidance regarding how should I go about for the solution?

Max
  • 9,100
  • 25
  • 72
  • 109
  • @BoristheSpider loop was not in the question, I added it to represent the time taking task. I will edit the quesiton – Max Aug 24 '14 at 12:45
  • 1
    I think the bigger issue here is race condition on `interruptTask`. This field is non-volatile and is accessed from multiple threads unsynchronized. – Piotr Praszmo Aug 24 '14 at 12:46
  • 1
    Well, a `Thread` in Java can only be stopped if it _cooperates_. Your task needs to be _interruptible_. You need to split the task into chunks and check a flag in-between processing. It's best not to use your own flag but to use the already existing [Interrupt API](http://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html). – Boris the Spider Aug 24 '14 at 12:49
  • @Banthar I agree interrupttask is being used in multiple thread unsynchronized. But I can't really visualize what making it synchronized will do? I mean what id meaning of race condition here? – Max Aug 24 '14 at 12:52
  • 1
    @Dude please do yourself a favour and read about [`volatile`](http://stackoverflow.com/a/106787/2071828) in Java. There **is** a race hazard here. This is why I said that you should use the Interrupt API instead. But if you do not know about atomicity/visibility I would **strongly** suggest you read up before doing any threading at all! – Boris the Spider Aug 24 '14 at 13:09

4 Answers4

2

There are two problems:

  1. t.run() should be t.start(). Otherwise you are doing both tasks in single thread.

  2. interruptTask should be volatile or should be accessed through synchronized getter/setter. Otherwise, Java is allowed to assume only one thread is using that field and optimize:

    while(!interruptTask){
        // something
    }
    

    into:

    if(!interruptTask){
        while(true){
            // something
        }
    }
    
Piotr Praszmo
  • 17,928
  • 1
  • 57
  • 65
  • The "while (true)" loop will loop indefinitely, never exiting ... . – ErstwhileIII Aug 24 '14 at 13:01
  • @ErstwhileIII Yes. That's whats happening in Dude's program. Java sees that `interruptTask` is never changed and optimizes while loop into if+infinite loop. That is a valid optimization, assuming `interruptTask` is not written to from other thread. Unfortunately Java cannot tell that this assumption is false. – Piotr Praszmo Aug 24 '14 at 13:08
  • @Banthar not quite - that is an oversimplification. The JVM will _cache_ the value of `interruptTask` in a thread's local cache. Then a thread will not see updated values of the variable. But +1 for the rest. – Boris the Spider Aug 24 '14 at 13:19
0

Changed answer relevantly to the change in question.

After Edit

private static class Mytask extends Thread{
  public void run(){

     while(true){
       if(Thread.currentThread().isInterrupted()){
          System.out.println("Thread Interrupted");
          break;  
       }
        //do stuff here
     }
  }
}

public static void main(String[] args) throws InterruptedException {
    Mytask t = new Mytask();
    t.start();
    try{
       Thread.sleep(1000);
    }catch(InterruptedException e){

    }
    System.out.println("Interrupting now");
    t.interrupt();
    System.out.println("end after interruption");
}
Sumeet Sharma
  • 2,573
  • 1
  • 12
  • 24
0

One simple solution would be to ensure that the "long-running" task does break periodically to allow another thread to set the interrupt flag. Often this is done by "sleeping" for some short period of time after some predefined interval. You will also want to be able to segment the "long running" code. Pseudo code below

   // interruptTask should be declared like volatile boolean interruptTask
    long interval = <in milliseconds>
    long lastPause = <system time>
    while(!interruptTask){
        if (<time now> - lastPause > interval) {
            sleep(pausePeriod);
            lastPause = <system time>
        } else {
          ... long running work increment (not do this a billion times)
        }
    }
ErstwhileIII
  • 4,829
  • 2
  • 23
  • 37
0
public class Job {

   static boolean interruptTask = false;

   private static class Mytask extends Thread{
      @Override
      public void run() {
         while(!interruptTask){
            //do some time consuming thing until interruption like looping over millions of times
         }
      }
   }

   public static void main(String[] args) throws InterruptedException {
      (new Mytask()).start();
      Thread.sleep(1000);
      interruptTask=true;
      System.out.println("end");
   }
}

In your code, a new thread was not being created when you started MyTask, thus thread.sleep(1000) nor interruptTask=true was being executed because the while loop was being executed, thus you could not interrupt the loop.

Oliver Funk
  • 166
  • 1
  • 15