Making a class Thread-Safe

Question

Given:

public class TestSeven extends Thread {

private static int x;

public synchronized void doThings() {
    int current = x;
    current++;
    x = current;
}

public void run() {
    doThings();
  }
}

Which statement is true?

A. Compilation fails.

B. An exception is thrown at runtime.

C. Synchronizing the run() method would make the class thread-safe.

D. The data in variable "x" are protected from concurrent access problems.

E. Declaring the doThings() method as static would make the class thread-safe.

F. Wrapping the statements within doThings() in a synchronized(new Object()) { } block would make the class thread-safe.

isn't it enough to mark doThings() as synchronized in order to make that class Thread-safe ? i see that the correct answer is D but the Model answer of this question is E, But i don't understand why?

Answer 1

E. Declaring the doThings() method as static would make the class thread-safe.

That is kind of a tricky answer. The method is already synchronized, but on the instance, whereas the state is in a static field, i.e. on the class. Making it static synchronized is indeed the correct answer, because then it synchronizes on the class, not on a (meaningless) instance.

D. The data in variable "x" are protected from concurrent access problems.

private static int x;

This is a static variable. It is shared by all instances of the class, so synchronizing on individual instances is not helpful, in the same way as F would not be helpful, which synchronizes on a complete throw-away dummy object.

Answer 2

According to the language spec:

A synchronized method acquires a monitor (§17.1) before it executes.

For a class (static) method, the monitor associated with the Class object for the method's class is used.

For an instance method, the monitor associated with this (the object for which the method was invoked) is used.

This means that in the code you provided the synchronized keyword causes the method to acquire a lock on this before executing the body of the method. However, since x is static that doesn't ensure that the update to x will be atomic. (Another instance of the class could enter the synchronized region and do an update at the same time since they have different this values and thus different locks.)

However, declaring doStuff static will make all calls to the method acquire the same lock (the one on the Class) and thus would ensure mutual exclusion in the method body.

The specifications is really spelling out that:

class A {
    static synchronized void doSomething() {
        // ...
    }
}

is literally the same thing as

class A {
    static void doSomething() {
        synchronized(A.class) {
            // ...
        }
    }
}

Similarly:

class B {
    synchronized void doSomething() {
        // ...
    }
}

is literally the same thing as

class B {
    void doSomething() {
        synchronized (this) {
            // ...
        }
    }
}