2

I had a question for my exam in which I didn't receive full points due to something I don't quite understand.

The question is the following: Given the following program, apply changes that do not allow sum to be printed as a negative number.

public class Summer {

    private int sum;

    public void up() {
       sum++;
       System.out.println(sum);
    }

    public void down() {
       sum--;
       System.out.println(sum);
    }
}

the changes I made were the following:

public class Summer {

    private volatile int sum;

    public synchronized void up() {
       sum++;
       System.out.println(sum);
    }

    public synchronized void down() {
       while (sum <= 0) {
           try {
                Thread.sleep(100);
           } catch (InterruptedException e) { } 
       }
       sum--;
       System.out.println(sum);
    }
}

The answer I got is that I cannot use sleep in this program, and I have to use wait and I must use function notifyAll to awake the Thread.

My question is, why is what I wrote wrong? shouldn't volatile not allow sum to get cached and therefore I always get the updated version of sum since there is no possible way I get a "dirty copy" and therefore there is no way to print a negative sum?

Raedwald
  • 46,613
  • 43
  • 151
  • 237
Rab
  • 147
  • 4
  • 1
    The problem is supposed to be a typical *producer-consumer* problem. BTW when you use *synchronized*, you dont need `volatile` – TheLostMind Feb 21 '16 at 10:58
  • 1
    Catching an exception but doing nothing with it is a code smell: there are very few places where this is the right thing to do. – Raedwald Feb 21 '16 at 12:37

2 Answers2

2

Sleep method puts thead on sleep method for a specific time while wait method holds threads execution untill someone awakes it. Hence your while loop will put your thread on endless loop. So use wait() instead of sleep(...) in down() and notify or notifyAll() in up method to awake down method when it increments and yes indees volatile variable gives each thread a fresh value always.

Harshad Sindhav
  • 171
  • 2
  • 7
1

Your methods are synchronized. If down() is called and sum is <= 0, it will loop infinitely. Since synchronized methods synchronzie on this, no other thread can enter up() to release down() from its loop.

As for a solution, I think this should solve the problem:

public class Summer {

    private volatile int sum;

    public synchronized void up() {
        sum++;
        System.out.println(sum);
        this.notify(); // notify waiting threads that sum has changed
    }

    public synchronized void down() {
        while (sum <= 0) {
            try {
                this.wait(); // wait while sum <= 0. It is not sufficient to
                             // receive a notification and proceed since
                             // multiple threads may call down(). Also, the
                             // thread may wake up due to an interrupt, so
                             // it is advised putting wait() in a loop.
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        sum--;
        System.out.println(sum);
    }
}
Turing85
  • 18,217
  • 7
  • 33
  • 58
  • Should use a notifyAll. – matt Feb 21 '16 at 11:46
  • @matt could you elaborate why? Each call to `up()` results in a call to `notify()` and since there must be at least as many `up()`'s as `down()`'s, it should be sufficient to call only `notify()`. – Turing85 Feb 21 '16 at 11:49
  • In general, notify is considered to be broken, and notifyAll should be used. I don't have a resource for that. – matt Feb 21 '16 at 11:52
  • This example http://stackoverflow.com/a/3186336/2067492 shows a deadlock from a similar mechanism. The rule is more of a, unless you actually need a notify, use notifyAll. Also, wait can get called multiple times for each down. – matt Feb 21 '16 at 13:19
  • @matt that is a different scenario, since both methods need to wait. Here, only one method waits. And calling `notify()` broken seems wrong to me since it does exaclty what it is supposed to do. In the given scenario, it may not harm to call `notifyAll(). – Turing85 Feb 21 '16 at 13:56
  • Sure, OP also said he was instructed to use notifyAll(). You left out an important detail about why your answer addresses the issue, and the ops would go into a deadlock. Since sleep doesn't release the lock, but continues holding the lock on the synchronized object, unlike wait. – matt Feb 21 '16 at 14:41