Bug 117418 - NPE in preferences
Summary: NPE in preferences
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Compendium (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.2 M6   Edit
Assignee: equinox.compendium-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
: 122762 126297 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-11-21 22:22 EST by Jeff McAffer CLA
Modified: 2006-08-23 15:20 EDT (History)
7 users (show)

See Also:


Attachments
Patch to org.eclipse.equinox.preferences (3.12 KB, patch)
2006-01-05 17:53 EST, Roy Paterson CLA
no flags Details | Diff
Improvement to orginal org.eclipse.equinox.preferences patch (4.78 KB, patch)
2006-01-09 17:32 EST, Simon Kaegi CLA
no flags Details | Diff
less kludge for the registry portion (5.32 KB, patch)
2006-01-17 22:01 EST, Simon Kaegi CLA
no flags Details | Diff
less kludge for the registry portion 2 (2.44 KB, patch)
2006-01-19 23:43 EST, Simon Kaegi CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff McAffer CLA 2005-11-21 22:22:04 EST
when starting the new runtime the following NPE was observed.   It likely happened because the registery service was not registered.  The code should be tightened up to handle this case.

java.lang.NullPointerException
 at org.eclipse.core.internal.preferences.PreferencesService.getPrefExtensions(PreferencesService.java:1134)
 at org.eclipse.core.internal.preferences.PreferencesService.initializeScopes(PreferencesService.java:91)
 at org.eclipse.core.internal.preferences.PreferencesService.<init>(PreferencesService.java:170)
 at org.eclipse.core.internal.preferences.PreferencesService.getDefault(PreferencesService.java:83)
 at org.eclipse.core.internal.preferences.Activator.registerServices(Activator.java:57)
 at org.eclipse.core.internal.preferences.Activator.start(Activator.java:40)
 at org.eclipse.osgi.framework.internal.core.BundleContextImpl$2.run(BundleContextImpl.java:994)
 at java.security.AccessController.doPrivileged(Native Method)
 at org.eclipse.osgi.framework.internal.core.BundleContextImpl.startActivator(BundleContextImpl.java:988)
 at org.eclipse.osgi.framework.internal.core.BundleContextImpl.start(BundleContextImpl.java:969)
 at org.eclipse.osgi.framework.internal.core.BundleHost.startWorker(BundleHost.java:316)
 at org.eclipse.osgi.framework.internal.core.AbstractBundle.resume(AbstractBundle.java:328)
 at org.eclipse.osgi.framework.internal.core.Framework.resumeBundle(Framework.java:1026)
 at org.eclipse.osgi.framework.internal.core.StartLevelManager.resumeBundles(StartLevelManager.java:573)
 at org.eclipse.osgi.framework.internal.core.StartLevelManager.incFWSL(StartLevelManager.java:495)
 at org.eclipse.osgi.framework.internal.core.StartLevelManager.doSetStartLevel(StartLevelManager.java:275)
 at org.eclipse.osgi.framework.internal.core.StartLevelManager.dispatchEvent(StartLevelManager.java:455)
 at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:189)
 at org.eclipse.osgi.framework.eventmgr.EventManager$EventThread.run(EventManager.java:291)
Comment 1 Thomas Watson CLA 2006-01-05 11:36:29 EST
*** Bug 122762 has been marked as a duplicate of this bug. ***
Comment 2 Thomas Watson CLA 2006-01-05 16:08:01 EST
see bug 118471 which is a consequence of this NPE in preferences activator.
Comment 3 Roy Paterson CLA 2006-01-05 17:52:00 EST
This exception is happening because the Preferences bundle is starting before the Registry bundle.  In the Eclipse SDK the startlevel values are configured to make sure this doesn't happen, but the Equinox launcher defaults all startlevels to the same value (4) and Preferences is alphabetically before Registry.

The solution is to only register the Preferences service if the Registry service is available.
Comment 4 Roy Paterson CLA 2006-01-05 17:53:38 EST
Created attachment 32555 [details]
Patch to org.eclipse.equinox.preferences

