3
\$\begingroup\$

Here is a class which I have written to create a simple Stack data structure which can be shared across multiple threads.

It's a simple LIFO which has 2 operations: one to push onto the top of the stack and the other one to pop from the top of the stack.

Please help me to find any issues with it.

package com.thread.test;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public class ThreadSafeStack<T> {

    private List<T> list;
    private ReentrantReadWriteLock lock;

    public ThreadSafeStack() {
        this.list = new ArrayList<T>();
        lock = new ReentrantReadWriteLock();
    }

    public T pop() {
        try {
            lock.writeLock().lock();
            if(!list.isEmpty()) {
                return list.remove(list.size()-1);
            }
        } finally {
            System.out.println("In finally");
            lock.writeLock().unlock();
        }
        return null;
    }

    public boolean push(T t) {
        try {
            lock.writeLock().lock();
            return list.add(t);

        }
        finally {
            lock.writeLock().unlock();
        }
    }

}
\$\endgroup\$
  • \$\begingroup\$ I'm unfamiliar with thread safety in Java, but from a little research, should your data structure implementation make use of the synchronized keyword? I may be wrong if that is handled by java.util.concurrent.locks.ReentrantReadWriteLock. However, I do not see anything on ReentrantReadWriteLock's documentation, it only mentions it is reentrant, not thread-safe. \$\endgroup\$ – esote Jun 28 '18 at 3:50
  • \$\begingroup\$ What's wrong with a ConcurrentLinkedQueue? \$\endgroup\$ – mtj Jun 28 '18 at 6:07
  • \$\begingroup\$ @mtj - I am trying to learn concurrency, thread safety and locking. So trying to use bare bones JDK. \$\endgroup\$ – AbNig Jun 28 '18 at 17:42
  • \$\begingroup\$ @esote my understanding is the synchronized pretty much provides shorthand for what the OP has done. In this instance, since they are not exploiting multi-read/one-write features of ReentrantReadWriteLock, a synchronise based solution might be preferable from a 'it's harder to get wrong' point of view. \$\endgroup\$ – VisualMelon Oct 26 '18 at 20:06
1
\$\begingroup\$

I don't normally review Java, but your code looks like it should work. You might consider using the synchronised language feature instead in this instance, as you are not gaining anything from manually using ReentrantReadWriteLock (e.g. you don't have anything which uses a Read lock (e.g. tryPeek)).


I appreciate that you want to use a minimal subset of the JDK, but I'd still advocate a Stack as the backing data-structure, since it (unsurprisingly) better represents the data, and in doing so reduces the opportunity for errors (e.g. ensuring you remove the topmost element, etc).


The return null in pop is not great, as your data-structure could just as well hold a null; it should either throw when you try to remove an element that doesn't exist, or you should provide some wrapper type which indicates whether a value was retrieved. At a minimum, the fact that the method returns null when the queue is empty should be clearly documented.

If the return null is to remain, I'd move it (or indeed a throw which replaces it) into an else clause within the try...catch, so that it is closer to the logic which determines it. Others might suggest inverting the if, so that you throw/return null on list.isEmpty(), and perform the meaningful operation otherwise.


Declaring "In finally" to the console might be useful for debugging, but as a consumer, this would infuriate me. Your class should provide a meaningful interface, and it shouldn't have any such unnecessary external influence.


You might consider making list and lock final, which will ensure they are assigned within the constructor, and prevent their being overwritten by accident. The type isn't disposable, so there is no need to clear the references at any point.


Be consistent in your styling: you use this.list, and then on the next line just lock. You've also a random empty line half way through push, with the rest of your code devoid of any spacing.

\$\endgroup\$
1
\$\begingroup\$

Looks fine, some minor improvements below...

  • make the private members final
  • If only using the write locking of ReentrantReadWriteLock just use a ReentrantLock
  • use the Lock interface for the lock private final Lock lock = new ReentrantLock()
  • in pop() throw an exception rather than returning null if the stack is empty
  • Replace the system.out w/ a Logger
  • Consider adding some javadocs to help the users of your implementation
\$\endgroup\$
  • \$\begingroup\$ Why throw an exception instead of returning null? That makes no sense. \$\endgroup\$ – FreezePhoenix Oct 27 '18 at 13:50
  • 1
    \$\begingroup\$ Then it is clear that the stack was empty so pop() was invalid, similar to java.util.Stack. What if we do push(null), then pop(), was it empty or are we returning items that were added \$\endgroup\$ – elvaston5 Oct 27 '18 at 15:02
0
\$\begingroup\$

It is unclear what you are trying to achieve but one of the below implementations should suit your requirements:

(If you're just trying to reinvent the wheel for educational purposes, I will recommend reading the source code of above mentioned java classes.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.