skipping synchronization on assuption of JAVA atomic primitive assignment.

[Ramya Chowdary]

I wanted following code in thread safe manner. So, In order to optimize I synchronized part of the method
Instead of entire method. Because part of the code occurs for every 10 mins ( intial un-synchronized will be called every-time)

Part of the method which is synchronized gets database updated value, and assigns it to member variable enabled.
This part of synchronizing code occurs for every 10 mins .

I didn't see any harm in doing this way unless JAVA primitive assignment is atomic.

Is java primitive assignment is atomic.
Also, please comment on my speculation on not making entire method synchronization.

Following is the code,

class ABC{
   
    private boolean enabled = false;    

    private boolean getEnabled(){
       If( LASTDBACCESSTIME < 10 MINS){
          return enabled; 
       }
       synchronized(this){
            //Get updated enabled value from database
           //update class level variable enabled with this value
       }
       return enabled;
    }
}

[Henry Wong]

Boolean variable assignments are atomic.... but... non-volatile variables may be cached, hence, it is possible for threads to see previous values. You should declare the variable volatile.

Henry

[Ramya Chowdary]

Thanks for taking time in replying me.

So, If I make it as volatile. Will the above method would be thread safe?

It also seems we need to take consideration boolean variable access atomicity.
(writing happening when reading in progress)
Also please let me know If you see any other synchronization issues

once again,

class ABC{  
      
     private volatile boolean enabled = false;      
   
     private boolean getEnabled(){  
        If( LASTDBACCESSTIME < 10 MINS){  
           return enabled;   
        }  
        synchronized(this){  
             //Get updated enabled value from database  
            //update class level variable enabled with this value  
        }  
        return enabled;  
     }  
 }

instead of

class ABC{  
      
     private boolean enabled = false;      
   
     private synchronized boolean getEnabled(){  
        If( LASTDBACCESSTIME < 10 MINS){  
           return enabled;   
        }  
        //Get updated enabled value from database  
        //update class level variable enabled with this value  
        return enabled;  
     }  
 }

[Mike Simmons]

Of those two code examples, I think you're much better off with the second one, synchronizing the whole getEnabled() method. Imagine what happens if two threads call getEnabled() just past 10 minutes since the last DB refresh.

With the first code (using synchronized(this)): Thread A will check the time, see it's past 10 minutes, enter the synchronized(this) block, and do a DB update. Thread B will check the time, try to enter the synch block, block until thread A completes its update, and then... thread B will do another DB update, even though thread A just completed such an update.

With the second code (using a synchronized method): Thread A will enter the sync method, check the time, see it's past 10 minutes, and do a DB update. Thread B will try to enter the sync method, block until thread A completes its work, then enter, check the time, and skip the DB update because it can easily use the value of enabled set by thread A much less than 10 minutes ago.

The DB update is by far the slowest thing in all this code, and repeating it is silly. Thus, I recommend the second code over the first, definitely.

However, a better solution might be to just create a ScheduledExecutorService (or a java.util.Timer) to, in a separate thread from everything else, update the enabled field every 10 minutes. And make enabled volatile. That way the other threads will never have to wait for a DB update. It will just be done automatically every 10 minutes. A thread that checks enabled just before a refresh will see the old value, and one that checks after that will see the new value.

[Ireneusz Kordal]

Mike Simmons wrote:Imagine what happens if two threads call getEnabled() just past 10 minutes since the last DB refresh.

The simple solution for this could be an additional check in the synchronized block:

class ABC{  
      
     private volatile boolean enabled = false;      
   
     private boolean getEnabled(){  
        If( LASTDBACCESSTIME < 10 MINS){  
           return enabled;   
        }  
        synchronized(this){  
              If( LASTDBACCESSTIME < 10 MINS){  
                   return enabled;   
              }  
             //Get updated enabled value from database  
             //update class level variable enabled with this value  
             // update LASTDBACCESSTIME
        }  
        return enabled;  
     }  
 }

Some remarks: LASTDBACCESSTIME must be volatile too (if you implemented it as variable)