This patch uses ServiceTracker in Preferences' activator to only register Preferences Service if there is a Registry Service available.
Comment 5 Thomas Watson CLA 2006-01-05 18:08:50 EST
Simon, can you try out this patch?  It should fix bug 118471 as well.  Thanks.
Comment 6 Simon Kaegi CLA 2006-01-06 00:54:25 EST
(In reply to comment #5)
> Simon, can you try out this patch?  It should fix bug 118471 as well.  Thanks.

It's a step in the right direction, but unfortunately no.
I get a swallowed NPE in PreferencesService.getPrefExtensions() when RegistryFactory.getRegistry is called.

The defaultRegistryProvider is set after the registry service is registered and the ServiceTracker triggered.

--
Also, bug 118471 is related but not caused by the NPE.
I think that there are a number of lifecycle bundle start/stop problems that still need looking at in preferences.
Comment 7 Simon Kaegi CLA 2006-01-09 17:32:12 EST
Created attachment 32712 [details]
Improvement to orginal org.eclipse.equinox.preferences patch

I made a few modifications to attachment 32555 [details]
- made an ordering change in org.eclipse.equinox.registry.Activator startRegistry() to register the provider before the service. This prevents the problem where a listening Customizer uses the RegistryFactory to work with Extensions
- synchronized the serviceAdded and serviceRemoved in the prefs Activator
- set the services to null in unregisterServices() in the prefs Activator

With these changes I've done testing and no longer get the NPE problem.

Note: I used the new multi-project patch format here. If that's a problem let me know and I can re-submit.
Comment 8 Roy Paterson CLA 2006-01-10 12:37:20 EST
Simon's patch looks good to me.
Comment 9 Thomas Watson CLA 2006-01-11 14:24:35 EST
DJ, can you review the preferences patch?
Pascal, can you review the registry patch?

thanks.
Comment 10 Thomas Watson CLA 2006-01-11 14:26:13 EST
Comment on attachment 32555 [details]
Patch to org.eclipse.equinox.preferences

review the new patch from Simon.
Comment 11 DJ Houghton CLA 2006-01-12 12:26:11 EST
Should there be a null check in #unregisterServices in case the osgiPreferencesService field has already been cleared? Is this case possible?
Comment 12 Roy Paterson CLA 2006-01-12 14:47:14 EST
A null there seems pretty unlikely - it could only happen if the service was being registered after the bundlecontext was no longer valid.  Still I think it might be possible.  We might as well put a null check in just in case.
Comment 13 DJ Houghton CLA 2006-01-12 16:00:44 EST
I have reviewed and released the code in the equinox.prefs project. I will leave the registry changes for Pascal.
Comment 14 Pascal Rapicault CLA 2006-01-13 09:59:59 EST
The patch looks good, CC'ed Oleg for further review on potential timing issues.
Comment 15 Oleg Besedin CLA 2006-01-16 10:54:38 EST
The startup order for the "initial" plugins is rather rigid and is hard to modify. The registry plugin is expected to start before the preferences plugin; I don't think anybody considered what happens if that is not the case.

<b>Jeff / Roy / Simon: can you describe a use case that requires activation order to be switched?</b>

If there is a need to do it, we probably can investigate what happens, but to me from the short descriptions in this bug it looks like the proper solution would be to follow the expected activation order.

(By the way, the change in the registry code is a bit of a kludge as RegistryProviderOSGI uses OSGI service to get the registry. Getting the registry is done lazily -  so the change itself is unlikely to present a problem, but logically it is backward.)
Comment 16 Simon Kaegi CLA 2006-01-16 13:54:17 EST
re:kludge
I don't really like the default provider interaction either. It woulde better if the setting of the default provider and registering of this service happened in one atomic step. 

For now, I'd still suggest that swapping the order is a reasonable defensive precaution especially since equinox.prefs tries to use the RegistryFactory in it's Customizer. 

Another perhaps better approach might be for equinox.prefs to avoid using the RegistryFactory and just use the registry service directly (?) I don't know if this could work but I think it would be an improvement.

--
re: use-cases
I can simulate situations with the console but these are contrived. (e.g. stop and restart the registry) Perhaps an update would trigger problems?

One thing to note (and I might have this wrong) is that the activation orders were changed partly in reaction to this problem. Would it be worthwhile to make the necessary changes to avoid having an activation order dependency here? With the patch I'm running these two bundles at the same start-level.



Comment 17 Jeff McAffer CLA 2006-01-16 22:57:16 EST
Avoiding start orderings is DEFINITELY a good thing (tm).  For example, having the prefs come up and wait/listen for a registry service might be just the ticket here.

We need to look carefully at any place where ordering is important.
Comment 18 Roy Paterson CLA 2006-01-17 10:25:10 EST
re: kludge

My understanding was that the prefs code still needed to work in a non-OSGi environment (jars on the classpath).  I think I heard this from Tom Watson - Tom?

re: use-cases

I noticed this bug when launching using the "Equinox OSGi Framework" launcher in the PDE.  By default this launcher makes all bundles the same startlevel.
Comment 19 Thomas Watson CLA 2006-01-17 10:43:09 EST
Yes, the runtime split bundles need to support running in a stand-alone java application (non-OSGi).
Comment 20 Oleg Besedin CLA 2006-01-17 10:48:05 EST
Regarding comment #18 and comment#19:

No, this is not the case. Registry and common are the only runtime plugins that are supposed to run without OSGi. Preferences definetely expects OSGi to be available.

Comment 21 Thomas Watson CLA 2006-01-17 11:09:25 EST
hmmm... ok, thanks for the clarification.  For some reason I thought the stuff split out of runtime was supposed to support stand-alone.  This should make it easier to remove the start order dependency between registry and prefs.  Registry should be the only place where we need to be able to handle when OSGi is not present.  All other cases in runtime should be able to handle different start orders by listening for the services they depend on to be registered.  I think this is what Jeff was suggesting in comment 17
Comment 22 Simon Kaegi CLA 2006-01-17 22:01:38 EST
Created attachment 33186 [details]
less kludge for the registry portion

Here's a less kludgy patch for the registry portion.

I think the IRegistryProvider used in the Activator can be simplified to the point where it just holds and returns the defaultRegistry.

I noticed another problem that this patch corrects. When the bundle was restarted a Core Exception was thrown when RegistryUtils.setRegistryProvider was called from startRegistry() because the old provider was still there. I've added RegistryUtils.resetRegistryProvider for stopRegistry() to call so that we can correctly handle bundle lifecycle.

What do you think?
Comment 23 Oleg Besedin CLA 2006-01-19 16:01:44 EST
From a quick glance, the latest patch removes some desired functionality:
1) registry provider accesses registry via OSGi to create a degree of flexibility
2) "pushing out" default registry is explicitly prohibited - that's why there is an exception if you try to set a registry provider when another registry provider has been set

