Is it bad idea to use getClass() for synchronization while initializing private static variable?

Question

So here is the problem. I have the following abstract class.

  public abstract class JAXBParser<T> {

  private static final Object lock = new Object();

  private static volatile JAXBContext context;

  private JAXBContext getContext() {
    if (context == null) {
      synchronized (getClass()) {
        if (context == null) {
          try {
            context = JAXBContext.newInstance(getJAXBClass());
          } catch (JAXBException e) {
            log.error("Couldn't create JAXB context", e);
          }
        }
      }
    }

    return context;
  }

  @SuppressWarnings("unchecked")
  public Optional<T> parse(File file) {
    log.debug(file.getAbsolutePath());
    try {
      JAXBContext context = getContext();
      Unmarshaller unmarshaller = context.createUnmarshaller();

      return Optional.ofNullable((T) unmarshaller.unmarshal(file));
    } catch (JAXBException e) {
      log.error(unmarshallerError(file.getAbsolutePath()), e);
    }

    return Optional.empty();
  }

  protected abstract Class getJAXBClass();
}

The idea is that I'll have couple of parsers with same behaviour so I can use this to follow the DRY principle (and to play with inheritance, multithreading, synchronization etc.). Also in this question it is stated that JAXBContext initialization is heavy, so it would be good idea to initialize it once. So I decided to initialize it once per child class using double-checked locking.

The question is the following:

Is it bad idea to use getClass() for synchronization? Would it be better to use lock object and why?

You will need to understand, that since class is available to basically anyone, if you synchronize on class object, you could be competing with everything else that also synchronizes on the same class object, and those things will not necessarily have anything to do with `getContext()` method calls - and your lock on class will make them wait for you to finish. Or make you wait for them to finish. If you are sure there's noone else who might compete for the lock on class object, or you don't mind you or them waiting - then it's totally OK. — M. Prokhorov, Mar 22 '19 at 11:34

score 2 · Answer 1 · answered Mar 22 '19 at 11:41

An instance of Class is also an Object, it's perfectly valid to synchronize on it.

However, as there is only one instance, you won't be able to differentiate between different (unrelated) synchronized sections, should you need more than one in the future.

That's also the problem with declaring methods synchronized - no only do they synchronize on themselves, but also on all their siblings and all other external code that might synchronize on the object, which isn't always obvious.

score 1 · Accepted Answer · answered Mar 22 '19 at 11:51

Locking on publicly visible objects has one big problem: you can't reason about the class's behaviour in isolation.

The reason being that other parts of the code may also choose to synchronise on your class object, potentially causing your getContext() method to block indefinitely.

Now as you might imagine, in practice this is rarely a problem^*. But when it is (and sometimes it is), it's an absolute nightmare to debug and identify. So given how easy it is to just define a private lock object and synchronise on that, I'd go with it.

^*Assuming no malicious code is present, that is. If you really want to, you can do really nasty things by synchronising on unwittingly shared objects, like this example I created, which "invisibly" connects an input and an output stream.

score 0 · Answer 3 · answered Feb 07 '23 at 23:38

Is it bad idea to use getClass() for synchronization while initializing private static variable?

It can be correct but there are usually alternative ways to write the code with less smell.

From SpotBugs: WL: Synchronization on getClass rather than class literal (WL_USING_GETCLASS_RATHER_THAN_CLASS_LITERAL)

This instance method synchronizes on this.getClass(). If this class is subclassed, subclasses will synchronize on the class object for the subclass, which isn't likely what was intended.

What is usually misunderstood about this rule is that code is correct when you are locking on the class that declares the field or there is no way to get a subclass returned from getClass like a final class or anonymous class. In those cases the lock returned is known to be the same and therefore safe in that regard. This pattern always suffers from being able to lock public object but that is a different risk.

So I decided to initialize it once per child class using double-checked locking.

That is not what is happening per your given code:

private static volatile JAXBContext context;

The declaring class of this field is JAXBParser.class and all subclasses are using that single field. For example, say you have classes JAXBParser1 extends JAXBParser and JAXBParser2 extends JAXBParser. Then synchronized (getClass()) is either going to lock JAXBParser1.class or JAXBParser2.class. Which would mean that contention only happens within each parser subclass. From your description that seems like it is what you intended. However, because the context field is not declared in each subclass you could have 2 threads (one from JAXBParser1 and one from JAXBParser2) racing to set the JAXBParser::context field in the root JAXBParser class without coordination to a shared location. Which means that you could even be sharing across the sibling parsers which would be a bug.

An alternative approach would be to use java.lang.ClassValue to create a context unique to each getJAXBClass() within your JAXBParser hierarchy:

public abstract class JAXBParser<T> {

    private static final ClassValue<JAXBContext> context = new ClassValue<>() {
        protected JAXBContext computeValue(Class<?> type) {
            try {
                return JAXBContext.newInstance(type);
            } catch (JAXBException e) {
                throw new UndeclaredThrowableException(e);
            }
        }
    };

    private JAXBContext getContext() {
        return context.get(getJAXBClass());
    }
}

In this example this would only share a context if the parser is using the same JAXB class.

Is it bad idea to use getClass() for synchronization while initializing private static variable?

3 Answers3