blocking queue implementation

Question

I've just implemented a custom blocking queue with an semaphore.

for a reason i cant find, my queue isn't getting blocked by the semaphore when my queue is empty.

here's my implementation:

package poolThread;

import java.util.LinkedList;
import java.util.Queue;
import java.util.concurrent.Semaphore;

public class MyQueue<E> {
Semaphore s = new Semaphore(0, true);
private Queue<E> queue = new LinkedList<E>(); 


public boolean isEmpty(){
    return this.queue.isEmpty();
}
public void enqueue(E e){
    queue.add(e);
    s.release();
}
public E dequeue(){
    E e = null;
    try {
        s.acquire();
    } catch (InterruptedException e1) {
        // TODO Auto-generated catch block
        e1.printStackTrace();
    }
    e = queue.remove();
    return e;

}

}

could you help me find the fault in my code?

it does only sometimes, i used this blocking queue in order to implement a threadpool and when i run few threads it does not block. — gil, May 04 '16 at 19:46
i also used the java implementation of ArrayBlockingQueue instead mine in order to check if it works and it does works, so i'm pretty sure the problem is in my blocking queue implementation. — gil, May 04 '16 at 19:52
could you show your test harness too? seems odd (and error-prone, since the list has no memory guarantees) to implement isEmpty by checking the list and not your available permits. also it would be better to throw InterruptedException and not eat it. — Nathan Hughes, May 04 '16 at 20:36

score 2 · Accepted Answer · answered May 04 '16 at 21:42

2

The problem here is the LinkedList - which isn't thread-safe. So even if the permits are acquired properly, the remove() operation on the LinkedList can (and will) cause troubles. Here's a simple "test case" to show the behavior:

MyQueue<String> x = new MyQueue<>();

ExecutorService es = Executors.newFixedThreadPool(2);
for (int j = 0; j < 2; j++)
    es.submit(() -> {
        String tn = Thread.currentThread().getName();
        for (int i = 0; i < 2; i++)
            x.enqueue("v" + i);

        for (int i = 0; i < 2; i++) 
            System.out.println(tn + " deq: " + x.dequeue());
    });

The output will be something like (you will see nulls due to NoSuchElementExceptions on the remove method):

pool-1-thread-2 deq: v0
pool-1-thread-1 deq: null

The simplest solution for this would be to replace your LinkedList with a java.util.concurrent.ConcurrentLinkedQueue.

answered May 04 '16 at 21:42

nyname00

2,496
2
22
25

all my idea behind the implementation is to implement everything my self to be thread safe and learn from it and not to use a java thread safe element. you got any suggestions for me how to implement it without the concurrent linked list? – gil May 04 '16 at 22:16
@gilleibovitz well then you will have to ensure list access happens only inside a `synchronized` block or use a `Lock`. See [here](http://tutorials.jenkov.com/java-concurrency/locks.html) for more info – nyname00 May 05 '16 at 06:48
@gilleibovitz: or use Collections.sychronizedList, which is what the example in Java Concurrency in Practice does. – Nathan Hughes May 05 '16 at 14:49

blocking queue implementation

1 Answers1