The fact is that, at present, the order of activation of default plugins is rather rigid. Removing this restriction is non-tivial at best.

Unless there is a valid use case, I recommend closing this bug as "invalid".
Comment 24 Simon Kaegi CLA 2006-01-19 23:43:19 EST
Created attachment 33328 [details]
less kludge for the registry portion 2

I'd like to try one last time and see if we can't mark this FIXED instead.

The simplest use case I can think of is the same as Roy's; running an "Equinox" application in the IDE which when run results in a NPE in PreferenceServices.getPrefExtensions(). This behaviour occurs because by default the uncustomized launcher has all bundles with the same startlevel.

By ensuring that the OSGi provider is available prior to registering the service we no longer get the NPE. (...FIXED...)

The provider as you say is set once (and only once) for the life of the runtime. This means that the same provider is used despite serveral starts and stops of the bundle. It might look backwards, but I think it's reasonable to register the provider early given the provider lives outside the scope of the bundlecontext.

In addition calling RegistryUtils.setRegistryProvider(defaultProvider) each time in startRegistry throws an uncaught CoreException for every subsequent start of the bundle. Ensuring that the provider registers just once in the Activator prevents this problem.

I recognize there are other problems that make startup rigid and I'm not suggesting we remove the current startorder just yet however it's something to shoot for.
Comment 25 Thomas Watson CLA 2006-01-20 08:45:56 EST
I have not looked at the patch, but I aggree with Simon's comment 24.  If possible I would really like to see this fixed.
Comment 26 Oleg Besedin CLA 2006-01-20 10:08:37 EST
Hi Simon,
First, sorry, I was in a hurry and forgot to say that it is good that you attracted our attention to this problem. I *personally* think that the default registry provider code is over-engineered and I would like to see it simplified - just as you did in the "less kludge for the registry portion" patch. That said, either allowing default provider to be pushed out or removing OSGi bridge from the default registry provider should be a conscious explicit decision and that didn't seem to be the case.

Now to the big picture. I am not feeling enthusiastic about this bug because it tries to deal with a small consequence of a much bigger issue. Let's say, this NPE is fixed. What would happen if preferences plugin is asked to provide a preference while registry is not available? I don't know. Even if doesn't throw an exception, would it return a proper result? 

