On Aug 31, 2010, at 4:31 PM, Jarek Gawor wrote: > Hi all, > > I ran into a problem with @Startup singletons with circular > dependencies. I tried to explain the problem in OPENEJB-1348. I also > attached a proposed patch for this problem that involves splitting > some of the Container.deploy() logic into two separate functions. For > that I added start() and stop() operations to the Container.java > interface (stop() was added for consistency as it wasn't really need > in this solution).
This will be good! Could help with the CDI integration as well because there exists a similar circular dependency situation where CDI Beans and EJB Beans can refer to each other, but there is no equivalent @DependsOn annotation that can span between the two. > I would appreciate review of this patch as I want to make sure these > changes sound and look right. Love the test cases. Always good to see those :) As a small tweak we should also split the deploy() method of the StatelessInstanceManager as its deploy method creates the pool and no invocations will succeed without that. The part that would get cut from the deploy() and moved to the start() is the filling of the pool to the minimum size. Possible bug side note that we don't need to worry about now: Since an @Stateless bean would now be available for invocation (will be ready after 'deploy' is finished) before its pool is filled to its minimum (the 'start' phase), if the bean is actually invoked prior to its start call it will result in an instance being created and added to the pool, then when its start phase is called the container will attempt to fill the pool to the minimum. This will result in extra instances beyond the minimum being created and added to the pool. Not the end of the world but something we should file a separate JIRA for so we don't forget it. Note the 'initializeDependencies' method of the SingletonInstanceManager shouldn't be necessary. The CheckDependsOn validation code should pick up that scenario where a @DependsOn points to a non-existent bean. There's a related CheckDependsOnTest that tests it. The 'createInstance(dependencyInfo)' call happens automatically when anyone invokes a singleton via it's proxy, so we should be good there and can let the createInstance calls happen naturally as part of usage. Technically, that would also cause a bug as the code that actually enforces the concept of the singleton is in the 'getInstance' method, calling 'createInstance' directly will bypass that can cause an instance to be created and immediately forgotten -- i.e. it doesn't stick unless called by 'getInstance'. We could definitely document that better. Feel free to throw some javadoc in if you're feeling ambitious :) Overall, great change. This is the kind of thing I've wondered if we'd eventually need, so far the answer has been "no" but we clearly had a too-strict singleton startup order requirement. This will be good as even if someone doesn't use the @DependsOn annotation, the circular reference still will work in all situations except an actual circular method call during @PostConstruct. A very nice improvement! -David
