what's the side effect of mutator not synchonized?

[David Spades]

@PrettySureThisIsThreadSafe
public class Objectify {

	private List<MyObject> objects = new ArrayList<MyObject>();
	
	void add(MyObject obj){

		synchronized (objects) {

			if(objects.size() < 4) objects.add(obj);
			else System.out.println("Denied");
			
		}
		
	}
	
	void throwOut(MyObject obj){
	
		objects.remove(obj);
	
	}
	
}

what's the side effect of this code? I intentionally didn't mark throwOut as synchronized and I'm pretty sure that I read somewhere that this is discouraged, but I couldn't think of any side effect. Can anybody give an example where this code would lead to problem?
thanks

[Tim Cooke]

David Spades wrote:what's the side effect of this code?

Do you have any thoughts on it? Let's hear them first.

[Henry Wong]

Also, what was the reason that you synchronized the add() method? Does that same reason apply to the remove method?

Henry

[David Spades]

    @PrettySureThisIsThreadSafe  
    public class Objectify {  
      
        private List<MyObject> objects = new ArrayList<MyObject>();  
          
        void add(MyObject obj){  
      
            synchronized (objects) {  
      
                if(objects.size() < 4) objects.add(obj);  
                else System.out.println("Denied");  
                  
            }  
              
        }  
          
        int getSize(){  
          
            return objects.size(); 
          
        }  
          
    }  

amended the code, sorry, it's not remove. It should've been size() method. IMO, getSize() doesn;t need synchronizing. lets say another thread is in the middle of adding a new element when getSize is called, so the new element is not yet included in the count, which is valid since the adding is not yet finished, right?
this is my reasoning. is there any fault in this?
thanks

[Henry Wong]

David Spades wrote: IMO, getSize() doesn;t need synchronizing. lets say another thread is in the middle of adding a new element when getSize is called, so the new element is not yet included in the count, which is valid since the adding is not yet finished, right?
this is my reasoning. is there any fault in this?

Can you show us where in the JavaDoc that the implementation is to be as you described? And to follow that, show us that the implementation is true for all versions, and future versions, of the JVMs?

Your reasoning, while sound, is still an assumption of the implementation. You need to code based on specification/documentation, and not based on a particular implementation (although admittedly, you can only test via implementation).

Henry

[Chris Hurst]

David Spades wrote: IMO, getSize() doesn;t need synchronizing. lets say another thread is in the middle of adding a new element when getSize is called, so the new element is not yet included in the count, which is valid since the adding is not yet finished, right?
this is my reasoning. is there any fault in this?

getSize needs to be synchronized. If one thread does all the adds and another was created at the same time as the first but never synchronizes on a common object with the mutating thread, getSize could in theory always return 0 for the second thread always/for ever i.e. empty collection regardless of any modifications by other threads because it never looks for change, the answer was zero the last time so surely its still zero as this thread hasn't modified it. By not not saying getSize is synchronized you are effectively allowing the reading thread to ignore all other threads changes if it decides its more efficient to do so i.e. your saying you don't care but you obviously do. In reality your getSize will most likely be observed to work (ish) but why take the risk ??

[Anupam Sinha]

Chris Hurst wrote:

David Spades wrote: IMO, getSize() doesn;t need synchronizing. lets say another thread is in the middle of adding a new element when getSize is called, so the new element is not yet included in the count, which is valid since the adding is not yet finished, right?
this is my reasoning. is there any fault in this?

getSize needs to be synchronized. If one thread does all the adds and another was created at the same time as the first but never synchronizes on a common object with the mutating thread, getSize could in theory always return 0 for the second thread always/for ever i.e. empty collection regardless of any modifications by other threads because it never looks for change, the answer was zero the last time so surely its still zero as this thread hasn't modified it. By not not saying getSize is synchronized you are effectively allowing the reading thread to ignore all other threads changes if it decides its more efficient to do so i.e. your saying you don't care but you obviously do. In reality your getSize will most likely be observed to work (ish) but why take the risk ??

Can getSize always return 0 for the current code? Doesn't synchronize also provides a happens before guarantee?

[Chris Hurst]

Anupam Sinha wrote:Can getSize always return 0 for the current code? Doesn't synchronize also provides a happens before guarantee?

If two threads synchronize on the same object you have your guarantee, so if two threads both 'add' it works but if another thread calls just getSize looking for objects to work with say it potentially doesn't :-( ie another thread could just call getSize and never sync at all.
In some edge cases the synchronized key word can be optimized away by Java i.e. if it can tell no other thread will ever contend, it was a common misconception that adding synchonized guarantees a memory barrier.

The add method has a read/write barrier but getSize has none at all so you have to work out the last sequence point for the two threads. The thread calling getSize may have effectively cached the size of the collection at zero in theory forever however changing getSize to synchronized prohibits any compiler optimization, caching etc etc i.e. its a volatile read the add method being a volatile write on its own may not be enough.