Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.java.programmer > #10739

Re: Question whether a problem with race conditions exists in this case

Show all headers | View raw

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 thought-
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 in 
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 will 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 name, 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 whatever 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 section.  All accesses are covered by the same lock (plus several other unnecessary 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 separate 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 programming.  That is not effective.  Don't do it.

*Think* about what you're doing.  What references have you checked to improve 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 you use for each block of writes (mutations).  You have the same actors in each 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 you should make it.  It should be quite straightforward to reason about how this code will work.

-- 
Lew

Back to comp.lang.java.programmer | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 09:07 -0800
  Re: Question whether a problem with race conditions exists in this case Andreas Leitgeb <avl@gamma.logic.tuwien.ac.at> - 2011-12-14 17:53 +0000
    Re: Question whether a problem with race conditions exists in this case Lew <lewbloch@gmail.com> - 2011-12-14 10:44 -0800
      Re: Question whether a problem with race conditions exists in this case Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2011-12-14 10:54 -0800
      Re: Question whether a problem with race conditions exists in this case Andreas Leitgeb <avl@gamma.logic.tuwien.ac.at> - 2011-12-14 21:15 +0000
        Re: Question whether a problem with race conditions exists in this case Tom Anderson <twic@urchin.earth.li> - 2011-12-15 14:58 +0000
          Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-15 08:40 -0800
          Re: Question whether a problem with race conditions exists in this case Gene Wirchenko <genew@ocis.net> - 2011-12-15 11:55 -0800
            Re: Question whether a problem with race conditions exists in this case Tom Anderson <twic@urchin.earth.li> - 2011-12-16 14:06 +0000
              Re: Question whether a problem with race conditions exists in this case Gene Wirchenko <genew@ocis.net> - 2011-12-16 10:09 -0800
    Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 11:47 -0800
  Re: Question whether a problem with race conditions exists in this case Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2011-12-14 10:53 -0800
    Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 11:54 -0800
      Re: Question whether a problem with race conditions exists in this case Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2011-12-15 16:38 -0800
        Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-15 23:01 -0800
          Re: Question whether a problem with race conditions exists in this case Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2011-12-16 09:34 -0800
            Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-17 11:55 -0800
              Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-19 05:33 -0800
  Re: Question whether a problem with race conditions exists in this case Lew <lewbloch@gmail.com> - 2011-12-14 11:04 -0800
    Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 12:32 -0800
      Re: Question whether a problem with race conditions exists in this case markspace <-@.> - 2011-12-14 14:13 -0800
        Re: Question whether a problem with race conditions exists in this case Eric Sosman <esosman@ieee-dot-org.invalid> - 2011-12-14 17:44 -0500
          Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 14:50 -0800
            Re: Question whether a problem with race conditions exists in this case markspace <-@.> - 2011-12-14 15:26 -0800
      Re: Question whether a problem with race conditions exists in this case Lew <lewbloch@gmail.com> - 2011-12-15 01:34 -0800
    Re: Question whether a problem with race conditions exists in this case Andreas Leitgeb <avl@gamma.logic.tuwien.ac.at> - 2011-12-14 21:38 +0000
  Re: Question whether a problem with race conditions exists in this case Eric Sosman <esosman@ieee-dot-org.invalid> - 2011-12-14 16:26 -0500
    Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 13:57 -0800
      Re: Question whether a problem with race conditions exists in this case Eric Sosman <esosman@ieee-dot-org.invalid> - 2011-12-14 18:05 -0500
        Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 23:25 -0800
          Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 23:28 -0800
      Re: Question whether a problem with race conditions exists in this case Tom Anderson <twic@urchin.earth.li> - 2011-12-15 14:44 +0000
        Re: Question whether a problem with race conditions exists in this case Lew <lewbloch@gmail.com> - 2011-12-16 08:27 -0800
          Re: Question whether a problem with race conditions exists in this case Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2011-12-16 09:41 -0800
            Re: Question whether a problem with race conditions exists in this case Tom Anderson <twic@urchin.earth.li> - 2011-12-16 18:51 +0000
          Re: Question whether a problem with race conditions exists in this case Tom Anderson <twic@urchin.earth.li> - 2011-12-16 18:50 +0000
    Re: Question whether a problem with race conditions exists in this case Saxo <saxo123@gmx.de> - 2011-12-14 14:13 -0800
      Re: Question whether a problem with race conditions exists in this case Lew <lewbloch@gmail.com> - 2011-12-14 16:55 -0800

csiph-web