Is a volatile hashmap sufficient in this case?

Question

I previously wrote a piece of code which loads classes dynamically from a jar file. So, basically there can be a.jar in 123-de directory and another a.jar in 456-fg directory. Now depending on certain parameters i decide which jar to use and loads a class say calc.java either from 123-de directory or 456-fg directory.

To do this I had to create classloaders and use these classloaders to load calc.java

Ofcourse there should be a single class loader per jar and single class loaded in memory per class.

To do this I used a concurrent hashmap which stores the class loader/ class. Say key for this concurrent hashmap is directory name.

So, Given a directory , i check if classloader is already present- if not i create on one then stores it.

if(classLoaderMap.get(directoryPath) == null){
        rlock.lock();
        try{
            if(classLoaderMap.get(directoryPath) == null){

                ClassLoader classLoader = // Create classLoader here.
                classLoaderMap.put(directoryPath, classLoader);
            }
        }finally{
            rlock.unlock();
        }
    }

This code is tested and is working fine. But today i was revisiting this code and observed that i did not really require concurrenthashmap because i am using explicit locking to write to it. I only need memory visibility because i am reading it outside lock. So, I am really thinking would just volatile hashmap had done the job? Should i revert it (Dont want though as it is already tested) OR is it okay if i keep this?

Answer 1

No, it's not safe to use a regular map, as you would need to lock on both reads and writes. One option is to use a ReadWriteLock so you can support non-exclusive reads, while writes remain exclusive to both reads and writes. Another option (probably the better one) is to stick with the ConcurrentHashMap and get rid of the lock. As to the question of how to update atomically, just use the atomic computeIfAbsent() method:

classLoaderMap.computeIfAbsent(directoryPath, p -> /* create class loader */);