Using Synchronization while Logging

Question

In my application I have written my own Logging utility using Java.Util.Logging

import java.io.IOException;
import java.io.InputStream;
import java.util.Properties;
import java.util.logging.FileHandler;
import java.util.logging.Level;
import java.util.logging.SimpleFormatter;



public class Logger {

    public final static String PROPERTIES_FILE = "Logger.properties";
    private static java.util.logging.Logger logger = null;

    private static void initialize() {
        try {
            logger = java.util.logging.Logger.getLogger(Logger.class.getName());
            FileHandler fh = new FileHandler("D:\\MyLogFile.log", true);
            logger.addHandler(fh);
            logger.setLevel(Level.ALL);
            SimpleFormatter formatter = new SimpleFormatter();
            fh.setFormatter(formatter);
            logger.log(Level.INFO, "Test Message Logged");

        }
        catch (IOException e) {
          System.out.println("Warning: Unable to read properties file " +
                             PROPERTIES_FILE );
        }   
      }

    public static synchronized java.util.logging.Logger getLogger(String name)
    {
        if(logger==null)
        {
        Logger.initialize();
        }
        logger.getLogger(name);
        return logger;
    }


}

Do I need to use Synchronization for getLogger method? Please give your comments. (This code is running in Multi-threaded environment)

Answer 1

I agree with other commenters that lazy initialization doesn't seem necessary here. The simplest way to initialize the logger variable is in a static initializer, which is guaranteed to only execute once at class loading time:

public class Logger {

    public final static String PROPERTIES_FILE = "Logger.properties";
    private static java.util.logging.Logger logger = null;

    private static void initialize() {
        try {
            logger = java.util.logging.Logger.getLogger(Logger.class.getName());
            FileHandler fh = new FileHandler("D:\\MyLogFile.log", true);
            logger.addHandler(fh);
            logger.setLevel(Level.ALL);
            SimpleFormatter formatter = new SimpleFormatter();
            fh.setFormatter(formatter);
            logger.log(Level.INFO, "Test Message Logged");

        }
        catch (IOException e) {
          System.out.println("Warning: Unable to read properties file " +
                             PROPERTIES_FILE );
        }   
    }

    static {
        initialize();
    }

    public static java.util.logging.Logger getLogger(String name)
    {
        logger.getLogger(name);
        return logger;
    }


}

However, You can avoid most synchronization with double-checked locking.

public class Logger {

    // note: volatile is required
    private volatile static java.util.logging.Logger logger = null;

    //... 

    public static java.util.logging.Logger getLogger(String name)
    {
        if(logger==null)
        {
           synchronized(Logger.class) 
           {
              if(logger == null)
                Logger.initialize();
              }
           }
        }
        logger.getLogger(name);
        return logger;
    }
}

In fact, in your case I think you can avoid synchronization altogether if you rewrite your initialize function so that it completely configures the logger in an local variable prior to assigning it to the (volatile) class variable:

private volatile static java.util.logging.Logger logger = null;
private static void initialize() {
    try {
        Logger logger = java.util.logging.Logger.getLogger(Logger.class.getName());
        FileHandler fh = new FileHandler("D:\\MyLogFile.log", true);
        logger.addHandler(fh);
        logger.setLevel(Level.ALL);
        SimpleFormatter formatter = new SimpleFormatter();
        fh.setFormatter(formatter);
        logger.log(Level.INFO, "Test Message Logged");

        Logger.logger = logger;
    }
    catch (IOException e) {
      System.out.println("Warning: Unable to read properties file " +
                         PROPERTIES_FILE );
    }   

    public static java.util.logging.Logger getLogger(String name)
    {
        if(logger==null)
        {
        Logger.initialize();
        }
        logger.getLogger(name);
        return logger;
    }
}

This has a potential to have initialize() execute several times, but I don't think you care, so long that every getLogger invocation will have a valid logger instance, even if that instance varies.

Answer 2

I wouldn't recommend it, because you'll be incurring the overhead of synchronization on every call to getLogger() when you only need it once for initialization. I'd do this instead:

private static boolean initialized = false;

private static synchronized void initialize() {
    try {
       if(!initialized)
       {            
        // Do initialization
        initialized = true;
       }

    }
    catch (IOException e) {
      System.out.println("Warning: Unable to read properties file " +
                         PROPERTIES_FILE );
    }   
  }

public static java.util.logging.Logger getLogger(String name)
{
    if(logger==null)
    {
    Logger.initialize();
    }
    logger.getLogger(name);
    return logger;
}

This way, if a second thread comes in while the first thread is still in initialize(), the second thread will block because of synchronized on initialize(). Once the first thread is done with initialization, the second thread proceeds, sees that initialized is true, doesn't redo the initialization, and moves on.

Answer 3

Yes, you need synchronized there. First to avoid calling initialize() several times and secondly to make logger change visible to other threads.

This begs a question: why can't you eagerly initialize() logger and avoid synchronization altogether?