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

SchedulingConfigurer's ScheduledTaskRegistrar should reliably shut down before TaskScheduler [SPR-15067] #19633

Closed
spring-projects-issues opened this issue Dec 29, 2016 · 1 comment
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 29, 2016

Ruslan Stelmachenko opened SPR-15067 and commented

When I define taskScheduler bean with setWaitForTasksToCompleteOnShutdown(true) and setAwaitTerminationSeconds(20) and at the same time schedule some tasks using SchedulingConfigurer and it's ScheduledTaskRegistrar, then taskScheduler never shutdown gracefully because it's queue contains programmatically scheduled tasks (using taskRegistrar), but no one cancels these tasks before taskScheduler tries to shutdown.

@SpringBootApplication
@EnableScheduling
public class SchedulingBugDemoApplication implements SchedulingConfigurer {

	private static final Logger log = LoggerFactory.getLogger(SchedulingBugDemoApplication.class);

	public static void main(String[] args) {
		SpringApplication.run(SchedulingBugDemoApplication.class, args);
	}

	@Bean
	public TaskScheduler taskScheduler() {
		ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler();
		scheduler.setThreadNamePrefix("myScheduler-");
		scheduler.setPoolSize(10);
		// block spring context stopping to allow SI pollers to complete
		// (to graceful shutdown still running tasks, without destroying beans used in these tasks)
		scheduler.setWaitForTasksToCompleteOnShutdown(true);
		scheduler.setAwaitTerminationSeconds(20);
		return scheduler;
	}

	@Override
	public void configureTasks(ScheduledTaskRegistrar taskRegistrar) {
		taskRegistrar.addTriggerTask(
				() -> work(),
				new CronTrigger("*/5 * * * * *"));

		// The second task sits in the scheduler queue and prevents scheduler to shutdown().
		// The task's cancellation lies in taskRegistrar's destroy() method, but no one
		// calls it until context bean destruction is in place. At the same time the context
		// waits for taskScheduler to terminate to continue beans destruction procedure.
		taskRegistrar.addTriggerTask(
				() -> work(),
				new CronTrigger("0 0 0 */1 * *"));
	}

	void work() {
		log.info("Working...");
	}

}

The second task sits in the scheduler queue and prevents scheduler to shutdown().
The task's cancellation lies in taskRegistrar's destroy() method, but no one calls it until context bean destruction is in place. At the same time the context waits for taskScheduler to terminate to continue beans destruction procedure.

When we use @Scheduled annotation, then this problem is not present because ScheduledAnnotationBeanPostProcessor's destroy() method explicitly calls ScheduledTaskRegistrar's destroy() method, which cancels all the scheduled tasks.

But when we register our tasks programmatically, no one calls ScheduledTaskRegistrar's destroy() method before TaskScheduler tries to shutdown() and awaitTermination().

Maybe we can force somehow to call ScheduledTaskRegistrar's destroy() method before TaskScheduler's destroy() method? Of course this can be done manually for example:

	private volatile ScheduledTaskRegistrar taskRegistrar;

	@Override
	public void configureTasks(ScheduledTaskRegistrar taskRegistrar) {
		this.taskRegistrar = taskRegistrar;
		// ...
	}

	@PreDestroy
	public void destroy() {
		taskRegistrar.destroy();
	}

This workaroud works because the @Configuration bean's destory() method called before TaskScheduler's. But I don't know if it is guaranteed or not.

I can imagine many workarounds for this but will be better to somehow make it work out of the box, as the @Scheduled annotation works.


Best regards, Ruslan.


Affects: 4.3.5

Attachments:

Issue Links:

  • #12206 findDefaultEntityManagerFactory should consider EMF bean's primary flag
  • #19256 ScheduledAnnotationBeanPostProcessor should reliably apply after AnnotationAwareAspectJAutoProxyCreator
  • #17839 AsyncAnnotationBeanPostProcessor could find TaskExecutor by type/name
  • #19635 Track bean dependencies for calls between @Bean methods within @Configuration classes
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 29, 2016

Juergen Hoeller commented

This turns out to be a dependency ordering problem: The ScheduledTaskRegistrar, or more specifically its containing ScheduledAnnotationBeanPostProcessor, isn't registered as dependent on the TaskScheduler bean and therefore not enforced to be shut down before the scheduler bean. It may still shut down first but that's dependent on registration order which is hard to predict for configuration classes, only really easily enforced with XML bean definitions.

Revising ScheduledAnnotationBeanPostProcessor's lookup to register the dependency with the factory addresses that problem, with the destruction callbacks automatically arriving in order now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants