issue with java synchronized

Question

I got a question concerning simultaneously access. I'm trying to implement my own scheduler. What I'm having is a triggerClass which is called by server (actually its a quartz-job, but thats not neccessary to stay so). That Trigger registers his desire to run to a Control Center which creates a Controller-class for that special job and stores it in a queue. As you can see, the Control Center is a singleton class and uses some synchronized methods:

getInstanceController(...) // gets the ( already existing ) controller for the JobInstance**

registerInstance(...) // registers a firing desire for a job (note: on job could fire multiple times using the same Controller instance)**

tryStart(...) // looks if a job can be executed now. E.g. does nothing if another controller is already running. 

Note: tryStart will be executed each time registerInstance ist called and its called at the end of startNextJobRun() when the current running job finnishes.

so. my first question is: Am i assuming right that the synchronous modifier for the method getInstanceController(...) is super fluid? Its never called from outside JobInstanceControlCenter (I actually run into a N*ullPointerException* because in the tryStartMethod he pulled all three controllers out of the queue and overwrote the entries in the activeInstanceControllers-map. When the jobs came back they tried to find their controller with getInstanceController(...) which were already gone.)

my next question is:

I tried to apply 3 JobTriggers ( each in a separate Thread ) which were designed to trigger at the time my server was start Up. ( I registered a mBean to the Server which creates a quartz scheduler and registers these jobs on it as running now ) When i did this, all three threads ran simultaneously ending up in running the synchronized methods simultaneously which was not supposed to happen.

I tried to apply these JobTriggers to start at the exact same nominal time, but 8 seconds in the future and the result was as expected. The 3 triggers starter simultaneously. The first thread locked the synchronized methods and the others waited until the first was finished.

Any suggestions what was wrong here of if i misunderstood something about synchronized?

public class JobTrigger
{

    public void execute(SomeContext context)
    {
        JobInstanceControlCenter.getInstance().registerInstance(context);
    }

}




public class JobInstanceControlCenter
{
    private static JobInstanceControlCenter _instance = null;

    private final Map<JobType, PriorityQueue<JobInstanceController>> queuedInstanceControllers;
    private final Map<JobType, JobInstanceController> activeInstanceControllers;

    private JobInstanceControlCenter()
    {
        // prevent instantiation
        queuedInstanceControllers = Collections.synchronizedMap(new HashMap<JobType, PriorityQueue<JobInstanceController>>());
        activeInstanceControllers = Collections.synchronizedMap(new HashMap<JobType, JobInstanceController>());
    }

    static final JobInstanceControlCenter getInstance()
    {
        if (_instance == null)
            _instance = new JobInstanceControlCenter();
        return _instance;
    }


    synchronized JobInstanceController getInstanceController(JobInstanceKey JobInstanceKey)
    {
        /**
         * momentary active Controller
         */
        if (activeInstanceControllers.get(JobInstanceKey.getJobType()) != null
                && activeInstanceControllers.get(JobInstanceKey.getJobType()).getJobInstanceKey().equals(JobInstanceKey))
        {
            if (JobScheduler.getInstance().getSchedulerVO().getIsEnabledLogging())
                logger.info("reuse active JobInstanceController: " + JobInstanceKey);
            return activeInstanceControllers.get(JobInstanceKey.getJobType());
        }

        /**
         * momentary queyed Controllers
         */
        PriorityQueue<JobInstanceController> queue = queuedInstanceControllers.get(JobInstanceKey.getJobType());
        if (queue != null && !queue.isEmpty())
            for (JobInstanceController controller : queue)
                if (controller.getJobInstanceKey().equals(JobInstanceKey))
                {
                    if (JobScheduler.getInstance().getSchedulerVO().getIsEnabledLogging())
                        logger.info("get next queyed JobInstanceController: " + JobInstanceKey);
                    return controller;
                }

        return null;
    }

    synchronized void registerInstance(JobExecutionContext context)
    {
        JobInstanceKey JobInstanceKey = JobInstanceKey.getJobInstanceKey(context);
        PriorityQueue<JobInstanceController> queue = queuedInstanceControllers.get(JobInstanceKey.getJobType());

        JobInstanceController instanceController = getInstanceController(JobInstanceKey);
        if (instanceController == null)
        {
            instanceController = JobInstanceController.createInstance(JobInstanceKey);
            if (queue == null)
                queue = new PriorityQueue<JobInstanceController>();
            queue.offer(instanceController);
            queuedInstanceControllers.put(JobInstanceKey.getJobType(), queue);
        }


        instanceController.registerNewJobRun(context);

        tryStart(JobInstanceKey.getJobType());

    }

    synchronized void tryStart(JobType jobType)
    {
        try
        {
            JobInstanceController activeController = activeInstanceControllers.get(jobType);
            if (activeController != null)
            {
                if (activeController.getJobInstanceState() == JobInstanceState.RUNNING)
                {
                    if (JobScheduler.getInstance().getSchedulerVO().getIsEnabledLogging())
                        logger.info("Active Controller was already running. Job aborted.");
                    return; // will be reconsidered when running job finishes
                }
                // note: startNextJobRun() will run synchronous
                boolean hadAnotherRun = activeController.startNextJobRun();     // true if next Job started, false if Controller was empty.
                if (!hadAnotherRun)
                {
                    finishedJobInstanceControllers.put(activeController.getJobInstanceKey(), activeController);
                    activeController = null; // finish empty Controller
                }
            }
            if (activeController == null) //either ativeController had been initially null, or it was set due to beeing empty
            {
                activeController = queuedInstanceControllers.get(jobType).poll();
                activeInstanceControllers.put(jobType, activeController);
                boolean hadAnotherRun = activeController.startNextJobRun();
            }
        }
        catch(ControllerException e) // some homemade Exception thrown by activeController.startNextJobRun()
        {
            logger.error("", e);
        }


    }

}

Answer 1

It does not look like you have a good reason to lazily create the JobInstanceControlCenter singleton.

Just do

private static final JobInstanceControlCenter _instance = new JobInstanceControlCenter();

Then you don't have to worry about synchronized getters or attempting Double checked locking. Unless you are intentionally attempting to defer loading this is by far the simplement and error free singleton pattern.

Answer 2

Your singleton factory method is not synchronized, try:

static final synchronized JobInstanceControlCenter getInstance(){

You can also try something more advanced like double-check locking but please be careful with it as it can be tricky and will only work on more recent jdk's. Google can provide reams of info on double check locking.

What's happening now is that your threads are running concurrently and therefore each grabs its own instance of JobInstanceControlCenter. Since your other synchronized methods are instance methods, they're synchronized on the instance of JobInstanceControlCenter.