Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TaskScheduler does not work with TaskDecorator #23755

Closed
ttddyy opened this issue Oct 4, 2019 · 3 comments
Closed

TaskScheduler does not work with TaskDecorator #23755

ttddyy opened this issue Oct 4, 2019 · 3 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@ttddyy
Copy link
Contributor

ttddyy commented Oct 4, 2019

TaskDecorator doesn't directly work with task scheduler implementations - ThreadPoolTaskScheduler/ConcurrentTaskScheduler.

Related: #18502

I think the underlying reason is there is no easy way of applying TaskDecorator to ScheduledExecutorService.

Currently, I workaround by wrapping ScheduledExecutorService with a proxy that performs task decoration.

Proxy Handler:

public class TaskDecoratingScheduledExecutorServiceInterceptor implements MethodInterceptor {

    private final TaskDecorator taskDecorator;

    public TaskDecoratingScheduledExecutorServiceInterceptor(TaskDecorator taskDecorator) {
        this.taskDecorator = taskDecorator;
    }

    @Override
    public Object invoke(MethodInvocation invocation) throws Throwable {
        Object[] args = invocation.getArguments();
        if (args.length == 0) {
            return invocation.proceed();  // no decoration, simply proceed
        }

        Object swapped;
        if (args[0] instanceof Runnable) {
            swapped = replaceRunnable(method, (Runnable) args[0]);
        } else if (args[0] instanceof Callable) {
            swapped = replaceCallable(method, (Callable) args[0]);
        } else if (args[0] instanceof Collection) { // see the ExecutorService API
            swapped = ((Collection<? extends Callable<?>>) args[0]).stream()
                    .map(callable -> replaceCallable(method, callable))
                    .collect(toList());
        } else {
            return invocation.proceed(); // bail out, no replace needed
        }
        args[0] = swapped;  // swap

        return invocation.proceed();
    }

    ....
}

Wrap created ScheduledThreadPoolExecutor in ThreadPoolTaskScheduler:

ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler() {

    private ScheduledThreadPoolExecutor scheduledThreadPoolExecutor;

    @Override
    protected ScheduledExecutorService createExecutor(int poolSize, ThreadFactory threadFactory, RejectedExecutionHandler rejectedExecutionHandler) {
        
        // keep it for "getScheduledThreadPoolExecutor()"
        this.scheduledThreadPoolExecutor = (ScheduledThreadPoolExecutor) super.createExecutor(poolSize, threadFactory, rejectedExecutionHandler);

        ScheduledExecutorService executorService = this.scheduledThreadPoolExecutor;

        // apply task decorator via proxy
        ProxyFactory proxyFactory = new ProxyFactory(executorService);
        proxyFactory.addAdvice(new TaskDecoratingScheduledExecutorServiceInterceptor(taskDecorator));

        return (ScheduledExecutorService) proxyFactory.getProxy();
    }

    @Override
    public ScheduledThreadPoolExecutor getScheduledThreadPoolExecutor() throws IllegalStateException {
        return this.scheduledThreadPoolExecutor;
    }
}

I think it would be nice that ThreadPoolTaskScheduler/ConcurrentTaskScheduler to have some API to work with TaskDecorator OR a way to easily customize underlying ScheduledExecutorService to apply decorators.

For example, a delegate class that takes TaskDecorator:

public class TaskDecoratingScheduledExecutorServiceDelegate implement ScheduledExecutorService {
  private final ScheduledExecutorService delegate;
  private final TaskDecorator taskDecorator;
  
  ...
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 4, 2019
@rstoyanchev rstoyanchev added the in: core Issues in core modules (aop, beans, core, context, expression) label Nov 10, 2021
@osamucamarques
Copy link

It would be great!

@osamucamarques
Copy link

osamucamarques commented Jan 31, 2023

till now I handle it like this:

image

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 27, 2023
@snicoll snicoll self-assigned this Nov 27, 2023
@snicoll snicoll added this to the 6.2.x milestone Nov 27, 2023
@jhoeller jhoeller assigned jhoeller and unassigned snicoll Mar 15, 2024
@jhoeller jhoeller modified the milestones: 6.2.x, 6.2.0-M1 Mar 15, 2024
@jhoeller
Copy link
Contributor

jhoeller commented Mar 15, 2024

On a similar note as with #32460, this is actually an area of inconsistency as of 6.1 since the new SimpleAsyncTaskScheduler comes with inherited TaskDecorator support from SimpleAsyncTaskExecutor. Even ConcurrentTaskScheduler comes with some TaskDecorator support from its ConcurrentTaskExecutor base class but does not apply it to scheduler operations, just to the inherited executor operations. We'll make this as consistent as possible for 6.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants