Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 332771

Summary: Resolver hook not driven for State resolution operations
Product: [Eclipse Project] Equinox Reporter: Glyn Normington <glyn.normington>
Component: FrameworkAssignee: Thomas Watson <tjwatson>
Status: RESOLVED FIXED QA Contact:
Severity: major    
Priority: P3 CC: dmitry, hargrave, tjwatson
Version: 3.7   
Target Milestone: 3.7 M5   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 330776, 332923    
Attachments:
Description Flags
New API for setResolverHookFactory none

Description Glyn Normington CLA 2010-12-16 12:18:09 EST
Virgo uses a snapshot of the framework state manipulated via the State abstraction and associated classes in order to calculate missing dependencies and provision them. In the successful case, Virgo installs all the needed bundles into the framework and resolves again (at which point the resolver hook *is* driven). In the failure case, Virgo dumps the state to disk for offline analysis and does not update the framework.

Since the resolver hook is not driven when Virgo uses the State to perform resolutions, wires are created freely between bundles in the kernel and user region which would be prevented by the resolver hook. Typically this results in a bunch of uses constraint violations in the State because there are two instances of Spring framework present (one in the kernel and one in the user region, but both visible in the State).
Comment 1 Thomas Watson CLA 2010-12-16 12:30:44 EST
I think the most simple solution would be to allow a ResolverHookFactory to be somehow passed when you create the snapshot State.  Your ResolverHookFactory could then delegate to the hooks you need for isolation.

Glyn, there is an internal method org.eclipse.osgi.internal.resolver.StateImpl.setResolverHookFactory(ResolverHookFactory) that you could try to use in the mean time to see if this idea works.
Comment 2 Thomas Watson CLA 2010-12-16 12:34:02 EST
I forgot to mention the class org.eclipse.osgi.framework.internal.core.CoreResolverHookFactory

An instance of this class is used by the framework for the framework's State.  It mainly does two things

1) creates shrinkable collections to prevent hooks from doing additions
2) delegates to the service registry to find the available hooks to call.  It of coarse uses logs of registry internals to do this so that service hooks cannot hide registry hooks from the framework itself.
Comment 3 Glyn Normington CLA 2010-12-16 13:26:32 EST
Thanks Tom. I can certainly try setResolverHookFactory, but I thought from our earlier discussion that it would be tricky to pass BundleRevision and Capability instances when running a State disconnected from the framework. What's your thinking on that?
Comment 4 Thomas Watson CLA 2010-12-16 13:57:35 EST
The Descriptions from the State API (BundleDescription, ExportPackageDescrption etc) have a getCapability() method that the resolver implementation calls for the Capability objects and BundleDescription implements BundleRevision.  The only thing that will not work (it will return null) is if you call BundleRevision.getBundle() on such a state that is not associated with the actual framework.

That is my theory anyway.  I would like you to give it a try and see if you hit any road blocks.
Comment 5 Glyn Normington CLA 2010-12-17 03:51:11 EST
Will do. Thanks.
Comment 6 Glyn Normington CLA 2010-12-17 07:20:29 EST
setResolverHookFactory seems to do the trick. :-)

The messiest part was getting hold of the bundle id of a bundle revision as in the following code:

private boolean isMember(BundleRevision bundleRevision) {
        Bundle bundle = bundleRevision.getBundle();
        if (bundle != null) {
            return this.regionMembership.contains(bundle);
        }
        if (bundleRevision instanceof ResolverBundle) {
            ResolverBundle resolverBundle = (ResolverBundle)bundleRevision;
            return this.regionMembership.contains(resolverBundle.getBundleDescription().getBundleId());
        }
        Assert.isTrue(false, "Cannot determine region membership of BundleRevision '%s'", bundleRevision);
        // Assume that bundle revisions with no bundles belong to the user region.
        return true;
}

Perhaps getBundleId could be added to the BundleRevision interface as part of the fix to this bug.

The full set of code changes for the experiment is in this commit:

http://git.eclipse.org/c/virgo/org.eclipse.virgo.kernel.git/commit/?h=bug330776-framework-hooks&id=b0b9515da0abc922d60634ae5f56866fbe6978bd

BTW I didn't try using CoreResolverHookFactory since it didn't seem to add much value to the experiment and I couldn't easily get hold of the service registry constructor parameter.
Comment 7 Thomas Watson CLA 2010-12-17 09:04:32 EST
(In reply to comment #6)
> 
> Perhaps getBundleId could be added to the BundleRevision interface as part of
> the fix to this bug.

hmmm, that is OSGi API, unfortunately I'm not sure there is justification.  The Bundle from the BundleReference should be used to get the id (but you don't have one).  I doubt we can argue to add getBundleID.  It would be the same as asking for BundleRevision to have a getBundleLocation() method.  Where does it end for copying methods from Bundle to BundleRevision?  The only reason getSymbolicName and getVersion and getTypes exist on the BundleRevision is because these could actually be different than the current revision used by the Bundle object.
bug330776-framework-hooks&id=b0b9515da0abc922d60634ae5f56866fbe6978bd

> 
> BTW I didn't try using CoreResolverHookFactory since it didn't seem to add much
> value to the experiment and I couldn't easily get hold of the service registry
> constructor parameter.

