Bug 209432 - Issue a warning when "Bundle-ActivationPolicy: lazy" adds no value
Summary: Issue a warning when "Bundle-ActivationPolicy: lazy" adds no value
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.4 M5   Edit
Assignee: bartosz michalik CLA
QA Contact:
URL:
Whiteboard:
Keywords: bugday, contributed
Depends on:
Blocks: 214156
  Show dependency tree
 
Reported: 2007-11-11 15:28 EST by Simon Archer CLA
Modified: 2008-01-02 17:37 EST (History)
3 users (show)

See Also:


Attachments
mylyn/context/zip (738 bytes, application/octet-stream)
2007-11-11 15:59 EST, Chris Aniszczyk CLA
no flags Details
lazy loading warning when there is no activator definied (5.32 KB, patch)
2008-01-02 15:11 EST, bartosz michalik CLA
no flags Details | Diff
mylyn/context/zip (1.65 KB, application/octet-stream)
2008-01-02 16:09 EST, Chris Aniszczyk CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Archer CLA 2007-11-11 15:28:05 EST
People love to be lazy.  They love laziness so much that they often add "Bundle-ActivationPolicy: lazy" to their bundle manifests without understanding when it is meaningful.  I'd like to see a warning issued by PDE when this header/value is used but:

1.  The bundle does NOT export any packages via the Export-Package header.
2.  The bundle does NOT define a bundle activator via the Bundle-Activator header.

If my understanding is correct, both 1 and 2 must be true, otherwise the header
Bundle-ActivationPolicy with the value "lazy" is meaningless.

The most important of these is 1, since without an exported package there will be no types visible for other bundles to "touch" to cause the bundle to be lazily started.  

Regarding 2, without a bundle activator defined in the manifest starting a bundle has no effect, apart from changing its state to ACTIVE.  So you could argue that is it not absolutely necessary to have a bundle activator defined in the manifest, but I cannot imagine anyone wanting/needing to lazy start a bundle without a bundle activator just to have the bundle's state change to ACTIVE.

Along with the warning, I'd like to see any applicable quick fixes.  The most reasonable quick fix I can think of would be "Remove header", since resolving 1 and 2 are probably too complicated for a quick fix.
Comment 1 Chris Aniszczyk CLA 2007-11-11 15:50:04 EST
Let's see what Tom the OSGi oracle has to say about this one.
Comment 2 Chris Aniszczyk CLA 2007-11-11 15:58:36 EST
If we do this, we should consider finalizing bug 163707.

