Synchronize by value rather that object

Question

I implemented a simple locking solution that creates a lock for a value rather than object and want to know the experts' opinion for possible performance or security drawbacks. The idea is to use it for account balance update acquiring the lock for unique account number.

Here is an implementation:

import java.util.*;

public class Mutex<T> {

    private final Set<T> set = new HashSet();

    public synchronized Lock acquireLock(
            T value
            ) throws InterruptedException {
        while(!set.add(value)) {
            this.wait();
        }
        return new Lock(value);
    }

    public class Lock {

        private final T value;

        public Lock(T value) {
            this.value = value;
        }

        public T getValue() {
            return value;
        }

        public void release() {
            synchronized(Mutex.this) {
                set.remove(value);
                Mutex.this.notifyAll();
            }
        }
    }
}

And here is a sample usage to check the operability:

public class Test {

    private Mutex mutex = new Mutex();

    public static void main(String[] args) {
        Test test = new Test();
        Thread t1 = new Thread(() -> {
            try {
                test.test("SameValue");
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        });
        t1.setName("Thread 1");
        Thread t2 = new Thread(() -> {
            try {
                test.test("SameValue");
            } catch (InterruptedException ex) {
                ex.printStackTrace();
            }
        });
        t2.setName("Thread 2");

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

    public void test(String value)
            throws
            InterruptedException {
        Lock lock = mutex.acquireLock(value);
        try {
            Thread.sleep(5000);
            System.out.println(Thread.currentThread().getName());
        } finally {
            lock.release();
        }
    }
}

Answer 1

Regarding your implementation,

I would have use a Set instead of a List to hold your values (I assume the values have proper equals/hashcode for this to make sense): the List#contains method is in O(n) which might be expensive if you have a lot of IBAN used at the same time.

Also, you should avoid using synchronize(this) (which is the same as the synchronized keyword on method).

To solve your problem, I use something like this:

import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.lang.ref.SoftReference;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class Locks<T> {

    private final Lock lock = new ReentrantLock();

    //a Bimap from guava might be better here if you have the dependency
    //in your project
    private final Map<Reference<?>, T> valuePerReference = new HashMap<>();
    private final Map<T, Reference<Lock>> locks = new HashMap<>();

    private final ReferenceQueue<Lock> lockReferenceQueue = new ReferenceQueue<>();

    public Locks() {
        final Thread cleanerThread = new Thread(new Cleaner());
        cleanerThread.setDaemon(true);
        cleanerThread.start();
    }

    /**
     * @param value the value the synchronization must be made on
     * @return a lock that can be used to synchronize block of code.
     */
    public Lock getLock(T value) {
        lock.lock();
        try {
            return getExistingLock(value).orElseGet(() -> createNewLock(value));
        } finally {
            lock.unlock();
        }
    }


    private Optional<Lock> getExistingLock(T value) {
        return Optional.ofNullable(locks.get(value)).map(Reference::get);
    }

    private Lock createNewLock(T value) {
        //I create ReentrantLock here but a Supplier<Lock> could be a parameter of this
        //class to make it more generic. Same remark for SoftReference below.
        final Lock lock = new ReentrantLock();
        final Reference<Lock> reference = new SoftReference<>(lock, lockReferenceQueue);
        this.locks.put(value,reference);
        this.valuePerReference.put(reference,value);
        return lock;
    }


    private void removeLock(Reference<?> reference) {
        lock.lock();
        try {
            final T value = valuePerReference.remove(reference);
            locks.remove(value);
        } finally {
            lock.unlock();
        }
    }


    private class Cleaner implements Runnable {
        @Override
        public void run() {
            while (!Thread.currentThread().isInterrupted()) {
                try {
                    final Reference<? extends Lock> garbaged = lockReferenceQueue.remove();
                    removeLock(garbaged);
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }
            }
        }
    }

}

I then use this like this:

import java.util.concurrent.locks.Lock;

public class Usage {

    private final Locks<String> locks = new Locks<>();


    public void doSomethind(String iban) {
        final Lock lock = locks.getLock(iban);
        lock.lock();
        try {
            //.. do something with your iban
        } finally {
            lock.unlock();
        }
    }
}

Although it uses ReentrantLock, the code can be easily modified for ReadWriteLock for instance.