The thread threat

February 14, 2003

Q: I have a class with a pair of static getter/setter methods that read and write a single static field. Do I need to make them synchronized for thread safety?

A:
To make the discussion concrete, let’s use this class as an example:

public class MaybeSafe
{
    public static int getFoo ()
    {
        return s_foo;
    }
    
    public static void setFoo (int foo)
    {
        s_foo = foo;
    }
    
    private static int s_foo;
} // End of class

If this API was intended to be accessed from multiple concurrent threads, it is manifestly not thread safe. This is true despite the fact that Java guarantees that updates to s_foo are atomic. If this is news to you, read on.

Consider the following scenario, with horizontal axis indicating actions by two threads and vertical axis indicating time:

        Thread1              Thread2
        ...                  MaybeSafe.setFoo(1);
        MaybeSafe.setFoo(2); ...
        ...                  MaybeSafe.getFoo();

Java does not guarantee that getFoo() in Thread2 will return 2 when the actions above complete, even though getFoo() occurs after setFoo(2) in Thread1. The two threads communicate via a globally shared static class field, but each thread is allowed to have its own memory view with independent copies of s_foo. Changes made by Thread1 to s_foo are not necessarily visible to Thread2 by the time getFoo() is called.

Two approaches ensure this API works as intended. One is to declare s_foo volatile. The only problem with this approach is that some JVMs do not implement volatile semantics correctly. Although the problem has been understood for some time, not all JVM vendors are likely to fix it yet because the relevant specification is still undergoing the Java Specification Request (JSR) revision process (see JSR 133 in Resources), and volatile‘s proposed semantics might end up stronger than in the original specification.

Thus, while volatile is a good option in theory, the only reliable way to fix the API in today’s practice is to declare both methods synchronized. In addition to being a mutual exclusion barrier (something that does not matter to MaybeSafe as long as the field reads and writes remain atomic), synchronized can ensure the desired thread safety model. If a Thread acquires an object monitor lock previously held by another Thread, then all memory view changes made by the latter become visible to the former. Thus, declaring both the setter and the getter synchronized will ensure that any thread reading s_foo will see the most recently set value.

Despite this common knowledge, even Sun’s JDK implementation (i.e., the reference implementation) continues to have thread safety bugs like the one above. As an example, here is a slightly simplified code extract from java.lang.System in Sun’s JDK 1.4.1 for Windows:

    private static Properties props;
    public static Properties getProperties() {
    return props;
    }
        
    public static void setProperties(Properties props) {
        if (props == null) {
            props = new Properties();
            initProperties(props);
        }
    System.props = props;
    }
    
    public static String getProperty(String key) {       
    if (key == null) {
        throw new NullPointerException("key can't be null");
    }
    if (key.equals("")) {
        throw new IllegalArgumentException("key can't be empty");
    }
    return props.getProperty(key);
    }

The setProperties()‘s intent is undoubtedly to have the new system properties available to all Threads in the JVM, not just the calling one. However, nothing ensures that in the code. No memory barriers either in the form of volatile or synchronized ensure that a caller of getProperty() will look inside the java.util.Properties instance installed by the last call to setProperties(). You can find more cases of the same problem in the JDK: java.net.Authenticator.setDefault()/requestPasswordAthentication(), java.sql.DriverManager.get/setLoginTimeout(), java.net.HttpURLConnection.get/setFollowRedirects(), and so on. java.util.Locale.get/setDefault() had the same problem in JDK 1.3 and changed to have a milder double-check locking problem in JDK 1.4 (see Note in Resources).

Although Java Memory Model (JMM) is undergoing reevaluation as we speak (see Resources), you can write correct code now and expect it to stay correct later. Incorrect code such as MaybeSafe will not be corrected retroactively by the new specification, because the memory model required for that would be too expensive to implement on many processor architectures.

The reason bugs such as the one illustrated above do not often show up at runtime is that they can only be seen on multiprocessor architectures with very aggressive memory models.

As a rule of thumb, whenever multiple threads share data, you must make sure the relevant threads exchange monitor locks to ensure consistent memory views. For Swing applications, where most of the action occurs on a single Abstract Windowing Toolkit (AWT) event thread, designing for thread safety is not as critical. For high-performance server applications, you can’t afford to ignore thread safety.

Source: www.infoworld.com