Make proper synchronisation in singleton

Question

I have a question regarding synchronisation. I have one single instance of:

`

@Service
public class MyService {

   //if singleton - should this map be static?
   private static Map<String, String> cache = new ConcurrentHashMap<String, String>();

 public String put(String value) {
    if (!cache.containsKey(value)) {
       String newValue = someRandomString();
        cache.put(value, newValue);
        return newValue;
    }
    return cache.get(value);
 }

public String get(String value) {
   return cache.entrySet().stream()
  .filter(e -> e.getValue().equals(value))
  .map(Map.Entry::getKey)
  .findFirst().get();
}

}

The question is - how to make proper synchronisation in case of singleton and many threads calling the service. Seems like both methods are affected and sometimes no appropriate data can be calculated/put into map for the same String values. Is ConcurrentHashMap to be replaced by SynchronizedMap? Should I make a synchronized block in both methods?

It's pretty misleading that `value` is the key in the first method and the value in the second. — shmosel, Aug 08 '18 at 19:55
It is really a bad duplicate, but really bad concerning the OP question. — davidxxx, Aug 08 '18 at 19:57

score 1 · Answer 1 · answered Aug 08 '18 at 19:45

1

Your put() method is not atomic. Try this instead:

public String put(String value) {
    return cache.computeIfAbsent(value, v -> someRandomString());
}

answered Aug 08 '18 at 19:45

shmosel

49,289
6
73
138

The `get()` method is not atomic, either. – John Bollinger Aug 08 '18 at 19:53
1

@JohnBollinger With respect to what? – shmosel Aug 08 '18 at 19:54
@JohnBollinger but it will not break the map state as `ConcurrentHashMap` is used. Only not updated value could be displayed in `get()`. It is the main reason to use `ConcurrentHashMap`. – davidxxx Aug 08 '18 at 19:55

John Bollinger · Answer 2 · 2018-08-08T20:04:00.373

0

The question is - how to make proper synchronisation in case of singleton and many threads calling the service.

The scope of your synchronization needs to cover whole logical operations, which in this case are much broader than individual method invocations on the internal Map. As such, you don't actually get anything useful from using ConcurrentHashMap or a synchronized Map implementation, because the scope of the critical regions they provide is not broad enough.

For your particular example, with only the put() and get() methods presented, the simplest approach would be to simply declare each method synchronized. And in that case, you can use a plain HashMap internally. For example,

public synchronized String put(String key) {
    // ...
}

public synchronized String get(String key) {
    // ...
}

This assumes that indeed there is only one instance of the class. If there were more than one, then synchronizing the methods would not be sufficient. Presumably, however, each instance would then want its own map, so changing cache to an instance variable would also be needed too, and that would resolve the problem together with the synchronization. Personally, I never would have made cache static in the first place.

edited Aug 08 '18 at 20:04

answered Aug 08 '18 at 19:49

John Bollinger

160,171
8
81
157

If you switch to `HashMap` you need to synchronize `get()` as well. – k5_ Aug 08 '18 at 19:53
Yes, @k5_, do you read "declare ***each*** method `synchronized`" differently? But I have updated to better highlight the point. – John Bollinger Aug 08 '18 at 19:55
Sorry missed that. – k5_ Aug 08 '18 at 19:56
1

This isn't reliable as long as `cache` is static. Though it probably shouldn't be. – shmosel Aug 08 '18 at 19:56
@shmosel, It will work fine if the instance is indeed a singleton, as the OP asserts. You're right, though, that the point bears some attention. – John Bollinger Aug 08 '18 at 19:59

Make proper synchronisation in singleton

2 Answers2