java synchronize multiple thread issue

Question

I just write some code to test the multiple threads how to synchronize,but I cannot get my expected result.The code can start 3 threads,but only one thread to process the shared resource.what is wrong with my code.

class ThreadDemo1{
   public static void main (String[] args){
       MultiThread tt = new MultiThread();
       new Thread(tt).start();
       new Thread(tt).start();
       new Thread(tt).start();
   }
}
class MultiThread implements Runnable {
  int tickets = 100;
  Object _lock = new Object();
  public void run () {
    System.out.println(Thread.currentThread().getName());
    synchronized(_lock) {
      while (true) {  
        if (tickets>0) {
          try {
            Thread.sleep(10);
          } catch (Exception e) {}
          System.out.println(Thread.currentThread().getName() + " is selling "+tickets--);
        }
      }
    }
  }
}

Answer 1

[1]First, there are several bad practice/mistakes in your posted code:

(1) It's better that the Lock Object to be singleton. You can use an static field object or the Class itself(Since there is only one Class in memory)

Object _lock = new Object();
private static final Object _lock = new Object();

(2) Put the while(true) {...} out of the synchronized block. In your code, if the 1st thread obtains the Lock, it will process ALL the tickets and will not stop. Should make every thread try to obtain the Lock in each iteration of the loop.

(3) For the Thread.sleep(10), I guess you mean the thread is doing some heavy job. But it's not a good practice to put this kind of code in synchronized block(Or another name: critical region). Because there is only one thread can access the synchronized block at one time. The behavior of you code is like a single thread program, because other threads must wait until the currently running thread finishes its job.

Pls see below code:

public class ThreadDemo1 {
    public static void main(String[] args) {
        MultiThread tt = new MultiThread();
        new Thread(tt).start();
        new Thread(tt).start();
        new Thread(tt).start();
    }
}

public class MultiThread implements Runnable {
    private static int tickets = 100;
    private static final Object _lock = new Object();

    public void run() {
        System.out.println(Thread.currentThread().getName());
        while (tickets > 0) {
            try {
                synchronized (_lock) {
                    if (tickets > 0) {
                        System.out.println(Thread.currentThread().getName() + " is selling " + tickets--);
                    }
                }
                Thread.sleep(10);
            } catch (Exception e) {
            }
        }
    }
}

[2]Second, if you just want to synchronize the threads in picking the tickets. Try to use Atomic* Classes instead of synchronized block, it’s No-lock and will bring you a better performance. Example:

import java.util.concurrent.atomic.AtomicInteger;

public class MultiThreadAtomic implements Runnable {
    private static AtomicInteger tickets = new AtomicInteger(100);

    public void run() {
        System.out.println(Thread.currentThread().getName());
        int ticketsRemaining = 0;
        while ((ticketsRemaining = tickets.getAndDecrement()) > 0) {
            System.out.println(Thread.currentThread().getName() + " is selling " + ticketsRemaining);
            try {
                Thread.sleep(10);
            }
            catch(InterruptedException ie) {}
        }
    }
}

Answer 2

You are sleeping while holding the lock. There is no reason to multithread if you are going to do that.

public void run () {
    System.out.println(Thread.currentThread().getName());
    while(tickets > 0) {
        synchronized(_lock) {
            if (tickets > 0) {
                System.out.println(Thread.currentThread().getName() + " is selling " + tickets--);
            }
        }
        try {
            Thread.sleep(10);
        } catch (Exception e) {
        }
    }
}

I'm guessing the sleep was a placeholder for your processing. If possible, you should do the check and decrement inside the synchronized block, but your lengthy processing outside it.

In order for locks and multi-threading to do anything useful for you, you must make sure that your synchronized code takes as little time as possible, since that is the code that can be run by only one thread at a time.

In your code, the only thing that wasn't effectively single-threaded was your first System.println.

---

FYI, with that in mind, if you could have your print statements accurate but possibly out of order, it would be even better to have:

public void run () {
    System.out.println(Thread.currentThread().getName());
    while(tickets > 0) {
        int oldTickets = 0;
        synchronized(_lock) {
            if (tickets > 0) {
                oldTickets = tickets--;
            }
        }
        if(oldTickets > 0) {
            System.out.println(Thread.currentThread().getName() + " is selling " + oldTickets);
            try {
                Thread.sleep(10);
            } catch (InterruptedException e) {
            }
        }
    }
}