0

My goal is to implement a service which operates on Widgets, which have a property name. That service's operations consist of multiple smaller steps, which should be executed synchronously per name, meaning no two operations on widgets of the same name may be interweaved.

The easiest solution is to just lock those operations entirely, for example with EJBs like this:

@Singleton
public class WidgetService {
    @Lock(LockType.WRITE)
    public void doThing(Widget widget) {
        // many individual steps
    }
}

However, this would also mean that operations on widgets of different names cannot be executed in parallel, which massively impacts performance in my case. So my solution was to implement a per-name locking mechanism. The Lock itself looks roughly like this:

public class NameLock implements AutoCloseable {

    private static final ConcurrentMap<String, ReentrantReadWriteLock> globalLocks = new ConcurrentHashMap<>();

    private final String name;

    private NameLock(String name) {
        this.name = name;
        final ReentrantReadWriteLock lock = globalLocks.computeIfAbsent(name, s -> new ReentrantReadWriteLock());
        lock.writeLock().lock();
    }

    @Override
    public void close() {
        final ReentrantReadWriteLock lock = globalLocks.get(name);
        lock.writeLock().unlock();
    }
}

and is used like this:

public class WidgetService {
    public void doThing(Widget widget) {
        try (NameLock lock = new NameLock(widget.getName())) {
            // many individual steps
        }
    }
}

However, this means that the globalLocks map gets populated with locks which never get cleaned up. I cannot simply call globalLocks.remove(name) in the close method, because due to parallelism another thread may already retrieved that lock and is blocked at the lock() method. Is there an elegant way I can achieve this in a threadsafe, non-memory leaking way?

I wrote a test for this, which should just finish fast without errors if the locking works as expected. This passes for my implementation, but still leaves the locks in the map behind:

@Test
public void lockMultiple() throws Exception {
    final int nThreads = 10;
    final int nTasks = 200;
    final Random random = new Random();

    final ExecutorService threads = Executors.newFixedThreadPool(nThreads);
    final Function<Integer, Integer> func = i -> {
        try (NameLock lock = new NameLock("foo")) {
            // fuzzing
            Thread.sleep(random.nextInt(10));
        }
        return i;
    };
    final List<Future<Integer>> futures = IntStream.range(0, nTasks)
            .mapToObj(i -> (Callable<Integer>) () -> func.apply(i))
            .map(threads::submit)
            .collect(Collectors.toList());
    for (Future<Integer> future : futures) {
        final Integer i = future.get(10, TimeUnit.SECONDS);
    }
}
Felk
  • 7,720
  • 2
  • 35
  • 65
  • I've been suggesting Guava's [Striped](https://google.github.io/guava/releases/23.0/api/docs/com/google/common/util/concurrent/Striped.html) many times recently. Looks like a good fit here as well. – Kayaman May 30 '18 at 11:31
  • That seems to do exactly what I need, thanks! – Felk May 30 '18 at 12:38

0 Answers0