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

TaskSchedulingAutoConfiguration prevents discovery of ScheduledExecutorService #15032

Closed
lukas-krecan-lt opened this issue Oct 31, 2018 · 15 comments

Comments

Projects
None yet
7 participants
@lukas-krecan-lt
Copy link

commented Oct 31, 2018

We are using DelegatingSecurityContextExecutorService to handle security annotatons in our scheduled tasks. It used to work before upgrading to Spring Boot 2.1.0 since ScheduledAnnotationBeanPostProcessor uses ScheduledExecutorService if it can not find any bean of type TaskScheduler.

TaskSchedulingAutoConfiguration introduced in 2.1.0 creates a default TaskScheduler if there is none, which makes ScheduledAnnotationBeanPostProcessor ignore our ScheduledExecutorService.

    @Bean
    // Ignored since Spring Boot 2.1.0
    public ScheduledExecutorService scheduler() {
        SecurityContext securityContext = SecurityContextHolder.createEmptyContext();
        securityContext.setAuthentication(new AuthenticatedUser(loggedInUser, "root"));

        ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(10);

        return new DelegatingSecurityContextScheduledExecutorService(scheduledExecutorService, securityContext);
    }
@lukas-krecan

This comment has been minimized.

Copy link

commented Oct 31, 2018

Seems to be related #15038

@philwebb

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

You might be able to use a SchedulingConfigurer to set the scheduler directly.

@philwebb

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@rwinch Does Spring Security offer a DelegatingSecurityContextTaskScheduler, or is there a recommended approach for this?

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

org.springframework.security.scheduling.DelegatingSecurityContextSchedulingTaskExecutor, perhaps?

@lukas-krecan-lt

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

The workaround is to wrap ScheduledExecutorService to ConcurrentTaskScheduler

@lukas-krecan-lt

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

There is lot of workarounds, it just makes migrating to 2.1.0 harder. Is it a documented intentional change?

@philwebb

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@lukas-krecan-lt, no, It's an unintended consequence of providing an auto-configured TaskScheduler bean. Generally, that feature seems useful so we just need to give some more consideration to this combination of features.

@philwebb

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

@wilkinsona

org.springframework.security.scheduling.DelegatingSecurityContextSchedulingTaskExecutor, perhaps?

It seems like TaskScheduler is intentionally different from a SchedulingTaskExecutor. The javadoc has:

This interface is separate from {@link SchedulingTaskExecutor} since it usually represents for a different kind of backend, i.e. a thread pool with different characteristics and capabilities. Implementations may implement both interfaces if they can handle both kinds of execution characteristics.

@snicoll

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

ScheduledAnnotationBeanPostProcessor uses ScheduledExecutorService if it can not find any bean of type TaskScheduler.

Thanks, I missed that case. It looks like we could backoff completely if a ScheduledExecutorService bean exists to let the framework bootstrap code use that supported code path. This doesn't sound too bad considering none of of the configuration properties would apply for that case then.

@snicoll

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Gave that a bit more thoughts.

This is a fallback code in Spring Framework that attempts to provide the best possible option given what's available in the context. I think we should try to align to that as much as we can if that's possible.

We should be able to detect a ScheduledExecutorService in the context and react to that but that poses some problem:

  1. If we let the auto-configuration kick-in, we have a situation where pooling properties are ignored because we provide an executor rather than configuring one. There is no notion of specifying an executor on the builder either (as it's a either-or proposition that could be confusing)
  2. If we back-off, which sounds like the logical thing to do based on 1. then anything down the road that expects a TaskScheduler will have to replicate this logic.

Right now there isn't any so it may not be so bad.

There is lot of workarounds, it just makes migrating to 2.1.0 harder.

@lukas-krecan-lt ignoring the fact we missed this one and that makes the upgrade harder, I don't understand how adding 3 lines of config to expose a TaskScheduler is "a lot of workarounds".

Having said that, I wonder if the Spring Security wrapper isn't generally useful and we shouldn't consider using that when Spring Security is at play. This can be done in the form of a customizer if we manage to expose the proper API on the builder.

@lukas-krecan-lt

This comment has been minimized.

Copy link
Author

commented Nov 1, 2018

Sorry, I meant that there is lot of options how to write a workaround. Adding 3 line config is easy once you know what's wrong. It took me some time to figure out that there is a new autoconfiguration etc. I am afraid that I am not the only one who uses ScheduledExecutorService to configure task scheduling.

@rwinch

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Having said that, I wonder if the Spring Security wrapper isn't generally useful and we shouldn't consider using that when Spring Security is at play. This can be done in the form of a customizer if we manage to expose the proper API on the builder.

I'm not sure that Boot can provide a wrapper without knowledge of what user should be leveraged. I think it is better to recommend users customize it themselves.

@lukas-krecan-lt

This comment has been minimized.

Copy link
Author

commented Nov 2, 2018

In theory TaskSchedulingAutoConfiguration could wrap existing ScheduledExecutorService into ConcurrentTaskScheduler but I see that TaskSchedulerBuilder is built around ThreadPoolTaskScheduler so it looks like a dead end too.

@snicoll

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

We are using DelegatingSecurityContextExecutorService

@lukas-krecan-lt can you please share why you're using that rather than DelegatingSecurityContextSchedulingTaskExecutor? The latter is the standard contract expected by scheduling support while the former is a fallback option in case it wasn't found. I'd recommend always using the primary/main use case.

The workaround is to wrap ScheduledExecutorService to ConcurrentTaskScheduler

That will work but I am not sure that's a valid workaround. I'd argue that DelegatingSecurityContextSchedulingTaskExecutor is what you should be doing.

I wonder if the use of a ScheduledExecutorService is common. For sure we should be doing something in Spring Boot, I am not sure yet what. This is flagged for team attention so we'll discuss this a bit more shortly.

@lukas-krecan-lt

This comment has been minimized.

Copy link
Author

commented Nov 2, 2018

I have inherited the code, so I do not know exactly, but I do not think there is some special reason.

Maybe emitting some warning and documenting the required change in the migration instructions would be good enough solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.