eclipse / ecf

ECF project repository
7 stars 14 forks source link

The BND Tools PreferencesComponent has NPE when started early #83

Closed merks closed 10 months ago

merks commented 10 months ago

When installing all SimRel components in a single IDE, i.e., via this product in the installer:

image

this exception is in the log upon startup:

eclipse.buildId=4.30.0.I20231115-1800
java.version=21.0.1
java.vendor=Eclipse Adoptium
BootLoader constants: OS=win32, ARCH=x86_64, WS=win32, NL=en_US
Command-line arguments:  -os win32 -ws win32 -arch x86_64

org.eclipse.ecf.remoteservices.tooling.bndtools
Error
Fri Nov 24 08:43:39 CET 2023
bundle org.eclipse.ecf.remoteservices.tooling.bndtools:1.0.0.v20230728-2232 (587)[org.eclipse.ecf.remoteservices.tooling.bndtools.PreferencesComponent(56)] : The activate method has thrown an exception

java.lang.IllegalStateException: The instance data location has not been specified yet.
    at org.eclipse.core.internal.runtime.DataArea.assertLocationInitialized(DataArea.java:61)
    at org.eclipse.core.internal.runtime.DataArea.getStateLocation(DataArea.java:146)
    at org.eclipse.core.internal.preferences.InstancePreferences.getBaseLocation(InstancePreferences.java:45)
    at org.eclipse.core.internal.preferences.InstancePreferences.initializeChildren(InstancePreferences.java:120)
    at org.eclipse.core.internal.preferences.InstancePreferences.<init>(InstancePreferences.java:60)
    at org.eclipse.core.internal.preferences.InstancePreferences.internalCreate(InstancePreferences.java:131)
    at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:351)
    at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:339)
    at org.eclipse.core.internal.preferences.PreferencesService.createNode(PreferencesService.java:406)
    at org.eclipse.core.internal.preferences.RootPreferences.getChild(RootPreferences.java:61)
    at org.eclipse.core.internal.preferences.RootPreferences.getNode(RootPreferences.java:94)
    at org.eclipse.core.internal.preferences.RootPreferences.node(RootPreferences.java:83)
    at org.eclipse.core.internal.preferences.AbstractScope.getNode(AbstractScope.java:40)
    at org.eclipse.core.runtime.preferences.InstanceScope.getNode(InstanceScope.java:80)
    at org.eclipse.ui.preferences.ScopedPreferenceStore.getStorePreferences(ScopedPreferenceStore.java:213)
    at org.eclipse.ui.preferences.ScopedPreferenceStore.<init>(ScopedPreferenceStore.java:127)
    at org.eclipse.ecf.remoteservices.tooling.bndtools.PreferencesComponent.activate(PreferencesComponent.java:167)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.apache.felix.scr.impl.inject.methods.BaseMethod.invokeMethod(BaseMethod.java:245)
    at org.apache.felix.scr.impl.inject.methods.BaseMethod.access$500(BaseMethod.java:41)
    at org.apache.felix.scr.impl.inject.methods.BaseMethod$Resolved.invoke(BaseMethod.java:687)
    at org.apache.felix.scr.impl.inject.methods.BaseMethod.invoke(BaseMethod.java:531)
    at org.apache.felix.scr.impl.inject.methods.ActivateMethod.invoke(ActivateMethod.java:317)
    at org.apache.felix.scr.impl.inject.methods.ActivateMethod.invoke(ActivateMethod.java:307)
    at org.apache.felix.scr.impl.manager.SingleComponentManager.createImplementationObject(SingleComponentManager.java:354)
    at org.apache.felix.scr.impl.manager.SingleComponentManager.createComponent(SingleComponentManager.java:115)
    at org.apache.felix.scr.impl.manager.SingleComponentManager.getService(SingleComponentManager.java:1002)
    at org.apache.felix.scr.impl.manager.SingleComponentManager.getServiceInternal(SingleComponentManager.java:975)
    at org.apache.felix.scr.impl.manager.AbstractComponentManager.activateInternal(AbstractComponentManager.java:785)
    at org.apache.felix.scr.impl.manager.AbstractComponentManager.enableInternal(AbstractComponentManager.java:674)
    at org.apache.felix.scr.impl.manager.AbstractComponentManager.enable(AbstractComponentManager.java:437)
    at org.apache.felix.scr.impl.manager.ConfigurableComponentHolder.enableComponents(ConfigurableComponentHolder.java:671)
    at org.apache.felix.scr.impl.BundleComponentActivator.initialEnable(BundleComponentActivator.java:310)
    at org.apache.felix.scr.impl.Activator.loadComponents(Activator.java:593)
    at org.apache.felix.scr.impl.Activator.access$200(Activator.java:74)
    at org.apache.felix.scr.impl.Activator$ScrExtension.start(Activator.java:460)
    at org.apache.felix.scr.impl.AbstractExtender.createExtension(AbstractExtender.java:196)
    at org.apache.felix.scr.impl.AbstractExtender.modifiedBundle(AbstractExtender.java:169)
    at org.apache.felix.scr.impl.AbstractExtender.addingBundle(AbstractExtender.java:139)
    at org.apache.felix.scr.impl.AbstractExtender.addingBundle(AbstractExtender.java:49)
    at org.osgi.util.tracker.BundleTracker$Tracked.customizerAdding(BundleTracker.java:477)
    at org.osgi.util.tracker.BundleTracker$Tracked.customizerAdding(BundleTracker.java:1)
    at org.osgi.util.tracker.AbstractTracked.trackAdding(AbstractTracked.java:258)
    at org.osgi.util.tracker.AbstractTracked.track(AbstractTracked.java:231)
    at org.osgi.util.tracker.BundleTracker$Tracked.bundleChanged(BundleTracker.java:452)
    at org.eclipse.osgi.internal.framework.BundleContextImpl.dispatchEvent(BundleContextImpl.java:949)
    at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:234)
    at org.eclipse.osgi.framework.eventmgr.ListenerQueue.dispatchEventSynchronous(ListenerQueue.java:151)
    at org.eclipse.osgi.internal.framework.EquinoxEventPublisher.publishBundleEventPrivileged(EquinoxEventPublisher.java:229)
    at org.eclipse.osgi.internal.framework.EquinoxEventPublisher.publishBundleEvent(EquinoxEventPublisher.java:138)
    at org.eclipse.osgi.internal.framework.EquinoxEventPublisher.publishBundleEvent(EquinoxEventPublisher.java:130)
    at org.eclipse.osgi.internal.framework.EquinoxContainerAdaptor.publishModuleEvent(EquinoxContainerAdaptor.java:217)
    at org.eclipse.osgi.container.Module.publishEvent(Module.java:499)
    at org.eclipse.osgi.container.Module.start(Module.java:486)
    at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel$2.run(ModuleContainer.java:1852)
    at org.eclipse.osgi.internal.framework.EquinoxContainerAdaptor$1$1.execute(EquinoxContainerAdaptor.java:136)
    at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.incStartLevel(ModuleContainer.java:1845)
    at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.incStartLevel(ModuleContainer.java:1786)
    at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.doContainerStartLevel(ModuleContainer.java:1750)
    at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.dispatchEvent(ModuleContainer.java:1672)
    at org.eclipse.osgi.container.ModuleContainer$ContainerStartLevel.dispatchEvent(ModuleContainer.java:1)
    at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:234)
    at org.eclipse.osgi.framework.eventmgr.EventManager$EventThread.run(EventManager.java:345)
