Concurrent access of String variable in java

Question

If multiple threads are triggered does String variable (status) need to be synchronized?

class Request{
  String status;
  ....// Some other variables used in thread
}

class Test{

      public static void main(String[] args){
      Requesr r = new Request();
      List<Future> list= new ArrayList<Future>();
      ExecutorService pool= Executors.newFixedThreadPool(10);
       for(String input : inputList){
         if(!"failed."equals(r.status)){
           RequestHandler request = new RequestHandler(input,r);
           Future f = pool.submit(request);
           list.add(f);
         }else{
            //fail the job and return;
        }
      }
    for (Future fTemp : list) {
       if (fTemp.get() == null) {
          // Task completed
        }
     }
   }
 }

class RequestHandler extends Runnable{
         Map<String,String> input;
         Requesr r;
         RequestHandler(Map<String,String> input, Request r ){
           this.input=input;
           this.r = r;
         }
        @Override
         public void run() {
         if(!"failed".equals(r.status)){
           try{
             //some logic
           }catch(Exception e){
             r.Status = "failed";//status is assigned a value only here
           }
         }
       }
    }

Does status need to be synchronized for it to be visible in the Test class for loop and in other threads? As mentioned below in comments I will use Future objects and cancel the running threads.

My doubt is whether above code works without synchronization logic. If it doesn't how can we add synchronization logic in this case?

Please, show how you start your thread. Given the code above you might never see `status` value changed if you start your thread after the `for loop` or even before it with or without using synchronization. — tsolakp, Dec 26 '17 at 19:47
A variable is never synchronized. The code you posted wouldn't compile, so it's basically impossible to answer the question. Post code that compiles and runs. — JB Nizet, Dec 26 '17 at 19:47
Is there a good reason not to use an `ExecutorService` here, and just cancel the outstanding futures when you encounter an error? — Andy Turner, Dec 26 '17 at 20:03
related questions: https://stackoverflow.com/questions/4756536/what-operations-in-java-are-considered-atomic/4756578 https://stackoverflow.com/questions/5307003/java-multi-threading-atomic-reference-assignment, https://stackoverflow.com/questions/15196355/reference-assignment-is-atomic-so-why-use-atomicreference — tkruse, Dec 27 '17 at 02:06
This code doesn't make sense. Why would you expect the status to be updated before you've finished submitting the tasks? — shmosel, Dec 27 '17 at 03:10
It is a requirement. Exception is such that I need to fail the entire job. So i dont want upcoming threads to get triggered. — Teja, Dec 27 '17 at 03:11
It doesn't make sense to fail only in the few milliseconds it takes to loop through `inputList`. You probably misunderstood the requirement. — shmosel, Dec 27 '17 at 03:28
@shmosel Edited the code. As thread pool size is limited. Jobs in queue should not execute the entire code if status is updated to failed in other thread. — Teja, Dec 27 '17 at 03:39
Now it makes a bit more sense, assuming there can be more than 10 tasks. Still, there are more conventional ways of cancelling tasks. — shmosel, Dec 27 '17 at 03:40
Thanks @shmosel for the inputs. Do I need to add synchronization logic for status variable? — Teja, Dec 27 '17 at 03:43
I would say yes, since there's no *happens-before* guarantee (that I'm aware of) between Executor tasks. The simplest solution is to make `status` `volatile`, as suggested in the answer below. — shmosel, Dec 27 '17 at 03:55

tkruse · Accepted Answer · 2017-12-27T02:37:54.907

0

The variable should probably be declared volatile. Else it may happen that a thread updates the value to "failed", but the main thread never sees this update. The reasons are explained here: http://etutorials.org/Programming/Java+performance+tuning/Chapter+10.+Threading/10.6+Atomic+Access+and+Assignment/

It's possible (depending on what the triggering code does) that this is unnecessary, but it's not worth taking the risk.

edited Dec 27 '17 at 02:37

answered Dec 27 '17 at 01:58

tkruse

10,222
7
53
80

Read a little further: *Note that I'm talking about using synchronization for thread-safety here, not synchronization for visibility of updates and accesses of variables.* That's very possibly an issue here. – shmosel Dec 27 '17 at 02:18
Sure. If one of the threads sets `status = "failed"`, there's no guarantee the main thread will see the update without some form of synchronization. – shmosel Dec 27 '17 at 02:27
I will set status to volatile. Do I need synchronization for getter setter methods. volatile String status; public void setStatus(String status){ this.status = status } public String getStatus(){ return status; } – Teja Dec 27 '17 at 04:10
According to the linked article using volatile should be enough, no need to add synchronized anywhere. – tkruse Dec 27 '17 at 04:20

Concurrent access of String variable in java

1 Answers1