[FIXED] How to properly use a "lock" variable for synchronization in a Serializable class?

Issue

I am trying to figure out how to avoid an NPE in the the code below. I am unable to purposely reproduce the error, but every once in a while I get an NPE at the line 40 synchronized(lock) {. My guess is that it's happening after a serialization/deserialization process - but that is just a guess.

My IDE gives me a compile "tip" that says synchronization on a non-final variable (lock), but to be quite honest, I'm not as familiar with synchronized code blocks and how a serializable class affects final variables.

As an FYI, the code below was copied/modified from a Struts Wiki: https://cwiki.apache.org/confluence/display/WW/HibernateAndSpringEnabledExecuteAndWaitInterceptor (towards the bottom of the page in the comments).

import com.opensymphony.xwork2.ActionInvocation;
import org.apache.struts2.interceptor.BackgroundProcess;
import org.springframework.orm.jpa.EntityManagerFactoryUtils;
import org.springframework.orm.jpa.EntityManagerHolder;
import org.springframework.transaction.support.TransactionSynchronizationManager;

import javax.persistence.EntityManager;
import javax.persistence.EntityManagerFactory;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.Serializable;

public class OpenSessionBackgroundProcess extends BackgroundProcess implements Serializable {

    private static final long serialVersionUID = 3884464561311686443L;

    private final transient EntityManagerFactory entityManagerFactory;

    // used for synchronization
    protected boolean initializationComplete;
    private transient Object lock = new Object();

    public OpenSessionBackgroundProcess(String name, ActionInvocation invocation, int threadPriority, EntityManagerFactory entityManagerFactory) {
        super(name, invocation, threadPriority);
        this.entityManagerFactory = entityManagerFactory;
        initializationComplete = true;
        synchronized (lock) {
            lock.notify();
        }
    }

    protected void beforeInvocation() throws Exception {
        while (!initializationComplete) {
            try {
                synchronized (lock) {  // <----- NPE HERE
                    lock.wait(100);
                }
            } catch (InterruptedException e) {
                // behavior ignores cause of re-awakening.
            }
        }
        EntityManager em = entityManagerFactory.createEntityManager();
        TransactionSynchronizationManager.bindResource(entityManagerFactory, new EntityManagerHolder(em));
        super.beforeInvocation();
    }

    protected void afterInvocation() throws Exception {
        super.afterInvocation();
        EntityManagerHolder emHolder = (EntityManagerHolder)
        TransactionSynchronizationManager.unbindResource(entityManagerFactory);
        EntityManagerFactoryUtils.closeEntityManager(emHolder.getEntityManager());
    }

    /**
     * Override default readObject() method when deserializing
     *
     * @param serialized the serialized object
     */
    private void readObject(ObjectInputStream serialized) throws IOException, ClassNotFoundException {
        serialized.defaultReadObject();
        lock = new Object();
    }
}

Solution

The code in the question and the linked wiki page has an unresolvable data race condition. Unfortunately, the constructor of BackgroundProcess allows a reference to this to escape the constructor by starting a new thread that calls beforeInvocation. Because this call can occur before the constructor of the child class completes, it is not safe to extend BackgroundProcess.

This code attempts to handle the race condition on entityManagerFactory through use of the lock object, initializationComplete flag, and wait/notify usage. However, this only moves the race condition from entityManagerFactory to lock, as Java only initialises the fields of subclasses after the parent class constructor completes. This is true regardless of whether the field is initialised inline or in the constructor.

You can find more details about this problem in the excellent accepted answer to the question Java: reference escape.

My best advice is to avoid the use of BackgroundProcess and find another way to solve the original problem.

Answered By - Tim Moore
Answer Checked By - Marilyn (JavaFixing Volunteer)