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

Consider Changing AutomaticJobRegistrar to Implement SmartLifeCycle [BATCH-2564] #1040

Closed
spring-projects-issues opened this issue Dec 15, 2016 · 2 comments
Labels
has: backports Legacy label from JIRA. Superseded by "for: backport-to-x.x.x" in: core type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

Gary Russell opened BATCH-2564 and commented

The AutomaticJobRegistrar is automatically start() ed when a ContextRefreshedEvent is emitted.

This is too late when components using jobs implement SmartLifecycle.

It should really implement SmartLifecycle and load jobs in an early-ish phase.

There is actually a TODO in the code...

public final void onApplicationEvent(ApplicationEvent event) {
	// TODO: With Spring 3 a SmartLifecycle is started automatically
    ...
}

See the referenced Stack Overflow question.

This can probably be a 4.0 only change; since there's a work-around.


Affects: 3.0.7

Reference URL: http://stackoverflow.com/questions/41145162/startup-race-condition-spring-batch-with-spring-integration

Attachments:

Referenced from: pull request #571, and commits 45ea34b, 4dc406e

Backported to: 3.0.9

@spring-projects-issues
Copy link
Collaborator Author

Joris Kuipers commented

I believe this issue is more pressing than apparent from the description. By implementing ApplicationListener and listening for every event, this class will be prematurely initialized when an event is published during the ApplicationContext construction. This happens always when using Spring Boot, as that publishes a DataSourceInitializedEvent which in turn causes this class to be initialized.
This will lead to cyclic dependencies between beans: I ran into this while porting an existing application using Spring Batch to Spring Boot.

Although I couldn't reproduce the exact issue (I would really get a cycle with a nice graphical depiction of that by Boot at startup), I have managed to reproduce a very similar case in a trivial application. I've attached a project with three Spring Boot main classes:

  1. ModularBatchConfigurationApplication uses @EnableBatchProcessing(modular = true). This is pretty much impossible to get going with a JPA Spring Boot app, as the ModularBatchConfiguration#configurers field causes a dependency cycle that I wouldn't know how to break (probably deserves its own issue: I think the ModularBatchConfiguration is not very well designed, as it ensists on defining a transactionManager bean even if the application already defines one itself or through Spring Boot)
  2. ConfigurerlessModularBatchConfigurationApplication foregoes the @EnableBatchProcessing(modular = true) and defines the relevant beans itself, including the AutomaticJobRegistrar. This showcases the problem with the premature initialization: in this example Spring doesn't detect the cycle but rather causes a NPE because the @Configuration class isn't fully dependency-injected yet when called (it misses the ApplicationContext
  3. CustomAutomaticJobRegistrar finally uses the same setup as above, but defines an updated automatic job registrar implementation using SmartInitializingSingleton

I know that by fiddling around certain things you can actually get the failing apps to work, but that's not the point: in a real, bigger app users might not be able to make those changes.

Also, if depending on SmartInitializingSingleton as I did isn't an option because it's Spring 4.1 and higher only, then using SmartLifeCycle as suggested by the existing code and this story will work as well to avoid the cycle in the dependency graph.

@spring-projects-issues
Copy link
Collaborator Author

Joris Kuipers commented

Oh, I just found that what I'm describing is basically what's already covered in BATCH-2506. However, the solution chosen there can still lead to the same problem, it's just less likely to happen. The whole idea of listening for events just to find out if the application has started or is stopping is wrong, the code really should just use SmartInitializingSingleton (with DisposableBean) or SmartLifeCycle instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has: backports Legacy label from JIRA. Superseded by "for: backport-to-x.x.x" in: core type: enhancement
Projects
None yet
Development

No branches or pull requests

1 participant