print alphabets and numbers using two threads one at a time

Question

I am working on below interview question where I need to print out alphabet and numbers using two threads. One prints alphabets (a,b,c...z) and other prints numbers(1,2,3....26). Now I have to implement it in such a way that the output should be:

a
1
b
2
...
...
z
26

So I came up with below code one without synchronization but for some reason it is not printing last alphabet which is z

class Output {
  private static final int MAX = 26;
  private static int count = 1;
  private static final Queue<Character> queue = new LinkedList<>(Arrays.asList(new Character[] {
      'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r',
      's', 't', 'u', 'v', 'w', 'x', 'y', 'z'}));
  private boolean isAlphabet = true;

  public void printAlphabet() {
    while (true) {
      if (count > MAX)
        break;
      if (!isAlphabet) {
        System.err.println(Thread.currentThread().getName() + " : " + queue.remove());
        isAlphabet = true;
      }
    }
  }

  public void printNumber() {
    while (true) {
      if (count > MAX)
        break;
      if (isAlphabet) {
        System.err.println(Thread.currentThread().getName() + " : " + count++);
        isAlphabet = false;
      }
    }
  }
}


public class PrintAlphabetNumber {
  public static void main(String[] args) {
    Output p = new Output();
    Thread t1 = new Thread(() -> p.printAlphabet());
    t1.setName("Alphabet");
    Thread t2 = new Thread(() -> p.printNumber());
    t2.setName("Number");

    t1.start();
    t2.start();
  }
}

Is there any issue in my above code? Also from synchronization perspective, does it look good or not?

`isAlphabet` should be declared as `volatile`. You can also use semaphore for this — Ivan, Oct 12 '18 at 20:16
I am allowed to use locks as well but in this particular scenario I doubt we need synchronization but I am open to use synchronization as well. — flash, Oct 12 '18 at 20:25
@Ivan why it has to be declared volatile. I understand we are sharing data between threads but don't you think for this particular problem we may not need synchronization? — flash, Oct 12 '18 at 20:30
Both threads are reading and writing to shared variable. It should be synched so that changes made by one thread are 100% visible to another — Ivan, Oct 12 '18 at 20:31
It does need to be volatile, because `isAlphabet = true` is not guaranteed to be visible to both threads otherwise. This is to allow the JVM more room to do optimizations. In an interview situation you would have to argue that a spinlock is the right choice for the situation, because it usually isn't. If you default to a spinlock without explaining the tradeoff, it would not be well received. It may be the right choice if the critical section is exceptionally short, but that's not true here -- `System.out.println` can and routinely does block for extended periods of time. — that other guy, Oct 12 '18 at 20:37
yes, count needs to be volatile too. There is no guarantee that an update to a shared variable is seen by another thread. You could for example not see that `count > MAX` is already true and print too many things. You do have some guarantees because of the sync point introduced by changing isAlphabet: https://stackoverflow.com/questions/17108541/happens-before-relationships-with-volatile-fields-and-synchronized-blocks-in-jav but that's not enough here if I'm not mistaken — zapl, Oct 12 '18 at 21:29
btw without using volatile either for count or isAlphabet, this program works fine without any issues. — flash, Oct 12 '18 at 21:51

score 5 · Answer 1 · answered Oct 12 '18 at 20:17

5

for some reason it is not printing last alphabet which is z

You abort when count > MAX, which is true after the last number.

After the last number, you're supposed to print the last letter, but now count > MAX so it's already stopping.

from synchronization perspective, does it look good or not?

No, this does not look good.

You are using a spinlock. This is very inefficient as both loops use 100% CPU constantly, whether they have work to do or not. It's also not guaranteed to work with non-volatile lock variables.

The classic Java solution would use wait()/notify().

answered Oct 12 '18 at 20:17

that other guy

116,971
11
170
194

Make sense now. How can I fix this? I thought in this particular scenario we don't need locks at all but i am open to use lock solution as well. If you have any better or efficient way to do this, please let me know – flash Oct 12 '18 at 20:27
In the simplest case you could make your methods synchronized, and add `notify()` whenever you change `isAlphabet` and `wait()` any time you check it and it's not the value you wanted. – that other guy Oct 12 '18 at 21:00
@zapl got it.. can you add an answer basis on this solution. Also it will be great if you can add Java 7 solution for this, it will help me understand better. – flash Oct 12 '18 at 21:55

score 1 · Answer 2 · answered Oct 12 '18 at 20:49

like they said this is not good code for doing this, but the issue with this code is that you got the if condition backwards.

public void printAlphabet() {
    while (true) {
        if (count > MAX)
            break;
        if (isAlphabet) {// this was !isAlphabet
            System.err.println(Thread.currentThread().getName() + " : " + queue.remove());
            isAlphabet = false;//also here
        }
    }
}

public void printNumber() {
    while (true) {
        if (count > MAX)
            break;
        if (!isAlphabet) {// this was isAlphabet
            System.err.println(Thread.currentThread().getName() + " : " + count++);
            isAlphabet = true;//also here
        }
    }
}

print alphabets and numbers using two threads one at a time

2 Answers2