Path: csiph.com!x330-a1.tempe.blueboxinc.net!newsfeed.hal-mli.net!feeder3.hal-mli.net!newsfeed.hal-mli.net!feeder1.hal-mli.net!npeer02.iad.highwinds-media.com!news.highwinds-media.com!feed-me.highwinds-media.com!postnews.google.com!glegroupsg2000goo.googlegroups.com!not-for-mail From: Lew Newsgroups: comp.lang.java.programmer Subject: Re: Question whether a problem with race conditions exists in this case Date: Wed, 14 Dec 2011 11:04:52 -0800 (PST) Organization: http://groups.google.com Lines: 159 Message-ID: <31419376.376.1323889493017.JavaMail.geo-discussion-forums@prmw6> References: <8bbbbee3-adcc-4f28-aff5-2e230b047401@u6g2000vbg.googlegroups.com> Reply-To: comp.lang.java.programmer@googlegroups.com NNTP-Posting-Host: 173.164.137.214 Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-Trace: posting.google.com 1323889608 32218 127.0.0.1 (14 Dec 2011 19:06:48 GMT) X-Complaints-To: groups-abuse@google.com NNTP-Posting-Date: Wed, 14 Dec 2011 19:06:48 +0000 (UTC) In-Reply-To: <8bbbbee3-adcc-4f28-aff5-2e230b047401@u6g2000vbg.googlegroups.com> Complaints-To: groups-abuse@google.com Injection-Info: glegroupsg2000goo.googlegroups.com; posting-host=173.164.137.214; posting-account=CP-lKQoAAAAGtB5diOuGlDQk0jIwmH0T User-Agent: G2/1.0 X-Google-Web-Client: true Xref: x330-a1.tempe.blueboxinc.net comp.lang.java.programmer:10739 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. =20 Second, _never_ use TAB characters in code posts except in literals. To=20 indent, use two or four spaces per indent level. Two tends to fit better i= n=20 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(). >=20 > public class Node { >=20 > AtomicBoolean useNewValue =3D 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 =3D null; Why do you redundantly set this to 'null'? > private Object previousValue =3D null; > private Object lock =3D 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 > } > } >=20 > 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 =3D this.newValue; > this.newValue =3D 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 =3D 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? > } > } > } >=20 > 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 // off the top of my head, not even compiled { private V value; private V previous; =20 private boolean useCurrent; synchronized public V getValue() { return useCurrent? value : previous; } synchronized public void setValue(V value, boolean useCurrent) { previous =3D this.value; this.value =3D value; this.useCurrent =3D 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. --=20 Lew