Also, would this fix apply to the other "lazy" headers?
Comment 3 Chris Aniszczyk CLA 2007-11-11 15:59:06 EST
adding a Mylyn context for the brave
Comment 4 Chris Aniszczyk CLA 2007-11-11 15:59:12 EST
Created attachment 82624 [details]
mylyn/context/zip
Comment 5 Thomas Watson CLA 2007-11-11 19:33:50 EST
(In reply to comment #0)
> 1.  The bundle does NOT export any packages via the Export-Package header.
I do not agree.  Bundle.loadClass can be used to load classes out of packages which are not exported.  This is what things like DS and the extension registry use to load implementation classes.  It is reasonable to want a bundle to be lazily activated the first time an internal class is loaded.


> 2.  The bundle does NOT define a bundle activator via the Bundle-Activator
> header.
I agree with this one.
Comment 6 Simon Archer CLA 2007-11-11 20:36:31 EST
Regarding comment 5...

>Bundle.loadClass can be used to load classes
>out of packages which are not exported. 

That's very interesting Tom, thanks for the insight.  But these sorts of things tend to keep me awake at night...  Since every Bundle is visible to every other Bundle, that means that any Bundle can call loadClass(String) on any other Bundle, passing in the fully qualified name of any type that is contained in the receiver Bundle.  That seems like a huge hole in the visibility rules.

So I tried it out (using Equinox 3.3) but found that while I could load a class fine, I was unable to anything useful with it.  Calling newInstance(), for example, was only possible if the class had a default constructor, but even then I always got java.lang.NoClassDefFoundError when I tried.  While there are certainly other reflection APIs that I've not tried, I would expect similar exceptions to be thrown.  I even added a "Dynamic-ImportPackage: *" header to relax the visibility rules.

The likes of DS aside for a moment, for the average developer, specifying Bundle-ActivationPolicy with the value "lazy", WITHOUT exporting any packages is generally a mistake and adds no value.  I see this a lot these days since people don't really stop to think what lazy activation means, but rather have heard somewhere that laziness is a virtue.

Back to my findings: Are you absolutely sure that DS use the technique you describe, since I could not do anything useful with the non-exported Class once loaded.  Thanks in advance for any additional insight you might have.
Comment 7 Thomas Watson CLA 2007-11-11 21:05:42 EST
(In reply to comment #6)
> Back to my findings: Are you absolutely sure that DS use the technique you
> describe, since I could not do anything useful with the non-exported Class once
> loaded.  Thanks in advance for any additional insight you might have.
> 

Absolutely positive.  DS/Spring/IPojo/ExtensionRegistry etc all use this approach.  The extender model is the key reason we added Bundle.loadClass.
Comment 8 Thomas Watson CLA 2007-11-11 21:33:04 EST
I was a bit terse in the last response.  Did not mean to brush your concerns aside Simon :)

Some may view the Bundle.loadClass as a hole in the visibility rules in OSGi.  But I do not agree.  A bundle's individual classloader still does not have access to things outside of its own constraints and private class space.  This is why you are getting NoClassDefFound errors.  I suspect you are trying to cast the result of newInstance to the actual type which you do not have access to.  In extender models like DS and the extension registry.  Typically the extender framework is responsible for constructing object and making it avaible to clients.  For example, to the service registry for DS and from createExecutableExtension for the extension registry.  The contract for the service is in the service interface.  The client casts the result to the service interface (which is shared between the implementor and the client) not the implementation class (which the client does not have access to).

Here is another thing to keep you up at night Simon, take a look at the new method  Bundle.getBundleContext() ;-)  This method was added because DS/Spring needed a framework independent way to register services on behave of another bundle.  The theory is security manager should prevent unprivileged bundles from using these types of methods.  After all, which is more dangerous Bundle.uninstall or Bundle.loadClass?  In the end, OSGi needs these types of methods for frameworks/platforms which are built on top of OSGi.  Unfortunately I don't see this trend slowing down.  The EEG is now talking about adding hooks into the service registry to allow its behavior to be modified by entities outside the framework.  On one hand, this is great because it opens up the framework and tries to prevent any more from getting pushed down.  Instead we have the proper hooks in place to allow others outside the framework to do the work.  This way when someone asks us to implement some crazy requirement we can simply say nope, but you can do it yourself if you really want to.  But on the other hand opening up the framework may prove to be unstable.  Now a framework will behave differently depending on what bundles are installed which hook into it.  I realize that I have not gone completely off topic and will hang up now, just thought I would share some reasons I stay awake at night ;-)
Comment 9 Simon Archer CLA 2007-11-11 22:31:20 EST
Thank you Tom for the ever insightful update.  Yes, you are correct, I was casting to the implementation class, and now I understand how DS and co. make use of loadClass(String) by simply treating the Object in some abstract way.

Yes, I too felt uneasy about Bundle.getBundleContext() when I first learned about it.  I remember the days when your BundleContext was sacred and should never be shared, but clearly those days are past.  I guess turning on security is the answer to most of these cases, but I don't know anyone that does.  I actually make use of this API to register OSGi services based on contributions made to an Eclipse extension point, so I appreciate its uses.

Back on topic now: It sounds to me as if a warning IS appropriate for the case where a bundle:

- Has the header Bundle-ActivationPolicy with the value "lazy", but
- Does NOT have the header Bundle-Activator set.

This has been an interesting discussion, thanks.
Comment 10 Chris Aniszczyk CLA 2007-11-12 08:40:10 EST
The mylyn context still applies and patches are welcomed :)
Comment 11 Thomas Watson CLA 2007-11-12 13:18:37 EST
(In reply to comment #9)
> Back on topic now: It sounds to me as if a warning IS appropriate for the case
> where a bundle:
> 
> - Has the header Bundle-ActivationPolicy with the value "lazy", but
> - Does NOT have the header Bundle-Activator set.

+1 this also applies to any the old lazy activation headers (Eclipse-AutoStart, Eclispe-LazyStart)

Comment 12 bartosz michalik CLA 2008-01-02 15:11:32 EST
Created attachment 86017 [details]
lazy loading warning when there is no activator definied 

the comment string is not extracted to the properties
Comment 13 Chris Aniszczyk CLA 2008-01-02 15:36:42 EST
Thanks Bartosz ;)

I'll look at this ASAP. This coincides with the please Simon Archer theme we have in PDE for the 3.4 release.
Comment 14 Chris Aniszczyk CLA 2008-01-02 15:56:30 EST
looking at this now
Comment 15 Chris Aniszczyk CLA 2008-01-02 16:09:22 EST
Ok, I externalized the string and all that good stuff.

Thanks Bartosz!
Comment 16 Chris Aniszczyk CLA 2008-01-02 16:09:27 EST
Created attachment 86023 [details]
mylyn/context/zip
Comment 17 Chris Aniszczyk CLA 2008-01-02 17:37:10 EST
Bartosz, I opened bug 214156 if you're interested in also supplying a quickfix :)