Java multiple thread synchronization

Question

I need to create an echo server that echo-prints the requested strings. One threads (Client) invokes the echo method to submit the strings to be echoed (which all echo method actually does is place the string in the job queue) and a separate thread then dequeues string from the queue and outputs them to the screen.

One of the steps was to make a queue static in order for it to be shared between threads and I did this by simply replacing:

public final Queue<String> requests = new LinkedList<String>();

with (not sure if correct)

    public static Queue<String> requests = new LinkedList<String>();

In this code:

public class EchoServer implements Runnable {

//make queue a static object
//public final Queue<String> requests = new LinkedList<String>();
public static Queue<String> requests = new LinkedList<String>();

public EchoServer() {
    new Thread(this).start();
}

//all echo does is place the string in the job queue
public void echo(String s) {
    requests.add(s);
}

public void run() {
    for(;;) realEcho(requests.remove());

    //synchronized here?
}

private void realEcho(String s) {
// do the real work of echo-printing
}

    public static void main(String[] args) {
    System.out.println(Thread.currentThread().getName());
    EchoServer r1 = new EchoServer();
    r1.echo("HEY"); 
    Thread t1 = new Thread(r1, "manager");
    t1.start();


    EchoServer r2 = new EchoServer();
    r2.echo("HI");
    Thread t2 = new Thread(r2, "client");
    t2.start();

    }

}

My issue now (besides the "NoSuchElement" exception and this happens because every thread tries to remove elements from the request queue without adding anything) is that I need to take care of the synchronization issue because the queue is shared among multiple threads. I am very lost when trying to figure out synchronization. Does anyone have some tips that could help me out? Any help is appreciated!

Answer 1

The problem is that LinkedList is not thread-safe.

Use a ConcurrentLinkedQueue instead.

Better yet, use a BlockingQueue, which does the same thing, except that it offers an additional method take() that will block until there is an item in the queue. That way, it won't be constantly rechecking the queue, eating up every spare processor cycle it can get checking if you added a new item in the last 50 nanoseconds since it checked last.
For example:

public class EchoServer implements Thread {

    public static BlockingQueue<String> requests = new LinkedBlockingQueue<>();

    public void echo(String s) {
        if(isInterrupted()) throw new IllegalStateException("Queue is closing!");
        requests.add(s);
    }

    @Override
    public void run() {
        try(
            while(!isInterrupted() || requests.peek()!=null)
               processEchoRequest(requests.take());
        } catch (InterruptedException e) {}
    }

    private void processEchoRequest(String s) { /* [...] */ }

}

The variables in the main method really need renaming:

public static void main(String[] args) {

    EchoServer clientEchoServer = new EchoServer(), \
               managerEchoServer = new EchoServer();

    managerEchoServer.start();
    managerEchoServer.echo("HI!");

    Thread manager = new Thread(managerEchoServer, "manager");
    manger.start();


    clientEchoServer.start();
    clientEchoServer.echo("HI!");

    Thread client = new Thread(managerEchoServer, "manager");
    client.start();

}