Allow to track locking information on scheduled tasks

[alienisty]

#24560 breaks custom TaskSchedulers injected in the ScheduledTaskRegistrar, where those schedulers rely on contracts attached to the scheduled Runnable instance. Wrapping it in an OutcomeTrackingRunnable hides whatever contract the original instance implements.

In our case, for example, the Runnable would implement other interfaces to provide locking information for allowing exclusive execution of schedules in a clustered environment, so that only one node would execute at the time.
With this change, our clustered TaskScheduler cannot see those interfaces and the schedules will run in parallel on all nodes.

Considering that we moved away from using the ScheduledTaskRegistrar and we now use the TaskScheduler directly, but I thought it would be beneficial for the framework if it provided an implementation of the feature that augments the original Runnable instance rather than wrapping it, possibly through a dynamic proxy or other aspect oriented approach.

[terje2001]

We ran into the exact same compatibility issue, our implementation of TaskScheduler uses annotations to delegate runnables to different thread pools.

We used the following wildly ugly workaround to get the inner runnable:

private fun unwrapRunnable(runnable: Runnable): Runnable =
  if (runnable.javaClass.simpleName == "OutcomeTrackingRunnable") {
    runCatching {
      val field = runnable.javaClass.getDeclaredField("runnable")
      field.isAccessible = true
      field.get(runnable) as? Runnable ?: runnable
    }.getOrElse { runnable }
  } else {
    runnable
  }

Please fix so we can delete the above code :-)

Thanks!!

[bclozel]

Thanks for reaching out.

I believe that the core problem here is that we let developers believe that the Taskis merely a holder for a Runnable. Besides OutcomeTrackingRunnable, runnables can be wrapped with SchedulingAwareRunnable or SubscribingRunnable. The runnable you get from the task is the thing to run, not necessarily the thing you fed the task in the first place. As a minimum, we should update the Javadoc to reflect the state of affairs.

In our case, for example, the Runnable would implement other interfaces to provide locking information for allowing exclusive execution of schedules in a clustered environment, so that only one node would execute at the time.
With this change, our clustered TaskScheduler cannot see those interfaces and the schedules will run in parallel on all nodes.

This use case is interesting, and we would be happy to consider how Spring can provide hook points or extensions to make this easier for you. Maybe this information can be held by the Task itself? Is there some contract that we could add that would help to make the locking decision in a better fashion?

Can you share code snippets that would illustrate how you are achieving this right now? Thanks!

[alienisty]

@bclozel unfortunately I am not at liberty to share any code, but my suggestion would be that rather than just wrapping the passed Runnable in OutcomeTrackingRunnable you could, instead, create a dynamic proxy which would implement SchedulingAwareRunnable and any other interface implemented by the incoming Runnable instance.

The gist would be something like:

public class Task {

  private final Runnable runnable;

  private TaskExecutionOutcome lastExecutionOutcome;

  /**
   * Create a new {@code Task}.
   * 
   * @param runnable the underlying task to execute
   */
  public Task(Runnable runnable) {
    Assert.notNull(runnable, "Runnable must not be null");
    this.runnable = (Runnable) Proxy.newProxyInstance(
      runnable.getClass().getClassLoader(),
      Stream.concat(
        Stream.of(runnable.getClass().getInterfaces()),
        Stream.of(SchedulingAwareRunnable.class)
      ).distinct().toArray(Class[]::new),
      new OutcomeTrackingHandler()
    );
    this.lastExecutionOutcome = TaskExecutionOutcome.create();
  }

  /**
   * Return the underlying task.
   */
  public Runnable getRunnable() {
    return this.runnable;
  }

  /**
   * Return the outcome of the last task execution.
   * 
   * @since 6.2
   */
  public TaskExecutionOutcome getLastExecutionOutcome() {
    return this.lastExecutionOutcome;
  }

  @Override
  public String toString() {
    return this.runnable.toString();
  }

  private class OutcomeTrackingHandler implements InvocationHandler {

    @Override
    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
      if (method.getReturnType() == Void.class && "run".equals(method.getName()) && method.getParameterCount() == 0) {
        Task.this.lastExecutionOutcome = Task.this.lastExecutionOutcome.start(Instant.now());
        runnable.run();
        Task.this.lastExecutionOutcome = Task.this.lastExecutionOutcome.success();
        return null;
      }
      return method.invoke(runnable, args);
    }

  }
}

Obviously AOP would be another option yb using an Around advice.

I hope this helps