Shedlock prevents Tomcat from graceful shutdown

[rgala]

When using Shedlock in a Spring Boot app run on external Tomcat, the Tomcat fails to shutdown gracefully and has to be killed.

Steps to reproduce:

1. Download Tomcat 8.5.45 from https://tomcat.apache.org/download-80.cgi and unzip
2. Build the attached project and copy WAR file to webapps directory of previously extracted archive
3. Change to a directory where Tomcat has been extracted
4. Run the bin\startup.bat script and wait for the Tomcat to boot up
5. Run the bin\shutdown.bat script

It should get stuck and the last log seen should be like the one below:

28-Aug-2019 16:14:04.995 INFO [main] org.apache.coyote.AbstractProtocol.destroy Destroying ProtocolHandler ["ajp-nio-8009"]

demo.zip

[rgala]

Looks like when ConcurrentTaskScheduler is constructed using an empty constructor (bean is declared in RegisterDefaultTaskSchedulerPostProcessor class), it creates a ScheduledExecutorService instance in the setScheduledExecutor method (Executors.newSingleThreadScheduledExecutor() line):

public final void setScheduledExecutor(ScheduledExecutorService scheduledExecutor) {
	if (scheduledExecutor != null) {
		this.scheduledExecutor = scheduledExecutor;
		this.enterpriseConcurrentScheduler = (managedScheduledExecutorServiceClass != null &&
				managedScheduledExecutorServiceClass.isInstance(scheduledExecutor));
	}
	else {
		this.scheduledExecutor = Executors.newSingleThreadScheduledExecutor();
		this.enterpriseConcurrentScheduler = false;
	}
}

The created ExecutorService instance never gets shut down and this probably prevents Tomcat from stopping.

I managed to workaround the issue by creating TaskScheduler and ScheduledExecutorService as beans like this:

@Bean
public TaskScheduler ts(ScheduledExecutorService ses) {
	return new ConcurrentTaskScheduler(ses);
}

@Bean
public ScheduledExecutorService ses() {
	return Executors.newSingleThreadScheduledExecutor();
}

And now the RegisterDefaultTaskSchedulerPostProcessor uses my ScheduledExecutorService bean instead of creating a new one with empty constructor, Tomcat shuts down without problems. Probably because Spring destroys those beans when closing. I am not sure if this is going to have an impact on ShedLock though :(

[lukas-krecan]

Thanks a lot for the investigation, will take a look at it

[rgala]

I think this is more a Spring bug then ShedLock. It is easy to reproduce without using ShedLock at all, just by specyfiying the following bean in an application:

@Bean
public TaskScheduler taskScheduler() {
	return new ConcurrentTaskScheduler(Executors.newSingleThreadScheduledExecutor());
}

When there is a TaskScheduler bean declared in an application (which happens automatically when ShedLock is enabled), then the ExecutorService does not get shut down when Spring shuts down.

There is a method scheduleTasks in ScheduledTaskRegistrar, and when there is a TaskScheduler bean in an application, it does not create it's own ExecutorService object in below code:

protected void scheduleTasks() {
	if (this.taskScheduler == null) {
		this.localExecutor = Executors.newSingleThreadScheduledExecutor();
		this.taskScheduler = new ConcurrentTaskScheduler(this.localExecutor);
	}

... }

and when this bean is destroyed, the destroy method does not call shutdown on ExecutorService:

@Override
public void destroy() {
	for (ScheduledTask task : this.scheduledTasks) {
		task.cancel();
	}
	if (this.localExecutor != null) {
		this.localExecutor.shutdownNow();
	}
}

I think that the ConcurrentTaskScheduler should implement the shutdown method that would call shutdown on it's internal ExecutorService field, for example

public void shutdown() {
	if (this.scheduledExecutor != null) {
		this.scheduledExecutor.shutdownNow();
	}
}

This method should be called from destroy method in ScheduledTaskRegistrar when Spring shuts down.

This is probably not an issue in Spring apps running on embedded container, because they are usually closed by sending SIGTERM to the process (like pressing CTRL+C in a console) which causes the shutdown hooks to be executed. ExecutorService implementations probably have their own shut down hooks and close themselves automatically when the JVM is shutting down.

[lukas-krecan]

You are right, ConcurrentTaskScheduler does not implement shutdown method, but ScheduledExecutorService does, so if you create bean of this type it gets destroyed.

[rgala]

You could consider instantiating your own ScheduledExecutorService bean, conditional on missing bean of the same type in case someone wanted to use their own bean.

[agoston]

I found this part too and was surprised; this is quite invasive behavior. Normally you'd expect user to set up their context properly and throw an initialization error if not.

Right now, for example, @scheduled annotations would work if this library was added as a maven dependency, but if a user chose to remove it, they would be broken, too.

[lukas-krecan]

Sorry, I do not understand. Schedulled annotations work without ShedLock by instantiating an internal TaskScheduler. ShedLock just needs to create a TaskScheduler bean to make it work together

[agoston]

Sorry, i had some weird/bad experiences when configuring concurrent scheduling in spring-boot (the default is single-threaded). It's just not as clearly defined what and how to do as one would expect. I ended up fiddling with SchedulingConfigurer and registering a ThreadPoolTaskScheduler into the spring context.

Hence my wondering that if shedlock does some of this configuration for the user, and he/she starts depending on this, it can lead to random weird failures if/when this autoconfig is removed for any reason (removal of lib, or maybe you choose to remove it in a newer version?).

Anyway, it's already there, so it doesn't matter, just generally, I prefer not to create such situations, as it's really hard to debug, being non-intuitive.

[lukas-krecan]

I see, I will try to document it better.

[lukas-krecan]

Since version 4.0.0 the default intercept mode is PROXY_METHOD which does not meddle with the task scheduler.