Lifecycle processing ignores phases for circularly dependent SmartLifecycle beans [SPR-8941]

[spring-projects-issues]

Hannes Schmidt opened SPR-8941 and commented

I have two beans A and B. Both are services, both implement SmartLifecycle. Both have an auto-wired dependency to the other bean. IOW, there is a circular dependency between them. A.getPhase() returns 1, B.getPhase() returns 2. Intuitively, the expected behavior is that A.start() is invoked before B.start(). The actual behavior in 3.0.5 is that B.start() is invoked before A.start().

DefaultLifecycleProcessor attempts to start A first, but before doing so starts all lifecycle beans among A's dependencies first. B is one of those so its start() is actually invoked first.

My take on this is that an injection dependency is not necessarily the same as a lifecycle dependency. Spring's lifecycle processing treats them as the same thing. Bean X may depend on bean Y and yet bean X might have to be started before Y for reasons orthogonal to dependency injection, say concurrency. SmartLifecycle's Javadocs state explicitly that depends-on has priority over phases, yet I feel that overriding getPhase() is a much stronger statement than introducing a bean reference, especially when it is auto-wired, so I would think that getPhase() should overrule depends-on relationships.

I don't see how these two dependency sets can be untangled without breaking backwards compatibility.
On the other hand, we could limit the fix to circularly dependent beans: If beans A and B depend on each other but belong to distinct phases, the dependency should be ignored. I'd be willing to supply a patch, if requested.

---

Affects: 3.0.5

1 votes, 1 watchers

[spring-projects-issues]

Hannes Schmidt commented

I have a fix but it is more intrusive than I would like it to be. It is very hard to efficiently determine whether two beans form a transitive circular dependency (correct if I am wrong) so I resorted to a global flag that tells the processor to prioritize phases over dependencies. The fix is in the form of a custom lifecycle processor but I can easily turn that into a patch against DefaultLifecycleProcessor.

BTW: I am pretty sure this issue affects to 3.0.6 as well as 3.1 (HEAD).

[spring-projects-issues]

Chris Beams commented

Hi Hannes!

Thanks for the detailed explanation so far. A patch and test case that demonstrates what you've described would be excellent. We can certainly take a look at this.

And actually, better than a patch would be a pull request against the new spring-framework GitHub repository. We've just completed this move from SVN, and haven't yet announced it publicly, but you're free nevertheless to create a fork and submit a pull request.

Also, while it's not a general solution to the problem, have you considered the isAutoStartup property for either A or B? I assume that you are aware of this option, and that you do indeed need both A and B to auto-start, but just in case either of those assumptions are not true, this might provide a suitable workaround for your needs. In fact, we recently addressed a related problem with #13552, the fix for which is available against 3.1 GA (and likely will be available against the forthcoming 3.0.7). You might want to take a look at that.

Chris

[spring-projects-issues]

Hannes Schmidt commented

Hi Chris,

Thanks for helping with this. I took a first stab at the problem on the master branch [1]. The bad news (for me) is that there was actually already a testcase verifying the behavior that I consider 'broken' or at least counter-intuitive. So the author thinks that a bean in phase 2 should be started before a bean in phase 1 if the phase 1 bean depends on the phase 2 bean. This is certainly a valid view point but it has two flaws IMO: it breaks down for circular dependencies (beans depending on each other are started out of phase as described above) and it favors a weak relationship over a strong one (declaring a phase for a lifecycle bean by overriding getPhase() is a much stronger statement than declaring a bean field referencing another bean).

Am I right in assuming that the context has already been initialized and dependencies resolved by the time the start() method is invoked on lifecycle beans? If that's the case, why is so much importance given to bean dependencies during lifecycle processing? These are not rhetorical questions, I really am not sure about this.

This commit introduces a flag switching between existing behavior (the default) and the new one which makes sure that dependencies aren't started out of phase. I also took the existing unit tests verifying the old behavior and added a similar switch such that they test both behaviors. Depending on the result of our discussion I would also want to refactor the DefaultLifecycleProcessor tests such that all tests are run against the old and the new behavior. I'd probably want to use JUnit's parameterized tests for that.

[1] #10

PS: Autostart won't work in my case but I have a workaround already in place that closely resembles what I have done in the above commit.

[spring-projects-issues]

Chris Beams commented

Hi Hannes,

Just getting back to this as we approach the 3.2 M1 release, sorry for the delay.

I think what you've laid out here makes a lot of sense. I'm basically for the change, and I don't think that the global flag is necessarily 'too intrusive', especially when considering that trying to narrow this switch down to circularly-dependent beans would (as you mention) be convoluted.

Would you be willing to take the pull request to the next stage as you suggest and round out the tests? See also my comments on the pull request itself about the contributor guidelines.

Thanks!

P.S. 3.2 M1 will most likely drop late next week, so if you're interested in getting this in, that's the timeline. If that's too soon, M2 will be fine as well, but thought you'd like to know.

[spring-projects-issues]

Bulk closing outdated, unresolved issues. Please, reopen if still relevant.