scottslewis commented 10 months ago

Hi @merks. ECF now has a bndtools enhancement (remote services support based on bndtools 7).

This is the relevant part of the stack trace

java.lang.IllegalStateException: The instance data location has not been specified yet. at org.eclipse.core.internal.runtime.DataArea.assertLocationInitialized(DataArea.java:61) at org.eclipse.core.internal.runtime.DataArea.getStateLocation(DataArea.java:146) at org.eclipse.core.internal.preferences.InstancePreferences.getBaseLocation(InstancePreferences.java:45) at org.eclipse.core.internal.preferences.InstancePreferences.initializeChildren(InstancePreferences.java:120) at org.eclipse.core.internal.preferences.InstancePreferences.(InstancePreferences.java:60) at org.eclipse.core.internal.preferences.InstancePreferences.internalCreate(InstancePreferences.java:131) at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:351) at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:339) at org.eclipse.core.internal.preferences.PreferencesService.createNode(PreferencesService.java:406) at org.eclipse.core.internal.preferences.RootPreferences.getChild(RootPreferences.java:61) at org.eclipse.core.internal.preferences.RootPreferences.getNode(RootPreferences.java:94) at org.eclipse.core.internal.preferences.RootPreferences.node(RootPreferences.java:83) at org.eclipse.core.internal.preferences.AbstractScope.getNode(AbstractScope.java:40) at org.eclipse.core.runtime.preferences.InstanceScope.getNode(InstanceScope.java:80) at org.eclipse.ui.preferences.ScopedPreferenceStore.getStorePreferences(ScopedPreferenceStore.java:213) at org.eclipse.ui.preferences.ScopedPreferenceStore.(ScopedPreferenceStore.java:127) at org.eclipse.ecf.remoteservices.tooling.bndtools.PreferencesComponent.activate(PreferencesComponent.java:167)

