CODEWITHSUNDEEP
javamultithreading
meteor
meteor

Reputation: 2568

ConcurrentHashMap without synchronized keyword

I have a ConcurrentHashMap<String,List<String>> that is accessed by threads which check if the key exists before appending a value. I got this working with the use of synchronized keyword. If synchronized keyword is not used then the values are wrong. Isn't ConcurrentHashMap threadsafe? Or is there a issue with this code? Is is possible to get this code working without using synchronized to achieve better performance? Here is the code that does that.

ExecutorService executorService = Executors.newFixedThreadPool(4);
        final ConcurrentLinkedQueue<Future<?>> futures = new ConcurrentLinkedQueue<Future<?>>();
        final ConcurrentHashMap<String, List<String>> map = new ConcurrentHashMap<String, List<String>>();
        final JsonParser parser = new JsonParser();
        File[] files = new File(dir).listFiles();
for (final File tfile : files) {
                futures.add((Future<String>) executorService.submit(new Runnable() {
                    public void run() {
                        Object obj = parser.parse(new FileReader(tfile.getAbsolutePath()));
                        JsonObject jsonObject = (JsonObject) obj;
                        String documentname = obj.get("name").toString();
                        synchronized (map) {
                            List<String> paths = new ArrayList<String>();
                            //if key already exists append new path to the value
                            if (map.containsKey(documentname)) {
                                paths = map.get(documentname);
                            }
                            paths.add(tfile.getAbsolutePath());
                            map.put(documentname, paths);
                        }
                    }
                }));
            }

Upvotes: 2

Views: 607

Answers (2)

Martin Ellis
Martin Ellis

Reputation: 9651

You could try replacing this:

String documentname = obj.get("name").toString();
List<String> paths = new ArrayList<String>();
//if key already exists append new path to the value
if (map.containsKey(documentname)) {
    paths = map.get(documentname);
}
paths.add(tfile.getAbsolutePath());
map.put(documentname, paths);

with this:

String documentname = obj.get("name").toString();
List<String> paths = Collections.synchronizedList(new ArrayList<String>());
List<String> existing = map.putIfAbsent(documentname, paths);
if (existing != null) {
    paths = existing;
}
paths.add(tfile.getAbsolutePath());

The putIfAbsent method avoids a race condition that occurs if two threads both try to check (using containsKey) then put an entry into the map.

The synchronizedList method wraps the nested collection with a synchronized wrapper, so that you don't need to synchronize accesses to the nested collection. Alternatively, you could use a concurrent data structure from java.util.concurrent.

ThreadSafe is a static analysis tool that finds concurrency bugs. It can run standalone, in Eclipse or be used as a SonarQube plugin. In particular, it has a checkers for the two bugs in the code you've shown:

  • Non-atomic use of get-check-put (which can be replaced with putIfAbsent) (example report)
  • Shared non-threadsafe content in a concurrent collection (example report)

One problem with the code using putIfAbsent that I've suggested is that it always creates a collection. When the map already contains an entry for the given key, the new collection is simply discarded. However, this may be inefficient for your application, and can put extra pressure on the garbage collector.

Therefore, you might want to consider something like this:

String documentname = obj.get("name").toString();
List<String> paths = map.get(documentname);
if (paths == null) {
   paths = Collections.synchronizedList(new ArrayList<String>());
   List<String> existing = map.putIfAbsent(documentname, paths);
   if (existing != null) {
       paths = existing;
   }
   paths.add(tfile.getAbsolutePath());
}

Note the "Rule Description" link in bottom-left of the screen in this ThreadSafe non-atomic get-check-put link. Click the "Rule Description" link for further explanation of the get-check-put problem, and possible solutions to it.

Upvotes: 4

JB Nizet
JB Nizet

Reputation: 692231

ConcurrentHashMap is thread-safe, but that doesn't mean anything you do with a ConcurrentHashMap is. There are several problems in the code, if synchronized isn't used:

  1. You're accessing an ArrayList concurrently, from multiple threads. An ArrayList is not thread-safe
  2. You're doing several operations (containKey, get, put) in sequence on the Map. Each of these operations is thread-safe and atomic, but the sequence of operations is not. If you need that sequence of operations to be atomic, then you need to synchronize. Or you need to use an equivalent atomic operation: putIfAbsent().

Upvotes: 2

Related Questions