Why java.util.ScheduledThreadPoolExecutor violates the Open-Close-Principle

In my last post I described the concept of the NotifyingExecutorService. In short, it is a ThreadPoolExecutor (TPE)-extension that returns Future-objects one can listen to. I thought it was a good idea to adopt that pattern to java’s ScheduledThreadPoolExecutor (STPE) which allows the execution of delayed and periodic tasks. However, I failed hard.

The STPE extends the normal ThreadPoolExecutor and adds several schedule()-methods to execute tasks in the future and/or allow repetition of tasks with a fixed frequence. Whenever a schedule-method gets called it creates a new ScheduledFutureTask – object that holds the incoming task and some timing parameters. The method then calls decorateTask() and adds the resulting ScheduledFutureTask to the work queue:

public  ScheduledFuture schedule(Callable callable, long delay, TimeUnit unit) {
        if (callable == null || unit == null)
            throw new NullPointerException();
        if (delay < 0) delay = 0;
        long triggerTime = now() + unit.toNanos(delay);
        RunnableScheduledFuture t = decorateTask(callable,
            new ScheduledFutureTask(callable, triggerTime));
        delayedExecute(t);
        return t;
    }

protected  RunnableScheduledFuture decorateTask(
        Callable callable, RunnableScheduledFuture task) {
        return task;
    }

As you can see, decorateTask() returns the incoming task by default. Javadoc and I thought this was a good place to install the NotifyingFuture-concept:

• Decorate the incoming task.
• Add register-methods.
• Notify listener when certain methods are called.

So I created a class that decorates the original task and overwrites the run()-method in the following manner:

@Override
    public void run() {
    	/*
    	 * execute task
    	 */
        this.decorate.run();
        /*
         * notify listener
         */
        notifyListener();
    }

Sadly, this approach works only for one-shot-executions. Periodic tasks won’t notify their listener and the reason lies in the nature of the decorator pattern that is used by the STPE. See the following snippet of the delegated ScheduledFutureTask (which is a private class inside the ScheduledThreadPoolExecutor):

/**
         * Runs a periodic task.
         */
        private void runPeriodic() {
            boolean ok = ScheduledFutureTask.super.runAndReset();
            boolean down = isShutdown();
            // Reschedule if not cancelled and not shutdown or policy allows
            if (ok && (!down ||
                       (getContinueExistingPeriodicTasksAfterShutdownPolicy() &&
                        !isStopped()))) {
                long p = period;
                if (p > 0)
                    time += p;
                else
                    time = now() - p;
                ScheduledThreadPoolExecutor.super.getQueue().add(this);
            }
            // This might have been the final executed delayed
            // task.  Wake up threads to check.
            else if (down)
                interruptIdleWorkers();
        }

        /**
         * Overrides FutureTask version so as to reset/requeue if periodic.
         */
        public void run() {
            if (isPeriodic())
                runPeriodic();
            else
                ScheduledFutureTask.super.run();
        }

When the task is periodic, runPeriodic() is called which is the root of all evil. Can you spot the pitfall? If not, look closer to line 16.
When the STPE is not yet shut down, the task adds itself to the queue. Remeber that we decorated that task, we didn’t subclass it. That means although the initial task contained our NotifyingFutureTask-implementation, whenever a task is periodic it get’s rid of it’s enclosing deocrator on the second execution.

Now tell me: how bad is that? Why has Sun chosen to use two different approaches (newTaskFor vs decorateTask) inside one class? Who knows the answer?