-2

In below code I am trying to use multiple threads to access queue until isFound = false. I have declared isFound variable as volatile and I am changing its value to true after some condition. If I change its value in one thread and by definition its updated value should be visible to all other threads. When I run this code, the value of isFound is still false.

If I change isFound from volatile to static it works fine why is that?

class FileAccessThread implements Runnable {

    volatile  boolean isFound = false;
    BlockingQueue<String> bq;
        public FileAccessThread(BlockingQueue<String> bq) {
        this.bq = bq;
    }

    public void run() {
        System.out.println("ifFound "+isFound);
        if(bq !=null && !isFound){
            try {
                System.out.println("Current Thread "+Thread.currentThread().getName());
                String filePath = bq.take();
                System.out.println("File Path "+filePath);
                Thread.sleep(2000);

                if(filePath.equals("c:/test/t5.txt")) {
                    isFound = true;
                }

            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

public class MultiFileAccess {

    public static void main(String[] args) {
        ExecutorService e = Executors.newFixedThreadPool(3);
        BlockingQueue<String> bq = new LinkedBlockingQueue<>();
        bq.add("c:/test/t1.txt");
        bq.add("c:/test/t2.txt");
        bq.add("c:/test/t3.txt");
        bq.add("c:/test/t4.txt");
        bq.add("c:/test/t5.txt");
        bq.add("c:/test/t6.txt");
        bq.add("c:/test/t7.txt");
        bq.add("c:/test/t8.txt");
        bq.add("c:/test/t9.txt");
        bq.add("c:/test/t10.txt");

        int lengthOfQueue = bq.size();

        for(int i=0;i < lengthOfQueue-1;i++){
            e.execute(new FileAccessThread(bq));
        }

        e.shutdown();
    }
}
Kylar
  • 8,876
  • 8
  • 41
  • 75
Madhu CM
  • 2,296
  • 6
  • 29
  • 38
  • 4
    You have 10 different FileAccessThread object, each with its own isFound variable. Unless you make it static. You should probably read about the fundamentals of the language.... – assylias Apr 18 '17 at 15:08
  • Apart from the fact that this is very poor design, you should make it `static volatile` if you want the current design to *always* work. – Erwin Bolwidt Apr 18 '17 at 15:12
  • I understand that there will be only one copy of static variable for all the objects but my question is why volatile will not work in this case ? – Madhu CM Apr 18 '17 at 15:13
  • if it's static volatile, it will work. If it's just volatile, then each thread has it's own copy, so the other threads never see when it gets set. – Kylar Apr 18 '17 at 15:17
  • If I am not wrong Volatile is not cached , the value is updated on main memory so that other threads can see it. But according to you "thread has it's own copy, so the other threads never see when it gets set" how does this applies ? – Madhu CM Apr 18 '17 at 15:21
  • 1
    @MadhuCM In your code, there are 10 volatile variables. Let's call them v1, v2, ..., v10. When Thread 1 sets v1 to true and Thread 3 checks v3, it reads false, because they are not the same variable. Volatile has nothing to do with this. – assylias Apr 18 '17 at 15:33
  • A good explanation as to why one could need the volatile keyword: http://tutorials.jenkov.com/java-concurrency/volatile.html – Tim Apr 19 '17 at 07:58

2 Answers2

4

I think you have a misunderstanding of what static and volatile do.

volatile imposes a set of memory read/write semantics on the variable. In this case, it doesn't matter, because you've defined each thread with it's own variable, so each thread only reads and writes it's own, local copy of isFound.

In the case where you've defined it as static, then all of the threads are sharing the same variable - so if one thread changes it, then the other threads will (likely) see it. You don't have any synchronization around the variable though, so it's non-deterministic as to when or which threads will see the change.

For a more in-depth look at static vs volatile, look at the answer to this question: Volatile Vs Static in java

Community
  • 1
  • 1
Kylar
  • 8,876
  • 8
  • 41
  • 75
0

Just change how you instantiate your Runnable:

Runnable oneInstanceOnly = new FileAccessThread(bq);
for(int i=0;i < lengthOfQueue-1;i++){
    e.execute(oneInstanceOnly);
}

You created many instances, each having its own isFound field, unless created as a static.

Guillaume F.
  • 5,905
  • 2
  • 31
  • 59