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.

Comment From: 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!!

Comment From: 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!

Comment From: 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

Comment From: bclozel

@alienisty too bad, hopefully @terje2001 can help us better understand this use case and how we can better support it.

In the meantime I created #35394 to solve the main problem here.

Comment From: alienisty

@bclozel I don't think the key is just documentation here. Spring itself is leveraging a contact attached to the runnable to do something with it, which makes, in my opinion, the current implementation choice brittle. I believe, as I suggested before, that using AOP or a dynamic proxy would be a better solution because they would be transparent and let any contact on the original runnable to remain available.

Comment From: bclozel

@alienisty I answered that point in my previous comment. I think that creating proxies for each task would be inefficient and would not solve the underlying problem for that API.

I am leaving this issue opened for now to find a way to support this use case properly (and more!), but for that we need community members willing to engage.

We did not get much similar feedback so far and it seems there is a workaround. If we cannot make progress there we will probably have to close this issue until we get more clarity.

Comment From: spring-projects-issues

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

Comment From: spring-projects-issues

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

Spring Allow to track locking information on scheduled tasks

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.

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.