And the ecf code in PreferencesComponent.java:167 is:

``ScopedPreferenceStore store = new ScopedPreferenceStore(InstanceScope.INSTANCE, "org.bndtools.templating.gitrepo");

This is trying to create a preferences store and as you can see from the stack trace it throws exception in the ScopedPreferenceStore constructor.

My question for fixing this is: What should the ECF code wait for (service registration, something else?) before trying to create a ScopedPreferenceStore? I already have a reference to the first IPreferenceService, but perhaps that's still too early. I just don't know what to wait for to be assured that ScopedPreferenceStore can be created. If you know or can point me in the right direction I would appreciate it.

laeubi commented 10 months ago

@scottslewis just a few remarks:

  1. creating ScopedPreferenceStore in activation of component seems quite odd, do you really need at there or can you delay that?
  2. As you are passing InstanceScope.INSTANCE anyways, if you just want to access the preference cant you use org.eclipse.core.runtime.preferences.IScopeContext.getNode( "org.bndtools.templating.gitrepo") instead, why using an UI component (== ScopedPreferenceStore) here?
  3. If for whatever reason you really really require that in an activator, you should add @Reference IWorkspace yagni; to your component to make it not start earlier than the workspace.
scottslewis commented 10 months ago

@scottslewis just a few remarks:

1. creating `ScopedPreferenceStore` in activation of component seems quite odd, do you really need at there or can you delay that?

I can delay it, but the question I pose above is: Delay it until when? It seems to me that the preferences startup/config should 'just happen'...e.g. before ScopedPreferenceStore init can be called...rather than consumers of the preference store having to wait for some specific event, but given it's not like this currently, I assumed in the existing code that IPreferenceService would not be registered until until preference store is ready, but that's apparently not true.

2. As you are passing `InstanceScope.INSTANCE` anyways, if you just want to access the preference cant you use `org.eclipse.core.runtime.preferences.IScopeContext.getNode( "org.bndtools.templating.gitrepo") ` instead, why using an UI component (== ScopedPreferenceStore) here?

I have my doubts about whether this would work, and so don't think it's worth trying just to find after deployment that it depends upon timing/ds threads or something.

3. If for whatever reason you really really require that in an activator, you should add `@Reference IWorkspace yagni;` to your component to make it not start earlier than the workspace.

I'm already doing this with IPreferenceService...which I thought would delay the component activation until the ScopedPreferenceStore was actually ready. I don't think it works because there are multiple IPreferenceServices registered.

So you believe IWorkspace is the service to wait for? Do have any definitive word about when during startup (e.g. service registered) that bundles can use the preference store?

