CODEWITHSUNDEEP
javamultithreading
Marvin
Marvin

Reputation: 1855

Is this Java class has visibility risk or thread-safe?

The main purpose of the class is to act like an Event Bus, multiple thread can register/unregister model through it.

public class ThreadSafeHash {
    private final HashMap<String, LinkedList<Model>> modelMap = new HashMap<>();

    public boolean addModel(final String category, final Model model) {
        LinkedList<Model> categoryContents = modelMap.get(category);
        if (categoryContents == null) {
            categoryContents = new LinkedList<>();
            modelMap.put(category, categoryContents);
        }

        synchronized (categoryContents) {
            return categoryContents.add(model);
        }
    }

    public boolean removeModel(final Model model) {
        LinkedList<Model> categoryContents = modelMap.get(model.getCategory());
        if (categoryContents == null) {
            return false;
        }

        synchronized (categoryContents) {
            return categoryContents.remove(model);
        }
    }
}

The main doubt is that multiple threads may put a list to the modelMap but this action is not synchroized, will this introduce visibility risk?

Upvotes: 1

Views: 184

Answers (3)

Dmitry P.
Dmitry P.

Reputation: 834

Note: This solution works only for Java 8 or higher

In addition I would suggest using ConcurrentHashMap.computeIfAbsent(..) method here, which returns a value by a key if it's present and computes and returns a new value (new LinkedList<>() in your case) if there is no such value mapped to the key. The whole method is atomic and if you use Collections.synchronizedList() on a LinkedList it will make the synchronized block unnecessary.

public class ThreadSafeHash {

private final ConcurrentMap<String, List<Model>> modelMap = new ConcurrentHashMap<>();

public boolean addModel(final String category, final Model model) {
      List<Model> categoryContents = modelMap.computeIfAbsent(
          category,
          k -> Collections.synchronizedList(new LinkedList<>());
      return categoryContents.add(model);
}

public boolean removeModel(final Model model) {
    List<Model> categoryContents = modelMap.get(model.getCategory());
    return categoryContents != null && categoryContents.remove(model);
}

}

Also note that the API of your ThreadSafeHash class gives a chance for it's user to make it inconsistent. If someone provides a wrong category for a model in the addModel(..) method, then the model never be removed, because removeModel(..) relies on model.getCategory().

Upvotes: 3

m c
m c

Reputation: 1104

HashMap is not thread-safe and that class is not thread safe because its access is not synchronised.

Bear in mind that even using a ConcurrentHashMap won't give you 100% "safety" against concurrency issues because the "pufIfAbsent" behaviour, provided by categoryContents == null and categoryContents = new LinkedList<>() then modelMap.put(category, categoryContents), is not atomic in any case.

ConcurrentMap.putIfAbsent

Upvotes: 1

Andy Turner
Andy Turner

Reputation: 140484

This is not thread safe because access to modelMap is not synchronized - but not because of visibility.

In the addModel method, two threads could both detect modelMap.get(category) == null, and so they would both put a new LinkedList into the map - one would stomp the other, so you would lose the model instance added in the first of those threads.

This happens because the "add if not present" check is not atomic; not that the updates are not visible.

Upvotes: 3

Related Questions