Synchronized in Java

Hi,
You need to put the keyword synchronized in front of the getTicket() method (and all other methods you want to be synchronized) of the TicketPool class.
Otherwise, that method will not be synchronized.

https://www.javatpoint.com/synchronization-in-java

Regards,
    Tomas Helgi

This class:

class TicketPool {
   int availableTicket;
   
   TicketPool(int totalTicket)
   {
      this.availableTicket = totalTicket;
   }
   
   void setTicket(int newNumber)
   {
      availableTicket = newNumber;
   }
   
   int getTicket(){
      return availableTicket;
   }
  }

Select allOpen in new window

is fundamentally not "thread safe".  If two threads try to work with it at once, they will get all manner of problems.
E.g.
This line:

pool.setTicket(pool.getTicket()-1); 

Select allOpen in new window

is a subtle trap.  Because if two threads start to execute this at the same time, they could both see a ticket pool that contains 5 tickets (e.g.) and then both reduce it to 4 (which would be wrong - they'd have taken 2 tickets from the pool).

So instead I think you should rewrite your TicketPool class, something like this:

class TicketPool {
   // Make sure all accesses to this variable are safe
   AtomicInteger availableTicket = new AtomicInteger() ;
   
   TicketPool(int totalTicket)
   {
      this.availableTicket.set(totalTicket);
   }

   // NOTE: This will return true IF the current ticket count
   // is "oldNumber" otherwise it will do nothing and return false.
   // Now the sequence:
   // int currentTickets = getTicket() ;
   // boolean changed = setTicket(currentTickets, currentTickets-1) ;
   // will modify the ticket count in a safe way and return true only
   // if the tickets were changed (i.e. another thread didn't get in there
   // and modify the tickets in between the reading and setting of the value).   
   boolean setTicket(int oldNumber, int newNumber)
   {
      return availableTicket.compareAndSet(oldNumber, newNumber) ;
   }
   
   int getTicket(){
      return availableTicket.get();
   }
      
}

Select allOpen in new window

Hi,

As TicketPool is, as I see it, a shared object then all methods in the class need to have the synchronized keyword as I mentioned in an earlier comment otherwise the class is not thread-safe as dpearson points out.
https://www.baeldung.com/java-thread-safety

Regards,
    Tomas Helgi

Adding "synchronized" to each of the methods in TicketPool as Thomas suggests will make it thread safe.  But that's kind of an "old school" approach to thread safety in Java.

The problem is that anyone can come along and add a method to the class, forget to include the synchronized keyword and break the safety.  Or even worse you can subclass this class and break the contract in all manner of ways.

That's why in Java we generally now want to prefer to use the classes from the concurrency package like "AtomicInteger".  They wrap the synchronization logic into the object, which ensures that any reference to the underlying data will be thread safe (because it's "inside" the Atomic object).

There are still occasionally places where using "synchronized" still makes sense but that's now a rare choice, not something you should often need to do.

Also while we're at it, you also should *never* be subclassing Thread and implementing the run() method.  That's again very old school and a bad idea.  Check out the Executors class instead which offers things like built in thread pool support and is the correct way to make a new Thread in 2020.  Your code won't look radically different but it will be much better behaved.

"When ever a thread enters into Java synchronized method or block it acquires a lock and whenever it leaves synchronized method or block it releases the lock. "

I put " synchronized(pool)" in line 3 below. If two threads try to access line 3, only one can allow to further execute while others will be waiting. The thread that are allowed to execute will be released the ticketpool at line 24,( ie. end of the block of " synchronized(pool)" ). Hence, I suppose one of the thread waiting at line 3 will get the right to execute one. If it doesn't lock ticketpool, what does line 3 mean ? In my result, it seems that lock to ticketpool will be released only after run() method is left completely. Please clarify as I'm a bit confused.

My post code has problem of only a single thread keep buying the ticket, without giving the chance to others. I don't have problem for two threads accessing the TicketPool variable at the same time.

Result :
John reserved ticket # 10
John bought ticket # 10, ticket left: 9
John reserved ticket # 9
John bought ticket # 9, ticket left: 8
John reserved ticket # 8
John bought ticket # 8, ticket left: 7
John reserved ticket # 7
John bought ticket # 7, ticket left: 6
John reserved ticket # 6
John bought ticket # 6, ticket left: 5
John reserved ticket # 5
John bought ticket # 5, ticket left: 4
John reserved ticket # 4
John bought ticket # 4, ticket left: 3
John reserved ticket # 3
John bought ticket # 3, ticket left: 2
John reserved ticket # 2
John bought ticket # 2, ticket left: 1
John reserved ticket # 1
John bought ticket # 1, ticket left: 0

Thx again.

      while(pool.getTicket()>0){
         
         synchronized(pool) {         
                     
           if (pool.getTicket()>0) {
                
             
            // place order: print out "Thread xx reserved ticket --- #n"
             System.out.println(Thread.currentThread().getName()+" reserved ticket # "+ pool.getTicket());
         
              //delay 10ms: sleep(10)
            try{
                 Thread.currentThread().sleep(10);
               }
                catch (InterruptedException itro){
                 System.out.println("sleep is interrupted");
               }
         
                //payment finished: decrease availableTicket by 1 
                pool.setTicket(pool.getTicket()-1);
         
                //print the leftover tickets
                System.out.println(Thread.currentThread().getName()+" bought ticket # " + (pool.getTicket()+1) + ", ticket left: "+pool.getTicket());
             }
               
      }

Select allOpen in new window

without giving the chance to others.  

And I think the clue is that only one thread is able to access the synchronized code at one time . . . which of course does not break the promise of 'synchronized' at all. What you are seeing is that one thread usually obtains all the tickets, (although this will not ALWAYS be the case).

As Doug has said, the complexity and pitfalls of concurrent thread coding is now not something that should be done without using the latest proper concurrency API, and as he also said, the Executors class.