I can use IWorkspace but to avoid unnecessary static dependencies I would prefer to have it be some service that is directly related to preference store initialization if possible (that's why I tried IPreferenceService first).

laeubi commented 10 months ago

I can delay it, but the question I pose above is: Delay it until when?

When you first need it... IPreferenceStore is a JFace concept and ScopedPreferenceStore is even org.eclipse.ui stuff so these are supposed to be used in an UI context (e.g. open a dialog or preference page) and that is the point where you should delay them.

It seems to me that the preferences startup/config should 'just happen'...e.g. before ScopedPreferenceStore init can be called...rather than consumers of the preference store having to wait for some specific event, but given it's not like this currently

This is not possible as you are passing in a SCOPE different scopes have different backends, in this case you are using Instance (aka Workspace) scope before the workspace is there (e.g. currently Workspace chooser dialog is showing to the user), also how should the config prevent the user from calling an activator?

I assumed in the existing code that IPreferenceService would not be registered until until preference store is ready

IPreferencesService is a completely different thing, to improve things for SCOPES I have created:

I have my doubts about whether this would work, and so don't think it's worth trying just to find after deployment that it depends upon timing/ds threads or something.

You can try it out in your IDE by choose in the run config to not use a default location for the workspace like this: grafik

but as said INSTANCE Scope is only there after the workspace was created so you need to wait for that....

So you believe IWorkspace is the service to wait for?

If you want to use INSTANCE scope yes ... for Bundlescope one can use for example like this: https://github.com/eclipse-platform/eclipse.platform/blob/d645ff3bb4e308b520cffe9bfb1e533e62ec22c6/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/CheckMissingNaturesListener.java#L46-L47

Do have any definitive word about when during startup (e.g. service registered) that bundles can use the preference store?

As said it depends and IPreferenceStore is an UI thing so most of the time one do not expect this to be used before the UI is actually shown to the user.

I can use IWorkspace but to avoid unnecessary static dependencies I would prefer to have it be some service that is directly related to preference store initialization if possible (that's why I tried IPreferenceService first).

IWorkspace already is a service ... in cases where one don't need the service at all but still wants to wait for workspace I now created:

to make it more declarative than implicit.

Beside that: I'm not the best person when it comes to documentation, so if you feel you would have wanted some of this information here found anywhere at the existing eclipse documentation don't hesitate to open a PR and add the missing pieces, this is quite "new" (about half and a year now) even maybe for other platform committers and still evolving but already has made the eclipse startup much more safe for those not using ResourcePlugin#getWorkspace) and for the resource plugin in general.

scottslewis commented 10 months ago

I can delay it, but the question I pose above is: Delay it until when?

When you first need it... IPreferenceStore is a JFace concept and ScopedPreferenceStore is even org.eclipse.ui stuff so these are supposed to be used in an UI context (e.g. open a dialog or preference page) and that is the point where you should delay them.

Some feedback for you: I know that it needs to wait until I first need it. The point of this component is to setup the defaults for the bndtools preferences, and I want it started as soon as the preferences store can handle it. The question is: Since the preference store isn't actually ready when IPreferenceService (not IPreferenceStore) is ready...at least the first one, then when can I safely access the preferences store as early as possible without having the ScopedPreferenceStore constructor throw an exception.

> **Beside that:** I'm not the best person when it comes to documentation, so if you feel you would have wanted some of this information here found anywhere at the existing eclipse documentation don't hesitate to open a PR and add the missing pieces, this is quite "new" (about half and a year now) even maybe for other platform committers and still evolving but already has made the eclipse startup much more safe for those **not** using `ResourcePlugin#getWorkspace`) and for the resource plugin in general. @laeubi This is pretty much the only useful information you provided in this post, and this isn't very useful at all. I've been looking through Eclipse documentation for > 20 years now, and I know it's lacking and that I can contributed code and or doc fixest if I want...and I've done more than my fair share of just that. Also, I know that the preferences stuff has changed multiple times during that period (I was an equinox committer for some time years ago), and that's probably why an article that I read about preferences 10 years ago probably isn't relevant today. That's why I was asking for the information in the post above (i.e. up to date information on preferences and Eclipse startup). Also, IMHO it would be a better startup design if the preferences initialization for Eclipse was done so that clients didn't have to guess about things like when it's safe to load an api class and call it's constructor like ScopedPreferenceService but as I've said in other contexts, as a project lead I've not got the time or inclination to contribute to the IDE improvement any further.
laeubi commented 10 months ago

The point of this component is to setup the defaults for the bndtools preferences

If you want to initialize defaults then you need to use a PreferenceInitializer, this is one of the first results if you search for "eclipse preference initialize" and haven't changed for the last 20 years.

Also note that the documentation says "Clients should only set default preference values for their own bundle." that is because otherwise you will get different results and as preferences can change anytime a bundle using these should setup appropriate listeners to act on changes so it does not really matter when they are set in the startup procedure. That is because Preferences are an UI thing and a user usually don't want to restart Eclipse for their changes to be applied.

that's probably why an article that I read about preferences 10 years ago probably isn't relevant today

I also highly doubt that any 10 year old article is recommending using a service component for that purpose, but if you like here is a more comprehensive explanation about how preferences defaults works :

https://gnu-mcu-eclipse.github.io/developer/eclipse/runtime-preferences/

Also, IMHO it would be a better startup design if the preferences initialization for Eclipse was done so that clients didn't have to guess about things like when it's safe to load an api class and call it's constructor like ScopedPreferenceService

You are mixing different concepts and design here, that is nothing "Eclipse" can prevent as all these classes are public API .... that don't mean it will work to use them in any combination bypassing the provided mechanisms to use them.

scottslewis commented 10 months ago

The point of this component is to setup the defaults for the bndtools preferences

If you want to initialize defaults then you need to use a PreferenceInitializer, this is one of the first results if you search for "eclipse preference initialize" and haven't changed for the last 20 years.

Thanks.

Also note that the documentation says "Clients should only set default preference values for their own bundle." that is because otherwise you will get different results and as preferences can change anytime a bundle using these should setup appropriate listeners to act on changes so it does not really matter when they are set in the startup procedure. That is because Preferences are an UI thing and a user usually don't want to restart Eclipse for their changes to be applied.

I appreciate the lecture, but I know what I'm doing.

> > > Also, IMHO it would be a better startup design if the preferences initialization for Eclipse was done so that clients didn't have to guess about things like when it's safe to load an api class and call it's constructor like ScopedPreferenceService > > You are mixing different concepts and design here, that is nothing "Eclipse" can prevent as all these classes are public API .... that don't mean it will work to use them in any combination bypassing the provided mechanisms to use them. Ugh. So pedantic. It's tiresome.