Sorry to have mislead you there.  I trying to be helpful in pointing you to some code that delegates to a set of hooks.  But as you found out it has its hands in too many internals to be of much help.
Comment 8 Glyn Normington CLA 2010-12-17 09:34:08 EST
(In reply to comment #7)
> hmmm, that is OSGi API, unfortunately I'm not sure there is justification.  The
> Bundle from the BundleReference should be used to get the id (but you don't
> have one).  I doubt we can argue to add getBundleID....

Agreed.

> > 
> > BTW I didn't try using CoreResolverHookFactory since it didn't seem to add much
> > value to the experiment and I couldn't easily get hold of the service registry
> > constructor parameter.
> 
> Sorry to have mislead you there.  I trying to be helpful in pointing you to
> some code that delegates to a set of hooks.  But as you found out it has its
> hands in too many internals to be of much help.

No problem.
Comment 9 BJ Hargrave CLA 2010-12-17 09:34:34 EST
(In reply to comment #7)
> (In reply to comment #6)
> > 
> > Perhaps getBundleId could be added to the BundleRevision interface as part of
> > the fix to this bug.
> 
> hmmm, that is OSGi API, unfortunately I'm not sure there is justification.  The
> Bundle from the BundleReference should be used to get the id (but you don't
> have one). 

In the o.o.f.wiring package, BundleRevision.getBundle() can never return null. The o.o.f.wiring package is for an after-the-fact examination of the work product of the resolver. That is, the resolved bundles. An installed but not yet resolved bundle would not have a BundleWiring, but would have a BundleRevision which points back to the Bundle.

I imagine the issue here relates to some internal state within the framework resolver where a BundleRevision is yet to have a Bundle object. But it does seem odd that a BundleRevision might not have a Bundle object?
Comment 10 Thomas Watson CLA 2010-12-18 09:31:59 EST
(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > 
> > > Perhaps getBundleId could be added to the BundleRevision interface as part of
> > > the fix to this bug.
> > 
> > hmmm, that is OSGi API, unfortunately I'm not sure there is justification.  The
> > Bundle from the BundleReference should be used to get the id (but you don't
> > have one). 
> 
> In the o.o.f.wiring package, BundleRevision.getBundle() can never return null.
> The o.o.f.wiring package is for an after-the-fact examination of the work
> product of the resolver. That is, the resolved bundles. An installed but not
> yet resolved bundle would not have a BundleWiring, but would have a
> BundleRevision which points back to the Bundle.
> 
> I imagine the issue here relates to some internal state within the framework
> resolver where a BundleRevision is yet to have a Bundle object. But it does
> seem odd that a BundleRevision might not have a Bundle object?

BJ, this is a case where glyn is using the Equinox resolver APIs to create a State that is disconnected from the framework so it has no Bundle objects associated with it.  Glyn is doing a sandbox resolution so the resolver does not have access to the real Bundle Objects.
Comment 11 Thomas Watson CLA 2010-12-18 09:35:52 EST
(In reply to comment #6)
> setResolverHookFactory seems to do the trick. :-)
> 
> The messiest part was getting hold of the bundle id of a bundle revision as in
> the following code:
> 
> private boolean isMember(BundleRevision bundleRevision) {
>         Bundle bundle = bundleRevision.getBundle();
>         if (bundle != null) {
>             return this.regionMembership.contains(bundle);
>         }
>         if (bundleRevision instanceof ResolverBundle) {
>             ResolverBundle resolverBundle = (ResolverBundle)bundleRevision;
>             return
> this.regionMembership.contains(resolverBundle.getBundleDescription().getBundleId());
>         }
>         Assert.isTrue(false, "Cannot determine region membership of
> BundleRevision '%s'", bundleRevision);
>         // Assume that bundle revisions with no bundles belong to the user
> region.
>         return true;
> }
> 

Glyn, I just released some changes to HEAD to make ALL BundleRevision objects passed to the hooks to be the actual BundleDescription objects since a BundleDescription implements BundleRevision.  So you hack should be much better by simply trying to cast it to BundleDescription if getBundle returns null.
Comment 12 Glyn Normington CLA 2010-12-20 03:56:22 EST
Great. We'll pick this up in due course.
Comment 13 Glyn Normington CLA 2010-12-22 06:24:20 EST
I raised the closely related bug 333071 separately for clarity.
Comment 14 Thomas Watson CLA 2011-01-04 15:50:23 EST
Glyn, is there any update on the experiment?  It seems rather successful.  I am wondering if we should consider making State.setResolverHookFactory(ResolverHookFactory) API?

The implementation should probably fail with an IllegalStateException if the hook has already been set to a non-null value.  Otherwise we run the risk of letting someone changing the factory used by the framework resolver.
Comment 15 Glyn Normington CLA 2011-01-05 03:35:08 EST
(In reply to comment #14)
> Glyn, is there any update on the experiment?  It seems rather successful.  I am
> wondering if we should consider making
> State.setResolverHookFactory(ResolverHookFactory) API?

I agree it's a good approach and that method could be made an API.

> 
> The implementation should probably fail with an IllegalStateException if the
> hook has already been set to a non-null value.  Otherwise we run the risk of
> letting someone changing the factory used by the framework resolver.

Agreed.
Comment 16 Thomas Watson CLA 2011-01-05 11:36:25 EST
Created attachment 186100 [details]
New API for setResolverHookFactory

Here is a patch for the new API.  It will throw an IllegalStateException if the factory is already set.  This behavior surfaced a bug in the framework for restarting.  The BaseStorage was not correctly nulling out some objects.  This left the framework in a stale state for restarts.
Comment 17 Thomas Watson CLA 2011-01-05 11:39:16 EST
Patch released.