1

I have a code from which I am trying to get the instance of my class as I have written a wrapper around java.util.logging.Logger.

Below is the snippet of code in my ClientLogger class -

private static final Map<Class<?>, ClientLogger> s_classLoggers = new ConcurrentHashMap<Class<?>, ClientLogger>();

final private Logger m_logger;

private ClientLogger(final Class<?> caller) {
    m_logger = Logger.getInstance(caller);
}   

public static ClientLogger getInstance(final Class<?> klass) {
    final ClientLogger result;

    if (s_classLoggers.containsKey(klass)) {
        result = s_classLoggers.get(klass);
    } else {
        result = new ClientLogger(klass);
        s_classLoggers.put(klass, result);
    }

    return result;
}

And this is the way I am initializing it in my other classes where I need to use my above logger -

private static final ClientLogger s_logger = ClientLogger.getInstance(TestLogger.class);

Now when I am running my static analysis tool, it is complaining as in my ClientLogger class -

Non-atomic use of check/put on this line s_classLoggers.put(klass, result);

So I modified my above code like this to make it thread safe -

private static final ConcurrentHashMap<Class<?>, ClientLogger> s_classLoggers = new ConcurrentHashMap<Class<?>, ClientLogger>();

public static ClientLogger getInstance(final Class<?> klass) {
    ClientLogger result;

    result = s_classLoggers.putIfAbsent(klass, new ClientLogger(klass));
    // is below line thread safe and efficient?
    if (result == null) {
        result = new ClientLogger(klass);
    }       

    return result;
}

Below is the way I will be initializing it to get my logger instance -

private static final ClientLogger s_logger = ClientLogger.getInstance(TestLogger.class);

So is my above code thread safe? I am doing result == null check since for the first time, it won't be there in the map so I need to make a new value of it and because of that, I need to remove the final modifier of result.

john
  • 11,311
  • 40
  • 131
  • 251
  • Think about whether it's efficient. This creates a ClientLogger for each invocation of getInstance, even if not absent. Also think about whether it makes sense. How can the == null case ever happen, and if it does, why would it help to initialize result with a local variable that is not stored in the map? – Simon Fischer Sep 30 '14 at 16:35
  • @SimonFischer I think you are right but for null case, it will happen for the first time initialization when I am trying to get the instance of logger, it was coming as null since putIfAbsent will return null if it is not there. Is there any better way of doing this? – john Sep 30 '14 at 16:38
  • You are right, I was going to correct myself about the null case the very second your response came in :-) But it is still pointless to assign result a new instance of ClientLogger in that case and not have it added to the map, because in that case you are returning something different than what the map holds. In case there was no previous value in the map you want to assign the newly created instance to result. But even then, you would want to avoid creating the new ClientLogger() in case there is an existing one. – Simon Fischer Sep 30 '14 at 16:44
  • Related: http://stackoverflow.com/questions/11126866/thread-safe-multitons-in-java – assylias Sep 30 '14 at 16:46
  • The linked solution is good since it gets before it decides to create a new instance. – Simon Fischer Sep 30 '14 at 16:49
  • @assylias Thanks for the link. Can you provide an example so that I understand better what has to be done? – john Sep 30 '14 at 16:53

1 Answers1

3

What you need is Java 8's

s_classLoggers.computeIfAbsent(klass, ClientLogger::new);

This will only create the object if it really has to.

Note, the ClientLogger::new is amazing short hand as it is short for k -> new ClientLogger(k) which is short for

new Function<Class<?>, ClientLogger>() {
    public ClientLogger apply(Class<?> k) {
         return new ClientLogger(k);
    }
}

and the lambda doesn't even generate a class at compile time, though the JVM can (and does in Java 8) create a class at runtime.

Otherwise you might find using a write lock is safer.

public static ClientLogger getInstance(final Class<?> klass) {
    ClientLogger result = s_classLoggers.get(klass);
    if (result != null) 
        return result; // fast path

    // slow, but rare path
    synchronized (s_classLoggers) {
        result = s_classLoggers.get(klass);
        if (result == null)
            s_classLoggers.put(klass, result = new ClientLogger(klass));
    }
    return result;
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Unfortunately, I don't have Java 8 as of now. Are there any other ways which we can do it in Java 7? – john Sep 30 '14 at 16:51
  • @user2809564 I have updated my answer. Locks are not bad if they are rarely used. I am assuming your cache has a high enough hit rate that it rarely adds loggers once the code is warm. – Peter Lawrey Sep 30 '14 at 16:53
  • Yes once the code is warm, it should be cache, when I tried your suggestion, it gave me error on `synchronised`? may be something got missed there. – john Sep 30 '14 at 16:56
  • @user2809564 yes, in the US and Java it's `synchronized` not `synchronised` as it is in UK English. – Peter Lawrey Sep 30 '14 at 16:57
  • Yeah I was able to correct that myself. One last question, assylias pointed me to a [link](http://stackoverflow.com/questions/11126866/thread-safe-multitons-in-java) which doesn't used synchronized. Do you think that might be other solution as well in my case? – john Sep 30 '14 at 17:01
  • @user2809564 you could, but synchronized is often cheaper than creating a new object and I assume ClientLogger is non-trivial. So you save a lock, which is rarely called, but could be creating objects which are discarded (although again there shouldn't be many) I prefer the lock as it's simpler to understand exactly what it is going to do. – Peter Lawrey Sep 30 '14 at 17:05
  • Thanks Peter, make sense now, I will go with this solution now. How we can say that synchronized is cheaper than creating a new object? If you can explain me then I will learn something new today. – john Sep 30 '14 at 19:01