5

I am wondering what is the best approach for following situation:

Lets say, that we have some kind of factory MyUtilFactory returning cached instances of MyUtil class. Multiple instances of MyUtil are holded in a static Map<String,MyUtil> utilsCache.

And now, abstract flow for lazy creation should be as follows:

  1. if cache utilsCache is empty, perform first initialization - load instances to static cache map.
  2. get instance from cache
  3. check if retrieved instance is initialized
  4. if instance is not initialized, perform instance initialization (expensive)
  5. return instance.

Now, step 1 and 4 needs to be synchronized, as initialization is time consuming and it would be executed multiple times, however it is required only for the first initialization of cache and instance.

It is easy to put everything in synchronized block(or even method) and there would be no problem at all, however I would like to avoid synchronization where it is not needed.

One approach would be something like that 

        if (!someObject.isInitialized()) { // check if it is ready without synchonization
            synchronized (someObject) { // its not ready so lets synchronize from here
                if (!someObject.isInitialized()) { //first thread will get the samo outcome, but other threads will not
                    performInitialization();
                }
            }
        }

Now the point of repeating the condition in synchronized block, is to allow first thread to perfom initialization. Any subsequent threads will wait for the first one to complete initialization, and than condition in synchronized if statment will not be fulfilled. It can be done just like that and it works preatty well, but I don't think it is the best and the most elegant approach for such issue. My question is, how to do it "the right way"

Antoniossss
  • 31,590
  • 6
  • 57
  • 99
  • Subjective questions like this are not a good fit for Stack Overflow, since they encourage opinionated answers. I'm afraid I'm voting to close as "primarily opinion based". If you can edit the question to be more concrete and objective, then I will withdraw that vote. – Duncan Jones Jan 08 '14 at 12:34
  • Using a [thread safe multiton pattern](http://stackoverflow.com/a/18149547/829571)? – assylias Jan 08 '14 at 12:36
  • @Duncan You are right. Asking for elegant standard solution is a query for opinions and poll, which is opposite of asking a standard elegant solution. You must close the question. – Val Jan 08 '14 at 12:38
  • `It is easy to put everything in synchronized block(or even method) and there would be no problem at all, ` IMHO go with this first and only if `performInitialization()` is perceptibly expensive, try to optimize. – PeterMmm Jan 08 '14 at 12:42
  • @assylias such pattern is already applied, however solution in post you have pointed out is not thread safe (multiple threads can create an put new instances for the same key in concurrent environement) – Antoniossss Jan 08 '14 at 12:43
  • @PeterMmm there is not much to do in to optimize initialization as it is based on external resources (networking etc.) – Antoniossss Jan 08 '14 at 12:44
  • @Antoniossss I believe the approach in the link I posted **is** thread safe (I wrote it!). If you tink otherwise please point out the issue. – assylias Jan 08 '14 at 12:45
  • That is quite easy - two threads are allowed to create new instance of `Multiton` object. It is obvious that only one of them will be placed in map, but if construction of `Multiton` object is expensive or it uses some kind of other common resources, you should (I want to) avoid it. – Antoniossss Jan 08 '14 at 12:48
  • No in my answer only one multiton object is created per key. – assylias Jan 08 '14 at 12:56
  • 2
    @assylias apparently OP doesn't mind to populate the map in the beginning, so it's a simpler problem. each entry is lazily initialized though. double-checked-locking is perfectly fine here. – ZhongYu Jan 08 '14 at 13:06
  • @drorb double check locking is NOT broken and has been fixed since Java 5 by use of the volatile keyword. – Tim B Jan 08 '14 at 13:43
  • @TimB you are correct. I was referring to the code shown in the question – Dror Bereznitsky Jan 08 '14 at 14:02
  • @drorb That's not what you said though, your comment says that all double check locking is broken... – Tim B Jan 08 '14 at 14:03
  • The elegant solution is just Guava's `CacheBuilder`. It does exactly what you want behind the scenes in terms of lazy initialization. – BeeOnRope Jan 09 '14 at 03:12

1 Answers1

5

Yes is the right way well known as double check locking read also

http://en.wikipedia.org/wiki/Double-checked_locking

there is a little problem with "someObject.isInitialized()": "someObject" must be volatile in java 6.

 private volatile SomeObject someObject= null;

Read this article it is very usefull

http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Tim B
  • 40,716
  • 16
  • 83
  • 128
venergiac
  • 7,469
  • 2
  • 48
  • 70
  • I didn't know there is a pattern for that. Thank you. As for code - this is the case where `someObject` is a field. What about local objects? Eg. object retrieved from common map? – Antoniossss Jan 08 '14 at 12:51
  • please: post the code: how the someObject is retrieved? – venergiac Jan 08 '14 at 12:53
  • `someObject=someMap.get(someKey);` I have read the article you provided on wiki and it explains how to use volatile field for local variables indeed. – Antoniossss Jan 08 '14 at 12:59
  • the issue is due to thread optimization: a thread can cache in local stack the value of the var. It is used for local field: I think your local field is "someMap". Consider also the class "ConcurrentMap" and "ReadWriteLock". – venergiac Jan 08 '14 at 13:07