Re: Question whether a problem with race conditions exists in this case
Saxo wrote:
I have a class Node as displayed below that holds a new value and the
First, good job with your SSCCE (http://sscce.org/). That was a well thoug=
ht-
out and complete job.
Second, _never_ use TAB characters in code posts except in literals. To
indent, use two or four spaces per indent level. Two tends to fit better i=
n
forum posts.
previous value. Which one applies is determined by the value of
useNewValue. In my application there is a list of such nodes where for
each node the current value is replaced with a new one. The new values
should become effective for the entire list all >at once< through an
atomic change of useNewValue in every node. For that purpose, when the
new value is set, for every node in the list the same AtomicBoolean
instance is passed on to setNewValue(...), which is stored in
useNewValueParam. When the commit is done, the value of this instance
of AtomicBoolean is changed to true (this way the thread doing the
commit does not have to enter the synchronized block as all the other
threads calling aNode.get) and thus the new value of every node
becomes visible at once to every thread calling aNode.get().
public class Node {
AtomicBoolean useNewValue = new AtomicBoolean(false);
A 'volatile boolean' should do here. Actually, a non-'volatile' variable w=
ill be just fine:
boolean useNewValue;
No need to set to 'false' what is already set to 'false'.
Why did you declare it package-private?
private Object newValue = null;
Why do you redundantly set this to 'null'?
private Object previousValue = null;
private Object lock = new Object();
Why not use the instance itself as the lock?
public Object get() {
synchronized(lock) {
if(useNewValue.get()) // 1
return newValue; // 2
return previousValue; // 3
}
}
public void setNewValue(Object newValue, AtomicBoolean
useNewValueParam) {
It is ridiculous to pass an 'AtomicBoolean'. You just need a 'boolean'.
synchronized(lock) {
if(this.useNewValue.get())
previousValue = this.newValue;
this.newValue = newValue;
// useNewValueParam is allways set to false when setNewValue is
called
That's patently untrue.
What the heck do you mean by this comment? I cannot make sense of it.
this.useNewValue = useNewValueParam;
You write a very confusing logic here. Is 'useNewValueParam' (terrible nam=
e, BTW) meant to affect the next time the accessor is called?
If so, you've pretty much thrown all your concurrency out the window. Any =
number of 'set' calls could occur, each flipping your boolean the other way=
, and you'll never know whether it's going to land butter side up.
If all you care about is that each reader see the correct current state wha=
tever it may be after whatever number of setters, then fine.
Regardless, you need to document the heck out of what you intend to do with=
this parameter. What the heck are you even trying to accomplish?
}
}
}
My question is now whether this approach is free of race conditions or
starvation issues if implemented as described above. I have some
You over-implemented it by a mile.
Use just one lock.
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).
You have synchronized those lines. They are all in the same critical secti=
on. All accesses are covered by the same lock (plus several other unnecess=
ary ones). You're good.
Except for how you coded it, that is.
Use the instance itself as the monitor, unless you really, really need a se=
parate one. It would be kind to maintainers to comment your implementation=
choice, and that in turn forces you to have a darned good reason.
Throwing a crapload of locks at a problem willy-nilly is superstitious prog=
ramming. That is not effective. Don't do it.
*Think* about what you're doing. What references have you checked to impro=
ve your ability to reason about concurrent programming?
Seriously, which ones have you read?
I really am asking.
Brian Goetz's _Java Concurrency in Practice_ is an excellent resource.
You synchronize each block of reads (accesses) with the same monitor that y=
ou use for each block of writes (mutations). You have the same actors in e=
ach block, no more, no less. In your example, you have three. And instead=
of 'Object', consider using generics.
public class Node<V> // off the top of my head, not even compiled
{
private V value;
private V previous;
private boolean useCurrent;
synchronized public V getValue()
{
return useCurrent? value : previous;
}
synchronized public void setValue(V value, boolean useCurrent)
{
previous = this.value;
this.value = value;
this.useCurrent = useCurrent;
}
}
This code will not behave the same as yours, but I presumptuously concluded=
that your code did the wrong thing. Either way, you can see how simple yo=
u should make it. It should be quite straightforward to reason about how t=
his code will work.
--
Lew