How to implement a Reader/Writer program using threads?

Question

I have an issue with a Reader/Writer implementation. I'm supposed to write a Reader class that takes a String from the console and adds it to a Queue and a Writer class that removes the String from the same queue and outputs it onto the console, using Threads. I wrote my program for just one String (type one String and it outputs that String through the Queue) and that worked perfectly. Now I'm struggling to make it so that I can input multiple Strings, press Enter and the Reader then adds it to the Queue and the Writer then displays it. If the String quit is typed, both threads have to stop and the program should end.

My Reader idea looks like this:

Scanner k = new Scanner(System.in);
in = k.nextLine();
if(in.equals("quit"))
  System.exit(0);

synchronized(q){
  while(!(in.equals("quit"))){
    // System.out.println(q.isEmpty());
    q.enqueue(in);
    in = k.next();
    if(in.equals("quit"))
      System.exit(0);
  }
}

And my Writer looks like this:

public void run(){
  synchronized(q){
    while(!q.isEmpty()){
      String out = q.dequeue();
      System.out.println(out);
    }
  }
}

My Reader seems to work fine, as I built in Sys.out.(q.isEmpty) after adding to the queue. It shows me that the queue is filling up, but nothing is output onto the console from the Writer class. Writing quit stops the program with no issues.

I don't think I understand threads perfectly. My main method just creates the Threads with Thread t1 = new Thread(new Reader(queue)); and the same for Writer and then starts both threads.

Answer 1

synchronized(q){
  while(!(in.equals("quit"))){
    // System.out.println(q.isEmpty());
    q.enqueue(in);
    in = k.next();
    if(in.equals("quit"))
      System.exit(0);
  }
}

This synchronized block is way too big. As a rule you want to synchronize for as short a time as possible. Get in and get out. The longer you synchronize the longer other threads are blocked.

Performing user input while synchronized is a no-no. Other threads shouldn't be blocked because the user's a slow typer.

Worse, you've got the entire program's loop inside the synchronized block. Bad reader! So greedy. It's not giving up the q lock until the user has entered all of their input and typed "quit". Only then does it release the lock and let the writer proceed.

while(!(in.equals("quit"))){
  // System.out.println(q.isEmpty());
  synchronized(q){
    q.enqueue(in);
  }
  in = k.next();
  if(in.equals("quit"))
    System.exit(0);
}

---

The writer has a different fatal flaw. As soon as the queue is empty it quits. The queue's going to be empty a lot of the time, though, isn't it? When it is the writer shouldn't just die.

A quick fix is to wrap the whole thing in an infinite loop:

public void run(){
  while (true) {
    synchronized(q){
      while(!q.isEmpty()){
        String out = q.dequeue();
        System.out.println(out);
      }
    }
  }
}

It'll keep the writer alive. But it will also chew up CPU time, looping millions of times while that darned user slowly pecks at the keyboard. If you check your system monitor you'll see the program spike to 100% CPU usage. Not great.

Fixing this problem is a bit out of scope for this Q&A. The short answer is to use wait() and notify() to have the writer go to sleep until there's something available.