5

I need a way to allow only one thread to modify data related to a service ticket. More than one thread may be attempting to modify the ticket data at the same time.

Below is a simplified version of my approach. Is there a better way to do this? Maybe with java.util.concurrent packages?

public class SomeClass1
{
    static final HashMap<Integer, Object> ticketLockMap = new HashMap<Integer, Object>();


    public void process(int ticketNumber)
    {
        synchronized (getTicketLock(ticketNumber))
        {
            // only one thread may modify ticket data here

            // ... ticket modifications here...
        }
    }


    protected static Object getTicketLock(int ticketNumber)
    {
        Object ticketLock;

        // allow only one thread to use map
        synchronized (ticketLockMap)
        {
            ticketLock = ticketLockMap.get(ticketNumber);

            if (ticketLock == null)
            {
                // first time ticket is locked
                ticketLock = new Object();
                ticketLockMap.put(ticketNumber, ticketLock);
            }
        }

        return ticketLock;
    }
}

Additionally, if I don't want the HashMap filling up with unused locks, I would need a more complex approach like the following:

public class SomeClass2
{
    static final HashMap<Integer, Lock> ticketLockMap = new HashMap<Integer, Lock>();


    public void process(int ticketNumber)
    {
        synchronized (getTicketLock(ticketNumber))
        {
            // only one thread may modify ticket data here

            // ... ticket modifications here...

            // after all modifications, release lock
            releaseTicketLock(ticketNumber);
        }
    }


    protected static Lock getTicketLock(int ticketNumber)
    {
        Lock ticketLock;

        // allow only one thread to use map
        synchronized (ticketLockMap)
        {
            ticketLock = ticketLockMap.get(ticketNumber);

            if (ticketLock == null)
            {
                // first time ticket is locked
                ticketLock = new Lock();
                ticketLockMap.put(ticketNumber, ticketLock);
            }
        }

        return ticketLock;
    }


    protected static void releaseTicketLock(int ticketNumber)
    {
        // allow only one thread to use map
        synchronized (ticketLockMap)
        {
            Lock ticketLock = ticketLockMap.get(ticketNumber);

            if (ticketLock != null && --ticketLock.inUseCount == 0)
            {
                // lock no longer in use
                ticketLockMap.remove(ticketLock);
            }
        }
    }
}


class Lock
{
    // constructor/getters/setters omitted for brevity
    int inUseCount = 1;
}
Jeff Miller
  • 1,424
  • 1
  • 10
  • 19
  • 2
    You should be able to use something like `ConcurrentHashMap` instead of `HashMap` for the ticket locks and avoid the `synchronized` bit which is poorly performing. `putIfAbsent()` should be exactly what you need. – manub Jun 12 '13 at 15:47
  • @Jeff check this thread http://stackoverflow.com/questions/659915/synchronizing-on-an-integer-value – Sergei Ledvanov Dec 29 '15 at 07:50
  • @manub ConcurrentHashMap can not handle this, because we need lock on both `getTicketLock ` and `releaseTicketLock ` method. BTW, I think this approach is good enough, with no condition problem, no memory leak, after I see lots of bad solutions. – tianzhipeng Jul 09 '18 at 03:37

1 Answers1

2

You might be looking for the Lock interface. The second case could be solved by a ReentrantLock, which counts the number of times it has been locked.

Locks have a .lock() method which waits for the lock to acquire and an .unlock method which should be called like

 Lock l = ...;
 l.lock();
 try {
     // access the resource protected by this lock
 } finally {
     l.unlock();
 }

This could then be combined with a HashMap<Integer, Lock>. You could omit the synchronized calls and cut down on lines of code.

serv-inc
  • 35,772
  • 9
  • 166
  • 188
  • 5
    How do you make sure that the locks get removed from the map when they are not going to be used any longer in a thread-safe way so that you make sure that the map doesn't grow out of control? – user3289695 May 28 '18 at 06:54
  • @user3289695 what about using something like Guava cache with key expiry? That's just an assumption. – Vitaly Chura Jul 15 '23 at 05:15
  • @user3289695 : how about checking the ReentrantLock.getHoldCount and deleting from the map if 0 ? – serv-inc Jul 18 '23 at 08:27