How to correctly create a SynchronizedStack class?

I made a simple synchronized Stack object in Java, just for training purposes. Here is what I did:

public class SynchronizedStack {
    private ArrayDeque<Integer> stack;

    public SynchronizedStack(){
        this.stack = new ArrayDeque<Integer>();     
    }

    public synchronized Integer pop(){
        return this.stack.pop();
    }

    public synchronized int forcePop(){
        while(isEmpty()){
            System.out.printl开发者_高级运维n("    Stack is empty");
            try {
                wait();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        return this.stack.pop();
    }

    public synchronized void push(int i){
        this.stack.push(i);
        notifyAll();
    }

    public boolean isEmpty(){
        return this.stack.isEmpty();
    }

    public synchronized void pushAll(int[] d){
        for(int i = 0; i < d.length; i++){
            this.stack.push(i);
        }
        notifyAll();
    }

    public synchronized String toString(){
        String s = "[";
        Iterator<Integer> it = this.stack.iterator();   
        while(it.hasNext()){
            s += it.next() + ", ";
        }
        s += "]";
        return s;
    }
}

Here are my questions:

• Is it OK not to synchronize the isEmtpy() method? I figured it was because even if another Thread is modifying the stack at the same time, it would still return a coherent result (there is no operation that goes into a isEmpty state that is neither initial or final). Or is it a better design to have all the methods of a synchronized object synchronized?

• I don't like the forcePop() method. I just wanted to create a thread that was able to wait until an item was pushed into the stack before poping an element, and I thought the best option was to do the loop with the wait() in the run() method of the thread, but I can't because it throws an IllegalMonitorStatException. What is the proper method to do something like this?

• Any other comment/suggestion?

Thank you!

• Stack itself is already synchronized, so it doesn't make sense to apply synchronization again (use ArrayDeque if you want a non-synchronized stack implementation)

• It's NOT OK (aside from the fact from the previous point), because lack of synchronization may cause memory visibility effects.

• forcePop() is pretty good. Though it should pass InterruptedException without catching it to follow the contract of interruptable blocking method. It would allow you to interrupt a thread blocked at forcePop() call by calling Thread.interrupt().

Assuming that stack.isEmpty() won't need synchronization might be true, but you are relying on an implementation detail of a class that you have no control over. The javadocs of Stack state that the class is not thread-safe, so you should synchronize all access.

I think you're mixing idioms a little. You are backing your SynchronizedStack with java.util.Stack, which in turn is backed by java.util.Vector, which is synchronized. I think you should encapsulate the wait() and notify() behaivor in another class.

The only problem with not synchronizing isEmpty() is that you don't know what's happening underneath. While your reasoning is, well, reasonable, it assumes that the underlying Stack is also behaving in a reasonable manner. Which it probably is in this case, but you can't rely on it in general.

And the second part of your question, there's nothing wrong with a blocking pop operation, see this for a complete implementation of all the possible strategies.

And one other suggestion: if you're creating a class that is likely to be re-used in several parts of an application (or even several applications), don't use synchronized methods. Do this instead:

public class Whatever {
  private Object lock = new Object();

  public void doSomething() {
    synchronized( lock ) {
      ...
    }
  }
}

The reason for this is that you don't really know if users of your class want to synchronize on your Whatever instances or not. If they do, they might interfere with the operation of the class itself. This way you've got your very own private lock which nobody can interfere with.