Community
Participate
Working Groups
I have plugin com.ex.A with a class LoadStuff with a static method that does: class.getClassLoader().getResourceAsStream(resource) and has Eclipse-BuddyPolicy: registered. I have plugin com.ex.B with a Worker class that calls into LoadStuff to load a resource. It depends on com.ex.A. I have a UI plugin com.ex.C that depends on com.ex.B, instantiates Worker and passes in the resource /properties/stuff.properties, which is contained in C, and has Eclipse-RegisterBuddy: com.ex.A It won't load the resource unless I make com.ex.C depend on com.ex.A. It seems that you not only need the buddy registration but also a direct dependancy. Then it works. But then if I change the policy from registered to global, it stops working. I'll attach 3 project example. PW
Created attachment 33700 [details] 3 plugins that won't load a resource
Created attachment 33758 [details] GlobalPolicy#loadResources(String) Patch This patch should resolve this bug, as well as fix some more of the issues in bug 110712, when a budy policy of "global" is used. This patch only affects the class org.eclipse.osgi.framework.internal.core.GlobalPolicy. The patch was made against the R3_1_maintenance branch/tag. I don't know how how different the 3.2 code is, but it should be easy to port. The issues I found are that the loadResource and loadResources implementations were using the utility method BundleLoader.getPackageName, when they should have been using the BundleLoader.getResourcePackageName. The second issue is that the loadResources method was only looking up one ExportedPackage, the first that matched, instead of all that matched. This makes sense for loadClass and loadResource, but not for loadResources.
thanks for the patch. We'll take a look
The patch looks good. I would favor the usage of Vector instead of ArrayList when collecting the results, so that you get a enumeration immediatly. Could you please provide a patch to the other policies provided since they all suffer from the same problem?
There are pieces of this fix for RegisteredPolicy in bug 110712. I can post those fixes here as well. I can work out another patch for the DependentPolicy fairly easily. Should the patch be against HEAD or the R3_1_maintenance branch? I know there's a refactoring with Equinox, but I didn't know that has affected this particular piece of code too much. Additionally, assuming there is going to be another 3.1 maintenance release, I personally need this in that branch. As for the ArrayList vs. Vector - I can convert it to use Vectors without issue. As a habit, I prefer to use ArrayLists instead since Vector's are internally synchronized, but I can convert it to keep it consistent. Thanks.
Created attachment 33797 [details] GlobalPolicy.java Patch (R3_1_maintenance) Patch for GlobalPolicy to retrieve all resources and use Vectors.
Created attachment 33798 [details] DependentPolicy.java Patch (R3_1_maintenance) Patch for DependentPolicy.java to retrieve all resources properly. Additionally, I've cleaned up this class a little bit, so that the thread synchronization is correct and safe. Previously, there was only one method that was synchronized, but this method alone wouldn't protect the data in all scenarios. I've just synchronized the public methods to make it more straight-forward. If this is seen as a bottle neck, then a read/write mutex or something similar could be used. The other way to resolve this would be to eliminate all synchronization and convert the lazy loading to upfront loading in the constructor.
Created attachment 33800 [details] RegisteredPolicy.java Patch (R3_1_maintenance) A patch for RegisteredPolicy.java to load all resources. I've reworked this class slightly. Originally it was extending DependentPolicy, but this seemed awkward and not altogether appropriate (it didn't seem to pass the "is-a" test for extension). As such, I just remove the extension and implemented IBuddyPolicy directly and pulled up the small portion of initialization code that it was borrowing. I believe this makes the initialization (constructor) a bit cleaner. Also, I eliminated the cast to AbstractBundle and just used the methods that were available, so as to preserve some isolation.
Created attachment 33801 [details] BundleLoader.java Patch (R3_1_maintenance) A patch for BundleLoader to load all resources. This was also posted on bug 110712, but seems most appropriate here now.
Created attachment 33802 [details] PolicyHandler.java Patch (R3_1_maintenance) A patch for PolicyHandler to load all resources. This was also posted on bug 110712, but seems more appropriate here now.
*** Bug 110712 has been marked as a duplicate of this bug. ***
As an aside, but somewhat related, question, how are plug-in fragments supposed to affect the Buddy Loading policies, if at all? I ran a test with a host plugin that has no buddy policy and then put a buddy policy of global in a fragment. The fragment code was unable to load an exported class initially. When I moved the global policy from the fragment to the host, the fragment was able to load the class. According to the documentation, it seems that you can have multiple policy values, so I was wondering if policies declared in a fragment should be appended to the policies of the host.
Given the way fragments are handled by osgi, that would make sense. Do you want to prototype? Tom can you think of any particular problem that this could cause?
Can buddy policies collide with each other? For example, if one fragment specifies one policy and another fragment specifies another policy? I don't think this is really an issue because the host can specify all supported policies in a list today. Fragments should just add to the list of policies specified by the host and we should ensure that duplicate policies are not added to the host list. This should be isolated to the BundleLoader class if someone is willing to prototype. I suggest a separate bug report be opened for this.
On the patches: - The BundleLoader#getResources can't be changed since this behavior is specified. The patch won't be applied. - PolicyHandler should provide use Vector - I don't see the point of the complete reorg of DependentPolicy and RegisteredPolicy. I was expecting change only in getResources().
Created attachment 34120 [details] PolicyHandler.java Patch (R3_1_maintenance) Converted PolicyHandler to use Vector instead of ArrayList. Any reason for using Vectors instead of ArrayLists? I'm assuming it's not because the Collections APIs are trying to be avoided, as this class uses Sets and HashSets already. Vectors are so much slower than ArrayLists, due to synchronization.
(In reply to comment #15) > - I don't see the point of the complete reorg of DependentPolicy and > RegisteredPolicy. I was expecting change only in getResources(). > Several reasons actually, most of which I mentioned in comment #7 and comment #8. The major reason for the changes to DependentPolicy are because it was using a lazy load mechanism and since there was at least one piece of syncrhonization and because this is a utility for class loading I assumed it must be thread safe. The current implementation was only synchronizing a single method ('addDependent'), but this wasn't completely protecting all of the fields that were being read and written. It looked like there could be conditions where a read/write race might occur. Perhaps my analysis was incorrect. As for RegisteredPolicy, the implementation was extending from DependentPolicy. This seemed awkward as it doesn't seem to pass the "is-a" test; "RegisteredPolicy is-a DependentPolicy". There is no mention in the documention that for a registered bundle to be in the class loader, it must make a dependency as well. If it doesn't need to have a dependency, then further changes probably need to be made to look at all bundles, not just dependents. Additionally, and the better reason, the only thing that RegisteredPolicy was really using from DependentPolicy was the loading of the BundleDescriptions, but even after it utilized this it looped back through all of the descriptions to eliminate all dependents that weren't registered. This seemed liked a waste of cycles. Another small change was to eliminate the need to cast the Bundle to an AbstractBundle to get the manifest data; I changed it to use the public interface. I apologize if I over stepped some bounds, but I thought I could provide some additional cleanup for stability and performance. If you'd like, I can easily provide a different set of patches for these files that maintains the original implementations.
(In reply to comment #15) > On the patches: > - The BundleLoader#getResources can't be changed since this behavior is > specified. The patch won't be applied. The change is actually to "findResources", correct? I just want to make sure we're discussing the same thing. Without the changes to BundleLoader, the other patches won't really do much at all. The problem is that findResources is currently acts as through we're only finding a single resource or a single class, meaning that you won't need to find the very first resource you encounter. This does not comply with the definition of the ClassLoader#getResources [1], nor what I would consider the spirit of the buddy class loading. The specific case that I've run into is that the plugin that declares a buddy policy of any type and exposes a package that is also exposed by another plugin. With the current implementation, the exposed packaged in the first plugin will be found and no other bundles will be searched. Here's a layout of what I'm discussing. Plug-in A - Export-Package: org.ex.StuffLoader, properties - Eclipse-BuddyPolicy: registered The class StuffLoader loads ALL resources using ClassLoader.getResources("properties/stuff.properties"). This project contains the resource "properties/stuff.properties" with default data. Plug-in B - Export-Package: properties - Eclipse-RegisterBuddy: A This plug-in uses StuffLoader and contains a resource "properties/stuff.properties" with some overriding data. Given the current implementation of BundeLoader, the search would stop as soon as the exported package in plug-in A was found. Additionally, this would fail even if plug-in A didn't have a file with the same URI, but still exported that package because it has other resources in their as well. If this is the defined behavior of BundleLoader or more specifically the ClassLoaderDelegate, then I would argue that the defined behavior is incorrect and should be fixed. The ClassLoader#getResources() [1] must return ALL resources available for a given URI. [1] http://java.sun.com/j2se/1.4.2/docs/api/java/lang/ClassLoader.html#getResources(java.lang.String)
Changing bundleLoader would probably make us fail the conformance tests since this is the class that implements the lookup defined in osgi. Buddy loading comes as an extension of the standard osgi lookup when none of the osgi lookup have found a class/resource. Therefore we have the liberty of changing it to return all the resources. For questions on the osgi spec I recommend you to post your questions on the osgi-dev mailing list from the osgi consortium: http://bundles.osgi.org/mailman/listinfo/osgi-dev
Created attachment 34153 [details] BundleLoader.java Patch (R3_1_maintenance) I reviewed my changes and they did go over board. This patch should keep the defined OSGi behavior intact and enable the buddy loading extension to still work. This patch maintains the original algorithm, but always checks for a buddy policy and adds any resources to the original results. During my tests, this resolves the issues that I've mentioned earlier.
Any thoughts or comments on the latest patch? I don't want to be a bother, but I have some project designs on hold that depend on the finalization of this functionality. Thanks.
We will try to have something for you in M5 ... I have not looked at the patches myself yet.
Ok, the original problem reported by Paul is completely different than the one trying to be addressed by the various patches attached to this bug report. The original problem that can be recreated by the project in attachment 33700 [details] is a problem with loading a single resource by using getResource not getResoures (plural). Here the registered policy is actually working as designed but it is probably not documented clearly what the registered policy does. In the comment of the RegisteredPolicy class we have the following: Registered policy is an implementation of a buddy policy. It is responsible for looking up a class in the bundles (registrant) that declare interest in the bundle that require the buddy loading. Note that the registrants must have a direct dependency on the bundle needing buddy. So you must not only register with the bundle needing the buddy but you must also be dependent on the bundle directly. The sentence "Note that the registrants must have a direct dependency on the bundle needing buddy." should probably be added to the documentation of "registered" buddy policy. If this was the only thing contained in this bug report then I would close this as invalid. But starting in comment 2 Nathan raised a separate issue having to do with delegating to the buddy policy all the time for getResources (plural) calls even when resources were found in the normal delegation. It is debatable if this is the correct coarse of action because buddies are only supposed to be delegated to if the class/resource could not be found under normal delegation. In this case the patches are changing this behavior for getResources to always delegate to buddies even when resources are available under normal delegation. The latest patch to BundleLoader still changes the defined OSGi behavior for Import-Package. If a package is imported we should not look anywhere else except the exporter of the package. We MUST return the result directly from the exporter if the package is imported. The current patch changes the behavior to always look locally and in buddies when the package is imported, neither should be asked if the package is imported. I do not have a particular problem with the other changes provided by the patches. In a way it does seem to make sense to search all places possible (including buddies) when getting an exhaustive list of URLs to a particular resource. I will attach a new patch that I think is acceptable (does not change Import-Package behavior), but given the relative sensitivity of this change I will need at least two other committers to approve before I will consider releasing. Therefore I'm slipping the Milestone to M6. You made your patches against 3.1_maintenance. This type of change is not a good candidate for a point release (i.e. for 3.1.3).
Created attachment 34446 [details] Combined patch to all files I combined all changes into a single patch. I reverted RegisteredPolicy back to extend DependentPolicy to limit the code changes just to the issue at hand. I cleaned up a few things in the patch. One functional thing that I found to be a problem was in DependentPolicy#loadResources. The original patch only seemed to add all dependents only if the result was not null from one of the dependents. This would seem to leave some resources out if the full depenency list was not completely built during the call the getResources. The current patch will ensure all dependents (immediate and recursive) are search during the getResources call.
DJ and Pascal. Please give the patch a thorough review. I'm slipping to M6 unless you both give a +1 in time for M5. Nathan, please make sure the DependentPolicy change still works in your env given the changes I made to ensure all dependents are searched. Thanks.
(In reply to comment #25) > Nathan, please make sure the DependentPolicy change still works in your env > given the changes I made to ensure all dependents are searched. Thanks. > Yes, this patch still still works for me and since I'm not using any import package or dynamic import statements, the functionality is exactly what I needed.
Pascal, can you review the latest patch. Need to decide if this is required for M6.
I'm a bit worried by the change in bundleloader by which we always consult the buddy loading, but aside from that the patch looks good and should be tried out for M6.
Patch released for M6.
[contributed patch applied]
adding "contributed" keyword to patches contributed by the community.
