CODEWITHSUNDEEP
javamultithreading
Ryanqy
Ryanqy

Reputation: 9506

This Stack class has some multithread problems?

I have a Stack class, like this

public class Stack {

    LinkedList list = new LinkedList();

    public synchronized void push(Object x) {
        synchronized (list) {
            list.addLast(x);
            notify();
        }
    }

    public synchronized Object pop () throws Exception {
        synchronized (list) {
            if (list.size() <= 0) {
                wait();
            }
            return list.removeLast();
        }
    }
}

When multiple thread access The Stack class, Has some multithread problems cause Stack crash?

Upvotes: 1

Views: 65

Answers (1)

Gray
Gray

Reputation: 116878

Has some multithread problems cause Stack crash?

In the future you should always provide the exception details so we can help better or at least define what "crash" means and provide log output. In this case I suspect you are getting a NoSuchElementException.

Your code is missing a small but critical change. 

synchronized (list) {
    // here's your problem, if should be while
    if (list.size() <= 0) {
        wait();
    }
    return list.removeLast();
}

The if clause here should be a while loop. As @Ivan points out, this is important if you architecture suffers from spurious wakeups, however this also fixes a much more likely race condition.

If you have multiple threads consuming from your stack, you could have 2 threads waiting to pop() with thread-A at the start of the synchronized block and thread-B at the wait(). When another thread does a push() and a notify() then the waiting thread-B will be moved from the wait queue to the blocked queue but it will be behind thread-A which was already blocked. When the lock is released by the thread that did the push, thread-A gets the lock first goes forward and gets the object from the list and unlocks. Then thread-B goes forward and calls removeLast() but there is no items in the stack and it throws NoSuchElementException.

// we use while here to protect against the race condition
while (list.size() <= 0) {
    wait();
}
return list.removeLast();

By using while, once thread-A gets the lock, it can re-check to make sure another thread hasn't "stolen" the item from the stack that it was notified about. For more details about this race condition see my old page on this topic here.

Couple other comments:

  • You do not need both a synchronized method and a synchronized block on the list. I'd recommend just removing the synchronized keyword on the methods unless there is other code you aren't showing us and just sync on the list. You then should do list.wait().
  • The list field should be private and final which are both recommended in threaded programs if you are synchronized on it.

Upvotes: 1

Related Questions