And it is not about the preferences plugin only. There are six or seven plugins specified on the initial bundle list. The same thing will apply to all of them. 

The actual situation here is that:
- We have initial bundles that have to be started in a certain order. 
- The only startup sequence tested and expected to work is the one specified in the osgi.bundles from config.ini
- Most of the core plugins can't be re-installed without Eclipse restart. (Update is the area that I know less about, but, at the very least, it was never tested. From the Eclipse viewpoint, everything that causes UI workbench reinstall should cause a restart anyway - and that includes all of the split runtime plugins.)

Could this picture be improved? Yes. However, let me emphasize, that fixing the NPE alone won't make it happen. It is the other way around - you see the NPE because you are trying to do something not expected from this plugin. In a sense, you are teaching a frog to fly :-).

So, what could we do?

1. Equinox launcher - making it work 
If the Equinox launcher plans to include initial bundles into default configuration, it has to specify proper start order for those bundles, just like PDE does. The "better" way to do this would be to use osgi.bundles line from config.ini and avoid hardcoding. Also, initial plugins might be put into a special section of the list of bundles included in the configuration.

2. Create a bug/enhancement request to remove start order requirement from initial plugins.

There are a number of reasons why osgi.bundles line is what it is today. (In case you were wondering, it didn't arrive to its present way arbitrarily, but as a result of rather long and painful process :-).) Mostly it is based on the order in which functionality is required on startup and can be released on shutdown as well as the requirement to be able to install bundles (org.eclipse.update.configurator).

Even if we can remove startup order restriction (and I would say there is a 50% chance that it won't be possible), if anybody decided to use the osgi.bundles, the Equinox launcher would be broken again.

To summarize, I think that there should be: A) a bug/enhancement request opened saying "Default Equinox launch configuration doesn't take into account osgi.bundles"; b) a bug/enhancement request opened saying "Relax startup order 
requirements and allow reinstall of the split runtime bundles".
Comment 27 Thomas Watson CLA 2006-01-20 10:22:27 EST
I opened bug 124652 to see about enhancing PDE equinox launcher.
Comment 28 Simon Kaegi CLA 2006-01-20 14:15:16 EST
Hi Oleg,
I understand what you're saying and am prepared to drop it.
There's too much going on here to resolve all at once.

When I get the chance I'll log a series of bugs/requests to look at fixing the various lifecycle problems in the split runtime (e.g restarting, installing, uninstalling etc.) Eventually once those issues are resolved we can begin to contemplate relaxing start order.
Comment 29 Thomas Watson CLA 2006-02-03 08:30:14 EST
*** Bug 126297 has been marked as a duplicate of this bug. ***
Comment 30 Thomas Watson CLA 2006-02-21 16:09:19 EST
We missed the M5
Comment 31 Thomas Watson CLA 2006-03-27 11:51:50 EST
Oleg, can you take another look at this defect again?  I think we have lost some context here.  
- Does this NPE occur anymore?
- We have moved to make all the core.runtime split bundles lazy-started (this includes equinox.registry and equinox.jobs).  I think this should ensure all bundles are activated in the correct order as they are needed.

All core.runtime split bundles are now lazy-started and are set to the default start-level "4".  They should all be started successfully as they are needed.  There should not be any more worries about start order.  The only exception is equinox.common which is included on the osgi.bundles list and ensured to start first at start-level "2".

I would hope we can mark this as FIXED for M6 (with no futher code changes).  If we cannot then we must evaluate for RC1.
Comment 32 Simon Kaegi CLA 2006-03-27 12:27:55 EST
Sorry meant to add a note.
I think this is FIXED with no further changes needed in the current code base.

In addition to all the changes you mentioned Preferences no longer mixes using the RegistryFactory API; it just uses the registry service directly.
Comment 33 Oleg Besedin CLA 2006-03-27 14:44:33 EST
Yes, this seems to be have been fixed by the changes to the startup order.
 
Comment 34 Thomas Watson CLA 2006-03-27 15:22:46 EST
Fixed for M6 per last two comments.  Thanks.
Comment 35 DJ Houghton CLA 2006-04-18 14:36:06 EDT
[contributed patch applied]
Comment 36 Thomas Watson CLA 2006-08-23 15:20:25 EDT
adding "contributed" keyword to patches contributed by the community.