Is the following implementation thread safe?

Question

I have a java thread safe map which stores String to Object mappings:

ConcurrentHashMap<String, MyObject> map = new ConcurrentHashMap<>();

Here MyObject is a user defined class. Now consider the following method:

public void replace(String key, MyObject o) {
    MyObject m = map.get("somekey");
    synchronized(m) {
        // Modify some internal components of m
        // and some other objects related to m
    }
}

My intention here is I want to maximise performance by not putting a lock on the whole class but only on threads that try to access the same key in the map. Basically every time an entry is replaced in the map (not added) this part of the code gets executed. Something like:

public void put(String key, MyObject o) {
    if (map.putIfAbsent(key, o) == null){ //do something
    }
    else {
        replace(key, o);
    }
}

For all practical purposes we can assume that this is the only method through which a reference to MyObject can be changed. Is this implementation thread safe?

That is impossible to know without the implementation of `MyObject`, and if it is possible to access this specific instance in another way. But the simple answer is: probably not. — Mark Rotteveel, Nov 02 '16 at 10:38
@MarkRotteveel I agree my question was incomplete. I have added some more details now. — Sohaib, Nov 02 '16 at 10:54
@NicolasFilotto You could think it in a way that put does not replace. You need to be explicit when replacing an entity. — Sohaib, Nov 18 '16 at 16:31
@Sohaib I thought that you resolved your problem by yourself, you are still looking for a solution? — Nicolas Filotto, Nov 18 '16 at 16:53
@NicolasFilotto I did resolve it yes. Thanks for your help in that. — Sohaib, Nov 18 '16 at 17:22

Nicolas Filotto · Accepted Answer · 2016-11-02T15:00:11.487

3

Is the following implementation thread safe?

It depends on how you access and modify your instances of MyObject in your code, if you always access and modify a given instance of MyObject within a synchronized block using this MyObject instance as object's monitor then yes it would be thread safe, otherwise it won't be.

Your put method is not thread safe as putIfAbsent and replace are not executed atomically which could lead to race condition issues, use merge(K key, V value, BiFunction<? super V,? super V,? extends V> remappingFunction) to do the exact same thing but atomically as next:

public void put(String key, MyObject o) {
    map.merge(
        key, o,
        (v1, v2) -> {
            // Modify some internal components of v1
            // and some other objects related to v1
            return v2;
        }
    );
}

edited Nov 02 '16 at 15:00

answered Nov 02 '16 at 10:42

Nicolas Filotto

43,537
11
94
122

Yes the question for me is not about modifying myObject but the fact that someone else might be trying to operate on the same key of the map by replacing it with a different instance of MyObject. – Sohaib Nov 02 '16 at 10:55
check my response update about the put method, is it what you wanted? – Nicolas Filotto Nov 02 '16 at 11:05
Yes but I need to update somethings on entry as well. I think the correct approach here would be something like this answer: http://stackoverflow.com/a/5640671/1901744 That may work even though I think it is not performant. – Sohaib Nov 02 '16 at 14:40
Simply move the code of `replace` in `put` like my response update and remove `replace` – Nicolas Filotto Nov 02 '16 at 15:01
1

Yes but there is specific code for `put` as well which is different from `replace`. Also I am bound to jdk1.7 for now. Thanks for the insights. I think I can come up with a solution to this now. – Sohaib Nov 03 '16 at 13:22

Is the following implementation thread safe?

1 Answers1