Is log4j not thread safe after all?

Question

(This is on SLES11, Java 7, Tomcat 6, log4j-1.2.16)

We use log4j to write different things to different logfiles. I've inherited this code, so for good or bad the general structure is here to stay for the time being.

The logger will create two logfiles: main.log and stats.log. A certain stats message is logged to both loggers via separate calls (you'll see below) and a whole bunch of other things is logged to the main log.

So all through our code you'll see things like Log.logMain(someMessageToLog);. In one single place in our code (which is executed by multiple threads) there is the following:

String statsMessage = createStatsMessage();
Log.logMain(statsMessage);
Log.logStats(statsMessage);

The name of the main logger is main, the name of the stats logger is stats. The problem is that sometimes under heavy load we see lines in main.log that have the string stats INFO in it. Everything in main.log should only have main INFO in it because that is the only logger logging to that file, plus we see mixed output in some lines. This seems like a thread-safety issue, but the log4j docs say log4j is thread-safe. Here's an example of what I mean:

2012-03-21 16:01:34,7742012-03-21 16:01:34,774| | stats main INFO   [INFO  http-8080-18]:  [message redacted]. 
2012-03-21 16:01:36,380| main 2012-03-21 16:01:36,380| INFO   [stats INFO  http-8080-15]: [message redacted]. 
2012-03-21 16:01:37,465| main INFO  2012-03-21 16:01:37,465 [| stats http-8080-1]: [message redacted]. 

Here's the Log class (stripped down to only show the loggers in question -- there are actually a bunch of other loggers in it, all set up similarly to these):

import org.apache.log4j.*;

import java.io.IOException;

final public class Log
{
    private static final String LOG_IDENTIFIER_MAINLOG = "main";
    private static final String LOG_IDENTIFIER_STATSLOG = "stats";

    private static final String MAIN_FILENAME = "/var/log/app_main.log";
    private static final String STATS_FILENAME = "/var/log/app_stats.log";

    private static final int BACKUP_INDEX = 40;
    private static final String BACKUP_SIZE = "10MB";

    private static final PatternLayout COMMON_LAYOUT =
        new PatternLayout("%d| %c %-6p [%t]: %m.%n");

    private static Logger mainLogger;
    private static Logger statsLogger;

    public static void init() {
        init(MAIN_FILENAME, STATS_FILENAME);
    }

    public static void init(String mainLogFilename,
                            String statsLogFilename) {
        mainLogger = initializeMainLogger(mainLogFilename);
        statsLogger = initializeStatsLogger(statsLogFilename);
    }

    public static void logMain(String message) {
        if (mainLogger != null) {
            mainLogger.info(message);
        }
    }

    public static void logStats(String message) {
        if (statsLogger != null) {
            statsLogger.info(message);
        }
    }

    private static Logger getLogger(String loggerIdentifier) {
        Logger logger = Logger.getLogger(loggerIdentifier);
        logger.setAdditivity(false);
        return logger;
    }

    private static boolean addFileAppender(Logger logger,
                                           String logFilename,
                                           int    maxBackupIndex,
                                           String maxSize) {
        try {
            RollingFileAppender appender =
                new RollingFileAppender(COMMON_LAYOUT, logFilename);
            appender.setMaxBackupIndex(maxBackupIndex);
            appender.setMaxFileSize(maxSize);
            logger.addAppender(appender);
        }
        catch (IOException ex) {
            ex.printStackTrace();
            return false;
        }
        return true;
    }

    private static Logger initializeMainLogger(String filename) {
        Logger logger = getLogger(LOG_IDENTIFIER_MAINLOG);
        addFileAppender(logger, filename, BACKUP_INDEX, BACKUP_SIZE);
        logger.setLevel(Level.INFO);
        return logger;
    }

    private static Logger initializeStatsLogger(String filename) {
        Logger logger = getLogger(LOG_IDENTIFIER_STATSLOG);
        addFileAppender(logger, filename, BACKUP_INDEX, BACKUP_SIZE);
        logger.setLevel(Level.INFO);
        return logger;
    }

}

---

Update:

Here's a little program that (at least for me) will reproduce the problem with the above Log class:

final public class Stress
{
    public static void main(String[] args) throws Exception {
        if (args.length != 2) {
            Log.init();
        }
        else {
            Log.init(args[0], args[1]);
        }

        for (;;) {
            // I know Executors are preferred, but this
            // is a quick & dirty test program
            Thread t = new Thread(new TestLogging());
            t.start();
        }
    }

    private static final class TestLogging implements Runnable
    {
        private static int counter = 0;

        @Override
        public void run() {
            String msg = new StringBuilder("Count is: ")
                .append(counter++).toString();

            Log.logMain(msg);
            Log.logStats(msg);

            try {
                Thread.sleep(1);
            }
            catch (InterruptedException e) {
                Log.logMain(e.getMessage());
            }
        }
    }
}

And some sample output in the logs:

$ grep stats main.log    
2012-03-23 15:30:35,919| stats 2012-03-23 15:30:35,919| main INFO  INFO   [ [Thread-313037]: Thread-313036]: Count is: 312987.
2012-03-23 15:30:35,929| stats INFO   [Thread-313100]: Count is: 313050.
2012-03-23 15:30:35,937| stats INFO   [Thread-313168]: Count is: 313112.
2012-03-23 15:30:35,945| stats INFO   [Thread-313240]: Count is: 313190.
2012-03-23 15:30:35,946| stats INFO   [Thread-313251]: Count is: 313201.
2012-03-23 15:30:35,949| stats INFO   [2012-03-23 15:30:35,949| main INFO  Thread-313281]: Count is: 313231.
2012-03-23 15:30:35,954| stats INFO   [Thread-313331]: Count is: 313281.
2012-03-23 15:30:35,956| 2012-03-23 15:30:35,956stats | main INFOINFO   [   [Thread-313356]: Count is: 313306.
2012-03-23 15:30:35,9562012-03-23 15:30:35,956| main | INFO  stats  [INFOThread-313359]:   Count is: 313309.
2012-03-23 15:30:35,962| stats INFO  2012-03-23 15:30:35,962| main INFO   [Thread-313388]:  [Count is: 313338.

and

$ grep main stats.log
2012-03-23 15:30:35,913| 2012-03-23 15:30:35,913| main INFO   [Thread-312998]: Count is: 312948.
2012-03-23 15:30:35,915| main INFO   [Thread-313014]: Count is: 312964.
2012-03-23 15:30:35,919| stats 2012-03-23 15:30:35,919| main INFO  INFO   [ [Thread-313037]: Thread-313036]: Count is: 312987.
2012-03-23 15:30:35,931| main INFO   [Thread-313116]: Count is: 313066.
2012-03-23 15:30:35,947| main INFO   [2012-03-23 15:30:35,947Thread-313264]: | Count is: 313214.
2012-03-23 15:30:35,949| stats INFO   [2012-03-23 15:30:35,949| main INFO  Thread-313281]: Count is: 313231.
2012-03-23 15:30:35,956| 2012-03-23 15:30:35,956stats | main INFOINFO   [   [Thread-313356]: Count is: 313306.
2012-03-23 15:30:35,9562012-03-23 15:30:35,956| main | INFO  stats  [INFOThread-313359]:   Count is: 313309.
2012-03-23 15:30:35,962| stats INFO  2012-03-23 15:30:35,962| main INFO   [Thread-313388]:  [Count is: 313338.

For what it's worth, out of a 145516-line main.log file, "stats" showed up in it 2452 times. So it's not uncommon but it's not like it happens all the time, either (and of course this test is pretty extreme).

Answer 1

http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/PatternLayout.html

You're sharing the PatternLayout between the two appenders, which according to the above API link:

This code is known to have synchronization and other issues which are not present in org.apache.log4j.EnhancedPatternLayout. EnhancedPatternLayout should be used in preference to PatternLayout. EnhancedPatternLayout is distributed in the log4j extras companion.

So create a new PatternLayout for each appender