Bug 346682 - [region] no check for duplicate BSN/version is done on Region.installBundle
Summary: [region] no check for duplicate BSN/version is done on Region.installBundle
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Components (show other bugs)
Version: 3.7   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.7 RC3   Edit
Assignee: equinox.components-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: Documentation
Depends on:
Blocks:
 
Reported: 2011-05-20 10:03 EDT by Thomas Watson CLA
Modified: 2011-05-24 13:24 EDT (History)
1 user (show)

See Also:


Attachments
work in progress (4.45 KB, patch)
2011-05-23 10:27 EDT, Thomas Watson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Watson CLA 2011-05-20 10:03:27 EDT
The javadoc contract of Region.installBundle state:

<javadoc>
If the bundle has the same bundle symbolic name and version as a bundle already present in the region or as a bundle import specified on a connection to another region, then BundleException with exception type DUPLICATE_BUNDLE_ERROR is thrown.
</javadoc>

This is currently not checked.  The framework can disable this check altogether with the configuration property org.osgi.framework.bsnversion=multiple.  But when this is set there is nothing to prevent the same BSN/Version pair from being installed into the same region.

We will have to add code to the Region impl to do this check.  I am not sure what the best approach to this is.  An initial thought is the following would have to be done:

1) In the RegionBundleEventHook.bundleInstalled(Bundle, Bundle) method we are going to have to detect if the region we are about to place the bundle into has visibility to another installed bundle with the same BSN/Version.  If so then we will have to blacklist the bundle being installed so that nobody can see it.

2) In the impl of Region.installBundle we will have to do a post process to see if the installed bundle has been blacklisted, if so then immediately uninstall it and throw a BundleException.

Glyn, WDYT?
Comment 1 Thomas Watson CLA 2011-05-23 10:27:18 EDT
Created attachment 196344 [details]
work in progress

This is work in progress if we decide such a change is a good idea.  Still need to write tests.  But tests get complicated because of the need to set the property org.osgi.framework.bsnversion=multiple.  Will likely have to fire up another framework instance configured properly to test.
Comment 2 Nobody - feel free to take it CLA 2011-05-24 05:40:06 EDT
I agree that it would be good to establish this invariant. Knowing that the combination of region id, bundle symbolic name, and bundle version is sufficient, within a given framework, to uniquely identify a bundle is a very useful property.

The proposed solution seems reasonable.

This will also remove uncertainty from Virgo which is now not in a good position to police this invariant separately from the Equinox region bundle.
Comment 3 Thomas Watson CLA 2011-05-24 08:35:40 EDT
(In reply to comment #2)
> I agree that it would be good to establish this invariant. Knowing that the
> combination of region id, bundle symbolic name, and bundle version is
> sufficient, within a given framework, to uniquely identify a bundle is a very
> useful property.
> 
> The proposed solution seems reasonable.
> 
> This will also remove uncertainty from Virgo which is now not in a good
> position to police this invariant separately from the Equinox region bundle.

Note that this invariant can only be enforced if Region.installBundle is used.  Any code using BundleContext.installBundle will still have the ability to install duplicate BSN/versions.  This is because a hook cannot cause the installation to fail, except in the very special case where a duplicate location string is detected.  In this case should we cause the newly installed bundle from BundleContext.installBundle to be isolated such that nobody can see it.  This would be pretty strange because from the installer's point of view it will look like the bundle got installed fine.
Comment 4 Nobody - feel free to take it CLA 2011-05-24 11:26:58 EDT
In that case, we wouldn't have an invariant at all as it could be broken by BundleContext.installBundle. I don't think a mere pre-condition check on Region.installBundle is worth the overhead.

I don't like the option of hiving off the bundle and pretending the install was ok as that will just cause confusion for users.

So, to implement this bug properly, I think we need a bundle lifecycle hook with the ability to fail bundle install.
Comment 5 Thomas Watson CLA 2011-05-24 11:29:42 EDT
(In reply to comment #4)
> In that case, we wouldn't have an invariant at all as it could be broken by
> BundleContext.installBundle. I don't think a mere pre-condition check on
> Region.installBundle is worth the overhead.

That is where I was headed.  So the fix to this bug for now is to remove the pre-condition check on Region.installBundle javadoc.

> 
> I don't like the option of hiving off the bundle and pretending the install was
> ok as that will just cause confusion for users.
> 
> So, to implement this bug properly, I think we need a bundle lifecycle hook
> with the ability to fail bundle install.

Agreed.  For now we cannot address this issue.  If we feel it should be fixed then we need to get a core enhancement to the spec.
Comment 6 Thomas Watson CLA 2011-05-24 11:35:05 EDT
Sigh, it is too bad we did not think to do the same trick we do for duplicate bundle locations for the duplicate BSN/Version check.  If only we had spec'ed that the framework must call bundle FindHook.find method to determine if the installing context could see the existing bundle with the same BSN/Version as the bundle being installed.

Perhaps that is what we can do when/if the specification decides to make org.osgi.framework.bsnversion=multiple as the default.
Comment 7 Nobody - feel free to take it CLA 2011-05-24 12:35:05 EDT
(In reply to comment #6)
> Sigh, it is too bad we did not think to do the same trick we do for duplicate
> bundle locations for the duplicate BSN/Version check.  If only we had spec'ed
> that the framework must call bundle FindHook.find method to determine if the
> installing context could see the existing bundle with the same BSN/Version as
> the bundle being installed.
> 
> Perhaps that is what we can do when/if the specification decides to make
> org.osgi.framework.bsnversion=multiple as the default.

Let's discuss this topic in the CPEG.

Meanwhile, I agree we should fix this bug by changing the javadoc.
Comment 8 Thomas Watson CLA 2011-05-24 13:24:36 EDT
I removed the javadoc statement about checking for duplicate bsn/version.

(In reply to comment #7)
> Let's discuss this topic in the CPEG.

I opened CPEG bug https://www.osgi.org/members/bugzilla/show_bug.cgi?id=2020