Reputation: 41
I have read answers to similar questions about the correct ways to use synchronized. However, they dont seem to explain why this issue occurred. Even though I added synchronized to my getValue and setValue method, i still get outputs like the following. Why does this happen?
output:
making set
Doing get
making set
Doing making set
get
Doing get
making set
making Doing get
set
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();
}
}
Upvotes: 4
Views: 165
Reputation: 183504
The problem is that your no-arg constructor calls setValue(...)
on the newly-created instance:
public StackNode() {
setValue(null);
setNext(null);
}
and your Runnable setValue
constructs a new instance of StackNode
, to pass to node.setNext(...)
:
node.setNext(new StackNode());
(even though your test never actually uses node.next
, so this is essentially a no-op aside from the output it produces). Since your synchronized
methods are instance methods (not static
methods), they have separate locks, which means that the call to setValue(...)
in the new instances' constructors is not synchronized with respect to the calls you make on node
.
Note that, although your specific problem is rather unusual (you have a getter and setter that are manipulating shared external state, namely System.out
, but do not have any corresponding shared locks to prevent interference), it is actually always a bad idea to call a method from a constructor, unless the method is private
or final
or static
or the class is final
, because a superclass constructor is called before a subclass instance is fully created, so if the constructor calls a method that's overridden in a subclass, the subclass method will receive an incomplete this
object and might misbehave terribly. You're better off changing your constructor to this:
public StackNode() {
value = null;
next = null;
}
(or just remove the assignment statements altogether, since fields of reference type are automatically initialized to null
anyway).
Upvotes: 2
Reputation: 1484
This should fix the issue.
public Object getValue() {
synchronized(System.out){
System.out.print(" Doing ");
System.out.println(" get ");
System.out.flush();
return value;
}
}
Upvotes: 4