Re: Question whether a problem with race conditions exists in this
case
On 12/14/11 9:07 AM, Saxo wrote:
I have a class Node as displayed below that holds a new value and the
previous value.
[snip]
public class Node {
AtomicBoolean useNewValue = new AtomicBoolean(false);
private Object newValue = null;
private Object previousValue = null;
private Object lock = new Object();
public Object get() {
synchronized(lock) {
if(useNewValue.get()) // 1
return newValue; // 2
return previousValue; // 3
}
}
public void setNewValue(Object newValue, AtomicBoolean
useNewValueParam) {
synchronized(lock) {
if(this.useNewValue.get())
previousValue = this.newValue;
this.newValue = newValue;
// useNewValueParam is allways set to false when setNewValue is
called
If it is always false, why pass it in at all? I don't believe this comment.
this.useNewValue = useNewValueParam;
}
}
}
Don't use tabs on usenet, it makes formatting bonkers. Use 2 to 4
spaces for indenting.
Since you have a lock here, you don't need to use an AtomicBoolean.
Even so, you're not using the AtomicBoolean as expected. The field for
it should be final, and you call "set(value)" on it, rather than replace
it with another AtomicBoolean instance. That defeats the purpose.
At the same time there is never more than one thread iterating over
the node list calling setNewValue. This is made sure through
serialization of threads that want to iterate over the list calling
setNewValue. Serialization of threads is a bit crude, but not relevant
at the moment for the discussion of the problem described here.
The threads are all serialized because of the synchronize(lock).
My question is now whether this approach is free of race conditions or
starvation issues if implemented as described above. I have some
doubts whether everything is fine here as useNewValue is changed by
the commit thread by reference without entering synchronized(lock)
{ ... }. So is everything still fine if a context switch happens
between line 1 and 2 or between line 1 and 3? It would be for sure if
the commit thread entered the synchronized(lock) { ... } block, but it
does not (changing to the new values all at once wouldn't be possible
otherwise).
synchronized(lock) prevents more than one thread from being inside *any*
synchronized(lock) block at *any* given time, so you will never have a
setter and getter running at the same time. This code will "work", but
is still not great quality.
I would be thankful if someone could look over this and tell me her/
his opinion.
Unrelated, it might be useful to use Generics here. I don't know much
of your use-case, but I'll demonstrate how it might be used. There are
actually two ways I would suggest doing this, depending on the frequency
of sets vs gets. If they are about equal, or sets are more frequent, I
suggest this class:
public class Node<V> {
private boolean useNew;
private V previousValue;
private V value;
private final Object lock = new Object();
public V getValue() {
synchronize(lock) {
return useNew ? value : previousValue;
}
}
public void setValue(V value, boolean useNew) {
synchronize(lock) {
previousValue = getValue();
this.value = value;
this.useNew = useNew;
}
}
}
If the sets are somewhat infrequent, I suggest the following class. It
will allow much faster access by multiple threads for getValue(). It
does have a small amount of overhead when setting the value.
public class Node<V> {
private volatile State<V> state = new State<V>(null, null, true);
private final Object setLock = new Object();
public V getValue() {
// volatile allows us to safely read a value.
return state.getValue();
}
public V setValue(V newValue, boolean useNew) {
// we need to serialize access here, otherwise we might get the
// wrong previous value.
synchronized(setLock) {
state = new State<V>(state.value, newValue, useNew);
}
}
// Note, this <V> is independent of the <V> in Node.
private static class State<V> {
private final V previousValue;
private final V newValue;
private volatile boolean V useNew;
public State(V previousValue, V newValue, boolean useNew) {
this.previousValue = previousValue;
this.newValue = newValue;
this.useNew = useNew;
}
public V getValue() {
return useNew ? newValue : previousValue;
}
}
}