Bugzilla – Full Text Bug Listing
| Summary: | Construct application contexts defined in extender fragments only once and share across extender instances | ||
|---|---|---|---|
| Product: | [RT] Gemini.Blueprint | Reporter: | Olaf Otto <olaf> |
| Component: | Core | Assignee: | Glyn Normington <glyn.normington> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | critical | ||
| Priority: | P3 | CC: | aaronjwhiteside, cleau, glyn.normington, norbert.wirges, pasch, ryan.heinen |
| Version: | unspecified | ||
| Target Milestone: | 2.0.0.M01 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 393961 | ||
| Attachments: | |||
|
Description
Olaf Otto
I've investigated this further, and the problem simply seems to be that in case blueprints are enabled, i.e.
private static final boolean BLUEPRINT_AVAILABLE = ClassUtils.isPresent("org.osgi.service.blueprint.container.BlueprintContainer", ChainActivator.class.getClassLoader());
is true, the following listeners are added in the ChainActivator:
if (BLUEPRINT_AVAILABLE) {
...
CHAIN = new BundleActivator[] { new ContextLoaderListener(), new BlueprintLoaderListener() };
}
This seems wrong - BlueprintLoaderListener extends ContextLoaderListener, and both create a new extender configuration.
IMO it should be
if (BLUEPRINT_AVAILABLE) {
...
CHAIN = new BundleActivator[] { new BlueprintLoaderListener() };
}
I will try to patch this and report back the testing results.
I've tested with the before mentioned change. This works to the extend that a) The context is only create once, as expected b) pure blueprint configurations are read and supported (i.e. xml files residing in /OSGI-INF/blueprint/ are subject to autodetection). However this change would mean that the extender is down backwards compatible since spring-osgi configurations residing in /META-INF/spring are *not* detected. So the situation with the 1.0.0.RELEASE is: - Both BlueprintLoaderListener and ContextLoaderListener instantiate the contexts residing in /META-INF/spring/extender/ - Both listeners are attached if blueprint is available, which leads to a duplicate context creation. However, I think the key issue is that even with the new blueprint spec the extender config is still obtained from /META-INF/spring/extender/. This does not make any sense. IMHO if /META-INF/spring/*.xml is changed to /OSGI-INF/blueprint/*.xml, as should the extender configuration, i.e. it should be /OSGI-INF/blueprint/extender/*.xml instead of /META-INF/spring/extender/*.xml. This would be a clean separation of old and new spec and would resolve the issue of having to support both config locations in all implementations. However, with this change an extender configuration that conforms to the blueprint spec by providing its configuration under /OSGI-INF/blueprint/extender/ would only be applied to blueprint conform bundles since it is only applied if a blueprint-based context configuration can be created (BlueprintContainerCreator). Perhaps this should be treated as a different issue; not supporting a mixed-spec case is probably desirable, i.e. one may define that *either* the old spring locations *or* the the blueprint locations must be used, but not both. Created attachment 202769 [details]
Modified version of ExtenderConfiguration and BlueprintExtenderConfiguration to overlay into extender maven project
Added a zip file containing the modified classes.
(In reply to comment #2) I've successfully tested supporting /OSGI-INF/blueprint/extender/ as the location for blueprint extensions and suggest this as a solution together with not supporting mixing old and new configuration locations within one deployment. Feedback is appreciated! Hello, There hast been no feedback so far - this is however an urgent issue for me; I have three very large deployments that I must equip with the outdated spring-osgi unless this is resolved. Thanks, Olaf I think the best situation for your case would be to use a patched version of Gemini Blueprint. This bug needs to be fixed but an upcoming release won't happen in the very near future so it won't address the urgency of your issue. Fix wise, the current behaviour will remain as is - the fix will focus on not instantiating, or at least avoiding the exception caused by the double initialization. I'm not clear why the double initialization is a problem - do you get some sort of exception? (In reply to comment #6) > I think the best situation for your case would be to use a patched version of > Gemini Blueprint. This bug needs to be fixed but an upcoming release won't > happen in the very near future so it won't address the urgency of your issue. > Done. Is there a Roadmap for upcoming releases? > Fix wise, the current behaviour will remain as is - the fix will focus on not > instantiating, or at least avoiding the exception caused by the double > initialization. The latter would not be an acceptable fix. IMO The guarantees of the application lifecycle *must* not be violated by an unexpected double instantiation of the context. > I'm not clear why the double initialization is a problem - do you get some sort > of exception? The double instantiation means that - amongst other things - singletons are instantiated twice and OSGi-Services (which I am exporting in the bundle) are also exported twice. Just think of what this will do with @Scheduled stuff or other Services relying on the fact that only one instance of a Service interface gets exported. In my case this does not lead to an exception but, much worse, yields incorrect application behavior at runtime. Also, this is a problem when using the extender interfaces for bean factory customization (such as OsgiBeanFactoryPostProcessor), since only one such post processor gets applied. I have tested this against the 1.1 m1 build and the problem still persists, i.e. the duplicate registration of blueprint extender configuration and extender configuration in ChainActivator still leads to a duplicate instantiation of the /meta-inf/spring/extender context. I would be more than grateful if this issue was addressed in the 1.1 release. The attached fix would still do the job IMHO (simply changing the location of the extender configuration if blueprint is supported). Thanks & regards, Olaf This is also a problem for us. Is it planned to include a fix for this in the 1.1 release? *** Bug 384410 has been marked as a duplicate of this bug. *** This is working as designed. Gemini Blueprint provides a Spring DM extender and, if BLUEPRINT_AVAILABLE (see comment 1), a Blueprint extender. The ability to configure these extenders is non-OSGi standard, which is why the extender configuration is placed in META-INF/spring/extender rather than OSGi-INF. Both extenders (in the case where they are both started) are configured the same based on the META-INF/spring/extender. So if one-time logic needs to be included in the beans which configure the extenders, it should guard itself along the lines of the test bundle attached to this bug (although the code should be thread safe). If we did decide to change the behaviour, I think the blueprint extender configuration should go in some obviously non-standard location like META-INF/blueprint/extender. However this would be a backward incompatible change and would need to be introduced at a major version boundary (e.g. at Gemini Blueprint 2.0.0.RELEASE). Given the lack of contributions to Gemini Blueprint, I think bug fixes are more likely to be the order of the day for now, so a 2.0 release is not envisaged. Before I close as WONTFIX, does anyone think that the one-time guard logic is not possible to implement? Hi Glyn First of all, thank you for picking this up again! Here are my five cents: (In reply to comment #11) > This is working as designed. Gemini Blueprint provides a Spring DM extender > and, if BLUEPRINT_AVAILABLE (see comment 1), a Blueprint extender. The > ability to configure these extenders is non-OSGi standard, which is why the > extender configuration is placed in META-INF/spring/extender rather than > OSGi-INF. Both extenders (in the case where they are both started) are > configured the same based on the META-INF/spring/extender. I disagree here: While the intend is indeed supporting both spring DM and Blueprint, it clearly is not intended to interpret every extender configuration as both at the same time. > So if one-time logic needs to be included in the beans which configure the > extenders, it should guard itself along the lines of the test bundle > attached to this bug (although the code should be thread safe). This may be feasible, but only using very dirty approaches (static fields, synchronization...). However there is significant risk that it may not, given that we are looking at a complete Spring application context lifecycle. Besides, the documentation clearly states that the extender configuration is a standard ApplicationContext [1] - in my understanding, it must thus adhere to corresponding guarantees regarding the bean lifecycle; uncontrollable duplication instantiation should not occur. > If we did decide to change the behaviour, I think the blueprint extender > configuration should go in some obviously non-standard location like > META-INF/blueprint/extender. However this would be a backward incompatible > change and would need to be introduced at a major version boundary (e.g. at > Gemini Blueprint 2.0.0.RELEASE). Given the lack of contributions to Gemini > Blueprint, I think bug fixes are more likely to be the order of the day for > now, so a 2.0 release is not envisaged. > Indeed, a loss of backwards compatibility could be a major issue... > Before I close as WONTFIX, does anyone think that the one-time guard logic > is not possible to implement? I think this is really hard to answer and depends on the complexity of the application. Taking the application I am working with it would be, at the very least, problematic. We are using a lot of advanced spring features (AOP for proxies, @Scheduled, @Async) and also declaratively produce and consume OSGi services. How would one, for instance, stop the provisioning of OSGi services by other means than shifting from declarative service publication (i.e. <bp:service>) to an ugly, completely programmatic service registration? Looking at the code (BlueprintExtenderConfiguration, specifically) I think the true issue here is that the container creation strategy and extender configuration are coupled. IMO, a possible solution would be to use one common ExtenderConfiguration _instance_ for all LoadListeners and let them determine the OsgiApplicationContextCreator. This would also aide in resolving an oddity that can be spotted in the BlueprintExtenderConfiguration - there, the contextCreator can no longer be customized, whereas the documentation states otherwise [1] (even though, agreed, there is no specific word on blueprint specific configuration constraints). [1] http://www.eclipse.org/gemini/blueprint/documentation/reference/1.0.1.RELEASE/html/app-deploy.html#app-deploy:extender-configuration Regards, Olaf Hi Olaf I'm now wondering whether your extender configuration needs to be as sophisticated as it appears to be. I recommend keeping extender configuration logic minimal in order to be completely bullet-proof and robust. I wonder if there is some application logic which can be factored out into a regular Spring DM/Blueprint powered bundle. Just a thought. Does it help? Regards, Glyn Hi Glyn, It is indeed sophisticated, however the extension provides quite some features and depends on being initialized before any of the blueprint/dm bundles, since it provides these with additional infrastructure and features (an annotation-based DSL, that is...). I have already considered moving features out of the fragment, however this does not seem feasible due to the startup-order dependency... However, after taking some time to dig deeper into the extender codebase, I believe that a backwards-compatible change is quite achievable. For this, one must decompose the ContextLoaderListener (which IMO is a bit of a large class) into a couple of collaborators representing the aspects involved: - staged configurations (Java beans cache, namespace handling, listeners, configuration) - registration of lifecyclemanagement These phases can then simply be implemented as BundleActivators to have them executed in the desired order. Then, it is possible to adapt the blueprint-specific stages of the initialization phase (as far as I can see, these are the namespace handling and the container-construction aspects in the BlueprintLoadListener) without duplicating the extender configuration. I took the liberty to perform this change on the 1.0.2.RELEASE (patch attached) and it appears to work quite well. That is, the unit tests are green and my complex test project is happy. I did not quite get how your integration tests are executed, so any advise there would be highly appreciated. Also, some of the documentation in the refactored code would still need to be shifted (the ContextLoaderListener is a lot smaller now). I know it's alot to ask, but would you have time to take a look at the changes? Is such a change an option here? Regards, Olaf (In reply to comment #13) > Hi Olaf > > I'm now wondering whether your extender configuration needs to be as > sophisticated as it appears to be. I recommend keeping extender > configuration logic minimal in order to be completely bullet-proof and > robust. > > I wonder if there is some application logic which can be factored out into a > regular Spring DM/Blueprint powered bundle. > > Just a thought. Does it help? > > Regards, > Glyn Created attachment 223184 [details]
Decomposed ContextLoaderListener with singleton configuration for 356683
(In reply to comment #14) > However, after taking some time to dig deeper into the extender codebase, I > believe that a backwards-compatible change is quite achievable. Ok, then how would extender configuration look at a high level, in order to preserve backward compatibility? The configuration configures both extenders today, so presumably that would be true with your change in the default case with some kind of option to do things the way you want? >I did not quite get how your > integration tests are executed, so any advise there would be highly > appreciated. Try: mvn clean install -Pit,equinox > I know it's alot to ask, but would you have time to take a look at the > changes? Is such a change an option here? Once your patch is passing the integration tests and I've understood how you plan to achieve backward compatibility, I'll gladly spend some time on it. Hello Gly Thanks for the hint. The integration tests are green: Tests run: 229, Failures: 0, Errors: 0, Skipped: 0 Concerning the approach: Yes, there is only one common configuration. With the patch, this configuration is only instantiated once before any load listeners are instantiated, and then shared. There is only one semantic change in the patch which is easy to revert (and, to be on the safe side, should perhaps be reverted as long as both BP and DM are supported at the same time): Should a user provide a custom OsgiApplicationContextCreator, it will now be used for both DM and BP contexts. While this is more or less consistent with the documentation, it may pose a risk regarding backwards compatibility. See ContextLoaderListener#getOsgiApplicationContextCreator(). If no specific creator is configured, it will obtain a default one. The default is then BlueprintContainerCreator in BlueprintLoaderListener. Regards, Olaf Created attachment 223222 [details]
Errors when applying patch
(In reply to comment #17) > Hello Gly > > Thanks for the hint. The integration tests are green: > > Tests run: 229, Failures: 0, Errors: 0, Skipped: 0 Great. > > Concerning the approach: Yes, there is only one common configuration. With > the patch, this configuration is only instantiated once before any load > listeners are instantiated, and then shared. Sounds good, although it is not strictly backward compatible as the configuration is now only instantiated once. I have no real problem with bumping the major version number if necessary to accommodate this change. (Who needs marketing versions anyway?) > > There is only one semantic change in the patch which is easy to revert (and, > to be on the safe side, should perhaps be reverted as long as both BP and DM > are supported at the same time): Should a user provide a custom > OsgiApplicationContextCreator, it will now be used for both DM and BP > contexts. While this is more or less consistent with the documentation, it > may pose a risk regarding backwards compatibility. See > ContextLoaderListener#getOsgiApplicationContextCreator(). If no specific > creator is configured, it will obtain a default one. The default is then > BlueprintContainerCreator in BlueprintLoaderListener. That sounds a bit strange would probablu be better reverted. > > Regards, > Olaf (In reply to comment #18) > Created attachment 223222 [details] > Errors when applying patch I tried applying the patch (so I could read the changes in context more easily) and got these errors. After you have reverted part of the patch as just discussed, please could you check it applies ok and let me know which directory you were in and what parameters you used. Created attachment 223234 [details] Corrected version of the previous patch using GIT against the master (In reply to comment #20) > (In reply to comment #18) > > Created attachment 223222 [details] > > Errors when applying patch > > I tried applying the patch (so I could read the changes in context more > easily) and got these errors. After you have reverted part of the patch as > just discussed, please could you check it applies ok and let me know which > directory you were in and what parameters you used. I created the patch via my IDE, I suppose the result is not compatible with the way git handles patches. So, I've done the following: - Reverted the semantics: the blueprint listener will now always use a blueprint container creator - Applied changes to master (instead of 1.0.2.RELEASE)... - Created a patchfile like so: git format-patch e0d5c341bce6861cbe6a7e0c5b01e4ebc4534e3f --stdout > 356683-against-master.patch Hope this helps, Olaf The patch applies cleanly now, looks good except that the new files are missing license headers, and the tests pass (at least on Equinox). I'm still not sure whether this would be regarded as a breaking change or a bug fix by anyone using extender configuration. It is conceivable that someone actually relies on the configuration being instantiated twice and would break with this change in place. To be on the safe side, I'm leaning towards including this fix in a 2.0 release. The downside of this is that it will require an Eclipse release review which involves a bit of effort on my part and then some elapsed time to go through the process. Also, we should carefully consider if there are any other candidate changes for a 2.0 release and if there are, then that might slow down the production of the release. However, we could spin a 2.0.0.M01 milestone pretty easily to get started and use that for testing. Copying in Aaron (the other committer) for his awareness and input. Meanwhile, Olaf, please could you confirm that you wrote 100% of the code in the patch and that you have the rights to contribute the code (e.g. your employer's permission if that is necessary). Please could you also add license headers to the new files and re-attach the patch. These are all Eclipse IP requirements in order for me to push your patch. (In reply to comment #22) > Meanwhile, Olaf, please could you confirm that you wrote 100% of the code in > the patch and that you have the rights to contribute the code (e.g. your > employer's permission if that is necessary). Please could you also add > license headers to the new files and re-attach the patch. These are all > Eclipse IP requirements in order for me to push your patch. It is necessary to pass patches of > 250 lines of code and configuration through Eclipse Legal, but I estimate you've written fewer LOCs than that. The patch is larger than 250 LOCs simply because you have factored out several nested classes into top-level classes. So once I have your confirmations and the new license headers, I can push your changes to a branch and start working on a 2.0.0.M01. Created attachment 223379 [details]
New patch version including license headers
Hi Glyn
I have added the missing license headers and hereby confirm that I have written 100% of the new code myself and do have the rights to contribute it to this project.
Regards,
Olaf
Thanks Olaf. I reversioned master to 2.0.0.BUILD-SNAPSHOT (after creating a 1.0.x branch,), and committed and pushed your change. It still builds cleanly on Equinox and Felix with the usual situation of broken integration tests on Knopflerfish (not an issue with your change). I create a bug train bugzilla to keep track of the tentative plan for 2.0.0 and will be looking at other bug fixes to include before issuing M01. If you need M01 sooner, please say so. Fixed. Many thanks to Olaf Otto for the contribution! Tested ok on Virgo. |