Page tree
Skip to end of metadata
Go to start of metadata

In extension of my work on refactoring the HarvestScheduler assignment in the unit test class, which I described in the More informative naming, less inline comments thread, I've struggled with the instantiation, assignment and cleanup of the HarvestSchedule'er. It has finally dawned on me that the the problem is 2 conflicting characteristics of the HarvestScheduler:

  1. The class is meant to be a Singleton . This means that only one instance of the class should exist a one time, which the Singleton pattern enforces by hiding the possibility of construction/creating instances from users of the class.
  2. The class extends the CleanupIF: This interface implies that it is possible to invoke this operation when closing down the class. This works fine in normal class, where the operation is symmetric to the construction of the class. Eg. an objects life cycle is: create -> living -> cleanup.

The conflict between the 2 aspects of the HarvestScheduler arises because a Singleton pattern hides the object creation control from its API control, where the CleanupIF is logically a functionality associated with the destruction of a object.

The problem can be seen in the follow HarvestScheduler code:

public void cleanup() {
  hsmon = null;
  instance = null;
}

This code enables a user to create multiple instances of the HarvestScheduler as simple as this:

HarvestScheduler h1 = HarvestScheduler.getInstance();
h1.cleanup();
HarvestScheduler h2 = HarvestScheduler.getInstance();

This means the that the contract for the Singleton-pattern has been broken. It also means that application behaviour will now depend on when the created HarvestScheduler instances are garbage collect.

What is even worse is that this is used in the unit test to emulate contruction of a new HarvestScheduler instance in each test case. Eg.

public class HarvestSchedulerTester extends TestCase {
...
public void tearDown() {
if (harvestScheduler \!= null) {
            harvestScheduler.close();
        }
...
}

public void testGetHoursPassedSince() throws Exception {
        harvestScheduler = submitNewJobsAndGetSchedulerInstance();
  }

Note: The HarvestSchedulers close call delegated to the Cleanup method.

Conclusion: The CleanupIF should never be used on Singletonic classes as this leads to complicated and brittle code, and may lead to inconsistent application behavior. A solution could be to split the HarvestScheduler into a plain thread execution scheduler singleton, and a 'JobManager' class containing the business logicextending the CleanupIF. Another solution might be to replace the singleton pattern with a Inversion-of-control pattern, delegation the creation control to a class/container external to the HarvestScheduler and its users.

  • No labels

3 Comments

  1. I have started to move the HarvestScheduler lifecycle control to a new HarvestJobManager class and remove the singleton aspect of the HarvestScheduler. The HarvestJobManager is also a first step towards creating the HarvestSchedulerApplication mentioned in 1963 Make a HarvestSchedulerApplication that runs the HarvestScheduler.

  2. You may want to share your ideas with Nicolas Giraud, as he also have some ideas for changing the scheduler.

    1. Good idea.

      As I mentioned earlier I'm planning on creating a sort of pre-commit review to validate the refactoring path I'm taking, before I put any of the major changes into svn. I'll see if I can't add Nicolas to the Crucible pre-commit review (if I'm able to create one satisfactory).