Volatile variable assignment in a synchronized block results in null

Question

here's a class:

public class Refreshable<T> {
    private final Object lock = new Object();

    private final Supplier<Object>[] dynamicSettings;
    private final Supplier<T> constructor;

    private volatile Object[] instanceKey;
    private volatile T instance;

    @SafeVarargs
    public Refreshable(Supplier<T> constructor, Supplier<Object>... dynamicSettings) {
        this.constructor = constructor;
        this.dynamicSettings = dynamicSettings;
    }

    public T getInstance() {
        Object[] currentKey = getCurrentKey();
        if (!Arrays.equals(currentKey, instanceKey)) {
            synchronized (lock) {
                if (!Arrays.equals(currentKey, instanceKey)) {
                    instanceKey = currentKey;
                    instance = constructor.get();
                }
            }
        }
        return instance;
    }

    private Object[] getCurrentKey() {
        return Arrays.stream(dynamicSettings)
                .map(Supplier::get)
                .toArray();
    }
}

The idea is in the name: return new instance when values of the parameters relevant for its creation have changed. constructor is always a lambda wrapping a constructor - it can't return null. It can throw an exception, though Used in our project like this:

new Refreshable<Service>(
    () -> new Service(settings.getParam1(), settings.getParam2()),
    settings::getParam1,
    settings::getParam1);

where parameter values returned by settings can change over time.

Long story short, under some load testing getInstance() returned null to several threads on first invocation (10 or more of them in the same second), at a line like this: serviceRefreshable.getInstance().someMethod(). So it's either the refreshable that was null, or the result of getInstance() and the fact that the error didn't repeat later in the run suggests the latter because refreshable is only assigned once (it's a singleton bean).

I understand that the constructor can throw an exception and prevent instance from being assigned, but it's not in the log (and NPE is).

Arrays.equals() here can't return true unexpectedly, because instanceKey is null initially and getCurrentKey() can't return null. All dynamicSettings suppliers are lambdas wrapping this invocation:

private <T> T getValue(String key, Class<T> type) {
    Object value = properties.get(key);
    if (value == null) {
        throw new ConfigurationException("No such setting: " + key);
    }
    if (!type.isInstance(value)) {
        throw new ConfigurationException("Wrong setting type" + key);
    }
    return (T) value;
}

How is this possible?

I also understand one can safeguard against this by wrapping instance and instanceKey in one object and reducing 2 assignments to one to make them atomic, as well as checking for instance == null along with comparing arrays. What I don't understand is where the hell did the exception that caused constructor to fail go :)

I even edited the production code to throw a RuntimeException in a constructor lambda supplied to one of these Refreshables and sure enough, I saw that exception in the log instead of a NPE. Wasn't under any kind of concurrent load, though

Answer 1

Long story short, under some load testing getInstance() returned null to several threads on first invocation

As I mentioned in my comment, there is a race condition where instanceKey has been assigned inside the synchronized block but instance has not yet been assigned and may be null. So after the one thread assigns the key, other threads will test the key, find it not null and equals and continue to return a null instance.

I think the creation of a class which holds both values is in order. I initially implemented this inside of an AtomicReference but I think just using volatile will get the job done. Something like:

private volatile InstanceInfo<T> instanceInfo;
...
public T getInstance() {
    // we store this in a non-volatile to only touch the volatile field once and to
    // ensure we have a consistent view of the instanceInfo value
    InstanceInfo<T> localInstanceInfo = instanceInfo;
    if (localInstanceInfo == null || !Arrays.equals(currentKey, localInstanceInfo.key)) {
       localInstanceInfo = new InstanceInfo<>(currentKey, constructor.get());
       instanceInfo = localInstanceInfo;
    }
    return localInstanceInfo.instance;
}
...
private static class InstanceInfo<T> {
   final Object[] key;
   final T instance; 
   public InstanceInfo(Object[] key, T instance) {
       this.key = key;
       this.instance = instance;
   }
}