How to choose which object to block in Java?

Question

I read answers to similar questions about the right ways to use synchronized. However, they do not seem to explain why this problem arose. Despite the fact that I added synchronization to the getValue and setValue methods, I still get the output, as shown below. Why is this happening?

exit:

set creation

Performance

set creation

Dialing

receive

Performance

set creation

creating doing get

set

The code:

package src; public class StackNode { private Object value; private StackNode next; private final Object lock = new Object(); public StackNode() { setValue(null); setNext(null); } public StackNode(Object o) { value = 0; next = null; } public StackNode(StackNode node) { value = node.getValue(); next = node.getNext(); } public synchronized Object getValue() { System.out.print(" Doing "); System.out.println(" get "); System.out.flush(); return value; } public synchronized void setValue(Object value) { System.out.print(" making "); System.out.println(" set "); System.out.flush(); this.value = value; } public synchronized StackNode getNext() { return next; } public synchronized void setNext(StackNode next) { this.next = next; } } 

Test:

 public class TestStackNode { private final static StackNode node = new StackNode(); @Test public void getSetValueTest() throws InterruptedException{ node.setValue("bad"); Runnable setValue = new Runnable(){ @Override public void run() { node.setNext(new StackNode()); node.setValue("new"); } }; Runnable getValue = new Runnable(){ @Override public void run() { Assert.assertEquals("new", node.getValue()); } }; List<Thread> set = new ArrayList<Thread> (); List<Thread> get = new ArrayList<Thread> (); for (int i = 0; i < 30000; i++){ set.add( new Thread(setValue)); get.add(new Thread(getValue)); } for (int i = 0; i < 30000; i++){ set.get(i).start(); get.get(i).start(); } for (int i = 0; i < 30000; i++){ set.get(i).join(); get.get(i).join(); } } 

Answer 1

This should fix the problem.

 public Object getValue() { synchronized(System.out){ System.out.print(" Doing "); System.out.println(" get "); System.out.flush(); return value; } } 

Answer 2

The problem is that your no-arg constructor calls setValue(...) for the newly created instance:

 public StackNode() { setValue(null); setNext(null); } 

and your Runnable setValue creates a new StackNode instance to go to node.setNext(...) :

  node.setNext(new StackNode()); 

(even if your test never uses node.next , so this is essentially non-op aside from the result they output). Because synchronized methods are instance methods (not static ), they have separate locks, which means that the call to setValue(...) in the new instance constructors is not synchronized with the calls you make to node .

Please note that although your specific problem is rather unusual (you have a getter and setter that control the overall external state, namely System.out , but do not have corresponding common locks to prevent interference), it is actually always a bad idea to call a method from the designer, unless the method is not private or final or static , or class final , because the superclass constructor is called before the subclass instance will be fully established, therefore, if the constructor is a method that is overridden in conn Behold, the subclass method will receive an incomplete object this and can be terribly mistaken. You better change your constructor to this:

 public StackNode() { value = null; next = null; } 

(or just remove the assignment instructions altogether, since fields of the reference type are automatically initialized to null ).