Bug 564876 - Provide a listener whiteboard adapter for IResourceChangeListener
Summary: Provide a listener whiteboard adapter for IResourceChangeListener
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Resources (show other bugs)
Version: 4.17   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.17 M3   Edit
Assignee: Alex Blewitt CLA
QA Contact:
URL:
Whiteboard:
Keywords: Documentation, noteworthy
Depends on:
Blocks:
 
Reported: 2020-07-02 12:50 EDT by Alex Blewitt CLA
Modified: 2021-03-31 15:37 EDT (History)
7 users (show)

See Also:


Attachments
Example project of an immediately started, enable on startup bundle (3.64 KB, application/zip)
2020-07-02 18:16 EDT, Alex Blewitt CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Blewitt CLA 2020-07-02 12:50:13 EDT
I’ve proposed a change at https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/165750 to provide a way of registering IResourceChangeListener instances via an OSGi service rather than an explicit call to IWorkspace.

There’s lots of calls in projects that look something like:

ResourcesPlugin.getWorkspace().addResourceChangeListener(resourceChangeListener);

This has an ordering dependency that the workspace needs to be running before this registration occurs. Of course, if the workspace doesn’t exist at this point we aren’t going to be receiving any events but we’d like to be able to start them in either order.

If we use the OSGi whiteboard pattern, we can turn it on its head and do:

context.registerService(resourceChangeListener, IResourceChangeListener.class);

(We do something similar in many places for DebugOptions.)

Now we have decoupled the start order dependency, and with the change above we’ll now pick up the binding when the resources plugin is available, and will automatically deregister when either service goes away.

We can also use this to register the listeners via DS:

<?xml version="1.0" encoding="UTF-8"?>
<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.4.0" immediate="true" name="ExampleResourceListener">
   <implementation class="org.example.ExampleResourceListener"/>
   <service>
      <provide interface="org.eclipse.core.resources.IResourceChangeListener"/>
   </service>
   <!-- 1 == IResourceChangeEvent.POST_CHANGE -->
   <property name="event.mask" type="Integer" value="1"/>
</scr:component>
The additional properties on the service will allow a subset of the event types to be passed in (in this case, 1 == POST_BUILD). There is a minor disadvantage in that this is an integer rather than a compile-time constant reference, though if the registration in code is used you can have a Dictionary including IResourceChangeEvent.POST_BUILD as the key in the dictionary.

This approach of having a whiteboard pattern (either DS or ServiceTracker) for listeners could be replayed on other listener types as well; but from what I can tell, the IResourceChangeListener is the most popular in hooking together the world.

Arguably it might be more ‘OSGi’ to attempt migrating the IResourceChangeEvent to an OSGi EventAdmin topic, but that would require more invasive dependency and code changes than exists at the moment. Having everything in DS means that we can start the components lazily and avoid the use of Activators, which will certainly help with the start-up times.
Comment 1 Christoph Laeubrich CLA 2020-07-02 14:02:03 EDT
Just a note on startup times:
If you really care about that, care must be taken on the consumer side to not eagerly fetch the service and thus trigger the creation of the service anyways.
Beside this, parsing the xml and handling for DS *might* be slower than register a class directly.

> immediate="true" 

would be a real anti-pattern here if it comes to startup times as the service is created and registered regardless if someone has any use for it or not.
Comment 2 Alex Blewitt CLA 2020-07-02 14:10:26 EDT
(In reply to Christoph Laeubrich from comment #1)
> Just a note on startup times:
> If you really care about that, care must be taken on the consumer side to
> not eagerly fetch the service and thus trigger the creation of the service
> anyways.

This is generally a problem for ServiceTracker, which auto-instantiates as many DS components as it can find :)

> Beside this, parsing the xml and handling for DS *might* be slower than
> register a class directly.

Computationally this approach certainly adds CPU overhead. However, start-up is a sequence of activators depending on other bundles' activators, and bundle activation is massively single threaded. (Even if you turn on multiple threads, as soon as you attempt to load a class from a bundle that hasn't been started, you're blocked until that class comes back.)

The other aspect is that the work done in the bundle isn't all CPU bound; rather, the activator loads some classes, which loads further classes, which loads further classes ... in fact, the majority of the work is IO bound rather than CPU bound. By extracting the single-threaded startup in the activators to independent components, we (a) don't block others trying to load a class from this one and (b) we can parallelise the waiting for classes to be loaded and defined. If we get to a point where we have no activators but everything is components in the future, start-up will be significantly faster.

> > immediate="true" 
> 
> would be a real anti-pattern here if it comes to startup times as the
> service is created and registered regardless if someone has any use for it
> or not.

However in this case, the client components are implementing a callback listener, rather than using that to provide a service for others to use. The bundle's activator is in the majority of cases creating the listener and then calling workspace.addlistener anyway, so when the bundle starts we want this component to be created.
Comment 3 Christoph Laeubrich CLA 2020-07-02 14:20:15 EDT
(In reply to Alex Blewitt from comment #2)
> This is generally a problem for ServiceTracker, which auto-instantiates as
> many DS components as it can find :)

Servicetrackers, as well as DS components can still lazy-fetch the service by just holding the service reference.


> If we get to a point where we have no
> activators but everything is components in the future, start-up will be
> significantly faster.

Not if the consumer/producer side is implemented in a naive fashion as outlined in your example as activating the bundle will in fact trigger the same code to be loaded as if an activator exits.

> However in this case, the client components are implementing a callback
> listener, rather than using that to provide a service for others to use.

That doesn't make any difference and in fact the consumer in the whiteboardpatter is "using" the service

> The bundle's activator is in the majority of cases creating the listener and
> then calling workspace.addlistener anyway, so when the bundle starts we want
> this component to be created.

I don't agree here, in fact when no resource change event ever occurs, the component does not need to be ever created. Using immediate="true" is the same as having register the service registered in the activator if one assumes that DS will be started at some point of the activation sequence.

That's one of the advantages of DS that it only creates services as demanded (if properly used), and do not create them in advance like an activator does, its the same idea behind the plugin.xml (your idea could also be implemented as part of an extension-point).
Comment 4 Andrey Loskutov CLA 2020-07-02 15:29:00 EDT
As Tom pointed on mailing list, eager Resources bundle init will simply not work before workspace selection dialog is shown. We had fun to fix few bugs related to that problem in the past.
Comment 5 Alex Blewitt CLA 2020-07-02 16:32:48 EDT
(In reply to Christoph Laeubrich from comment #3)
> (In reply to Alex Blewitt from comment #2)
> > This is generally a problem for ServiceTracker, which auto-instantiates as
> > many DS components as it can find :)
> 
> Servicetrackers, as well as DS components can still lazy-fetch the service
> by just holding the service reference.

I started writing a reply to this, but it grew too big, so I published it as a blog post instead:

https://alblue.bandlem.com/2020/07/why-servicetracker-is-bad.html

The corresponding repository with code is here:

https://github.com/alblue/why-servicetracker-is-bad

TL;DR using ServiceTracker.open() causes a lazily instantiated DS component to be created, even if no-one uses the result of the ServicETracker.

> > If we get to a point where we have no
> > activators but everything is components in the future, start-up will be
> > significantly faster.
> 
> Not if the consumer/producer side is implemented in a naive fashion as
> outlined in your example as activating the bundle will in fact trigger the
> same code to be loaded as if an activator exits.

But importantly, not in the same thread, and not whilst holding the bundle-wide classloader lock.

> > The bundle's activator is in the majority of cases creating the listener and
> > then calling workspace.addlistener anyway, so when the bundle starts we want
> > this component to be created.
> 
> I don't agree here, in fact when no resource change event ever occurs, the
> component does not need to be ever created. Using immediate="true" is the
> same as having register the service registered in the activator if one
> assumes that DS will be started at some point of the activation sequence.

No, because it will be in a different thread and one in which is not holding the bundle's classloader lock. As a result, you'll be able to spin up many of them in parallel while other dependent bundles also continue to start, as opposed today when they are blocked because it is all serial.

> That's one of the advantages of DS that it only creates services as demanded
> (if properly used), and do not create them in advance like an activator
> does, its the same idea behind the plugin.xml (your idea could also be
> implemented as part of an extension-point).

To be fair, the plugin.xml also causes eager activation because the loading of the plugin.xml causes the lazy activation of the bundle to trigger as well :) But we are wildly off topic here and none of the above affects the discussion of the point of this bug, which is to provide a means by which IResourceChangeListener instances can be published in the OSGi service registry such that they are automatically associated with the workspace instead of having to call GlobalSingleton.getDefault().getWorkspace().addRegistryChangeListener() which as noted elsewhere has a number of start-up dependency assumptions baked in.
Comment 6 Alex Blewitt CLA 2020-07-02 16:33:38 EDT
(In reply to Andrey Loskutov from comment #4)
> As Tom pointed on mailing list, eager Resources bundle init will simply not
> work before workspace selection dialog is shown. We had fun to fix few bugs
> related to that problem in the past.

Nothing in this bug suggests eager resource activation will occur, nor that it will be shown before the workspace selection dialog is shown.
Comment 7 Andrey Loskutov CLA 2020-07-02 16:39:40 EDT
(In reply to Alex Blewitt from comment #6)
> (In reply to Andrey Loskutov from comment #4)
> > As Tom pointed on mailing list, eager Resources bundle init will simply not
> > work before workspace selection dialog is shown. We had fun to fix few bugs
> > related to that problem in the past.
> 
> Nothing in this bug suggests eager resource activation will occur, nor that
> it will be shown before the workspace selection dialog is shown.

immediate=true argument will *not* load classes from core.resources bundle?

https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/165750/2/bundles/org.eclipse.core.resources/OSGI-INF/ResourceChangeListenerRegistrar.xml
Comment 8 Alex Blewitt CLA 2020-07-02 18:11:57 EDT
(In reply to Andrey Loskutov from comment #7)
> (In reply to Alex Blewitt from comment #6)
> > (In reply to Andrey Loskutov from comment #4)
> > > As Tom pointed on mailing list, eager Resources bundle init will simply not
> > > work before workspace selection dialog is shown. We had fun to fix few bugs
> > > related to that problem in the past.
> > 
> > Nothing in this bug suggests eager resource activation will occur, nor that
> > it will be shown before the workspace selection dialog is shown.
> 
> immediate=true argument will *not* load classes from core.resources bundle?
> 
> https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/165750/2/
> bundles/org.eclipse.core.resources/OSGI-INF/ResourceChangeListenerRegistrar.
> xml

I think it's worth pointing out that this component is inside the org.eclipse.core.resources bundle. Ergo no, it will not load classes from the org.eclipse.core.resources bundle while it is stopped.

When the org.eclipse.core.resources bundle has started (i.e. after the Activator has run), then the component will be instantiated and load classes from the same bundle that it is in. But I don't see how that affects the specific issue?

To be explicitly clear: a DS component with an immediate component does not trigger starting of the bundle. Therefore it cannot load the classes from the bundle, because it's not started.

TL;DR bundles that are stopped will remain stopped until acted on by a third force. That force does not include DS, which does not start components (marked immediate or otherwise) until the bundle is started.
Comment 9 Alex Blewitt CLA 2020-07-02 18:16:25 EDT
Created attachment 283487 [details]
Example project of an immediately started, enable on startup bundle

The example ding.zip contains an OSGi declarative services component which is immediately activated. When run in an OSGi runtime with ds (as in Ding.launch) you will note that the bundle is not automatically started, and although DS is present it will not start up the component. It is not until the Ding bundle is started that the component will be immediately activated.
Comment 10 Christoph Laeubrich CLA 2020-07-03 01:27:22 EDT
(In reply to Alex Blewitt from comment #5)
> I started writing a reply to this, but it grew too big, so I published it as
> a blog post instead:
> 
> https://alblue.bandlem.com/2020/07/why-servicetracker-is-bad.html
> 
> The corresponding repository with code is here:
> 
> https://github.com/alblue/why-servicetracker-is-bad
> 
> TL;DR using ServiceTracker.open() causes a lazily instantiated DS component
> to be created, even if no-one uses the result of the ServicETracker.

That is because you use no Customizer and the default implementation fetches the service immediately. I have created a pullrequest [1] to show how it would work.

The same would happen if you use DS with a @Reference so I think your blog-post is misleading:

- ServiceTracker is not 'bad', even though it can be used wrong depending on the desired behavior
- Using DS does not prevent lazy-activation of DS components unless the component that references the lazy-component itself is not activated
- I never said that servicetracker won't block or won't be able to activate ds-components

So to keep up with your blog-post wording: "Unfortunately" even people that have authored several books about eclipse do not necessarily know how to use servicetrackers and thus I mentioned that such an implementation (if the desired effect is to be archived) "care must be taken on the consumer side ", simply adding white-board pattern to the mix does not solve the issue, nor does using DS (even though I would recommend this whenever possible).

[1] https://github.com/alblue/why-servicetracker-is-bad/pull/1
Comment 11 Christoph Laeubrich CLA 2020-07-03 01:37:24 EDT
(In reply to Alex Blewitt from comment #9)
> it will not start up the component. It is not until
> the Ding bundle is started that the component will be immediately activated.

Sure if your bundle is never started the Activator is never run and your startup won't be affected in any way so no difference to DS here, so what we are talking about?
Comment 12 Alex Blewitt CLA 2020-07-03 04:01:50 EDT
(In reply to Christoph Laeubrich from comment #11)
> (In reply to Alex Blewitt from comment #9)
> > it will not start up the component. It is not until
> > the Ding bundle is started that the component will be immediately activated.
> 
> Sure if your bundle is never started the Activator is never run and your
> startup won't be affected in any way so no difference to DS here, so what we
> are talking about?

I'm not sure; this was in reply to a separate thread from Andrey:

(In reply to Andrey Loskutov from comment #7)
> (In reply to Alex Blewitt from comment #6)
> > (In reply to Andrey Loskutov from comment #4)
> > > As Tom pointed on mailing list, eager Resources bundle init will simply not
> > > work before workspace selection dialog is shown. We had fun to fix few bugs
> > > related to that problem in the past.
> > 
> > Nothing in this bug suggests eager resource activation will occur, nor that
> > it will be shown before the workspace selection dialog is shown.
> 
> immediate=true argument will *not* load classes from core.resources bundle?
> 
> https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/165750/2/
> bundles/org.eclipse.core.resources/OSGI-INF/ResourceChangeListenerRegistrar.
> xml

I may have misunderstood their comment on the concern wrt immediate=true. My point was that the component was in the same bundle and so it isn't possible for this bundle to cause the activation of the resource bundle. Maybe Andrey can help enlighten me as to whether this was the concern or something else?
Comment 13 Christoph Laeubrich CLA 2020-07-03 04:08:53 EDT
(In reply to Alex Blewitt from comment #12)
> I may have misunderstood their comment on the concern wrt immediate=true. My
> point was that the component was in the same bundle and so it isn't possible
> for this bundle to cause the activation of the resource bundle. Maybe Andrey
> can help enlighten me as to whether this was the concern or something else?

if resourcelistener class is inside resource-bundle and resourcebundle has lazy-activation policy and your component has immediate=true and is loaded/activated before then this will trigger the activation/loading of the resource-bundle.
Comment 14 Alex Blewitt CLA 2020-07-03 04:23:04 EDT
(In reply to Christoph Laeubrich from comment #10)
> (In reply to Alex Blewitt from comment #5)
> > I started writing a reply to this, but it grew too big, so I published it as
> > a blog post instead:
> > 
> > https://alblue.bandlem.com/2020/07/why-servicetracker-is-bad.html
> > 
> > The corresponding repository with code is here:
> > 
> > https://github.com/alblue/why-servicetracker-is-bad
> > 
> > TL;DR using ServiceTracker.open() causes a lazily instantiated DS component
> > to be created, even if no-one uses the result of the ServicETracker.
> 
> That is because you use no Customizer and the default implementation fetches
> the service immediately. I have created a pullrequest [1] to show how it
> would work.

Sure, that's my point. Many users of the ServiceTracker won't know that if you're using it 'the easy way' then it'll trigger eager activation.

Thanks for the PR though, it's a good example of how to do it properly. I'll leave it open so that people can see the additional work necessary to be lazy with the service, because I think it's enlightening to see how much additional work it takes to do the work in a lazy way.

> The same would happen if you use DS with a @Reference so I think your
> blog-post is misleading:
> 
> - ServiceTracker is not 'bad', even though it can be used wrong depending on
> the desired behavior
> - Using DS does not prevent lazy-activation of DS components unless the
> component that references the lazy-component itself is not activated
> - I never said that servicetracker won't block or won't be able to activate
> ds-components

Sometimes it's necessary to have poetic license to move the needle :) But yes, I agree you can have DS components that enforce their requirements and you can use a ServiceTrackerCustomiser to do the lazy approach with ServiceTracker.

I'd argue that it's easier to do the right thing with DS and easier to do the wrong thing with ServiceTracker, and as a result, using a ServiceTrackerCustomiser isn't done, certainly not in the wild in the Eclipse codebase.

> So to keep up with your blog-post wording: "Unfortunately" even people that
> have authored several books about eclipse do not necessarily know how to use
> servicetrackers and thus I mentioned that such an implementation (if the
> desired effect is to be archived) "care must be taken on the consumer side
> ", simply adding white-board pattern to the mix does not solve the issue,
> nor does using DS (even though I would recommend this whenever possible).

It's not really that; it's that the use case of ServiceTracker within the Eclipse codebase widely uses the non-reference approach.

I have releng.aggregator checked out and running:

$ git submodule foreach "git grep ServiceTrackerCustomizer[\(\<] || :" | cat

shows only four use cases outside of rt.equinox.framework and rt.equinox.bundles.

Entering 'eclipse.jdt'
Entering 'eclipse.jdt.core'
Entering 'eclipse.jdt.core.binaries'
Entering 'eclipse.jdt.debug'
Entering 'eclipse.jdt.ui'
Entering 'eclipse.pde.build'
Entering 'eclipse.pde.ui'
Entering 'eclipse.platform'
Entering 'eclipse.platform.common'
Entering 'eclipse.platform.debug'
Entering 'eclipse.platform.releng'
Entering 'eclipse.platform.resources'
Entering 'eclipse.platform.runtime'
bundles/org.eclipse.e4.core.services/src/org/eclipse/e4/core/internal/services/ResourceBundleHelper.java: new ServiceTrackerCustomizer<LoggerFactory, Logger>() {
Entering 'eclipse.platform.swt'
Entering 'eclipse.platform.swt.binaries'
Entering 'eclipse.platform.team'
Entering 'eclipse.platform.text'
Entering 'eclipse.platform.ua'
Entering 'eclipse.platform.ui'
Entering 'eclipse.platform.ui.tools'
Entering 'rt.equinox.binaries'
Entering 'rt.equinox.p2'
bundles/org.eclipse.equinox.p2.console/src/org/eclipse/equinox/internal/p2/console/Activator.java:public class Activator implements BundleActivator, ServiceTrackerCustomizer<IProvisioningAgent, IProvisioningAgent> {
bundles/org.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java:public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCustomizer<IAgentServiceFactory, Object> {
bundles/org.eclipse.equinox.p2.testserver/src/org/eclipse/equinox/p2/testserver/Activator.java:public class Activator implements BundleActivator, ServiceTrackerCustomizer<HttpService, HttpService> {

On the other hand, if you look solely at InternalPlatform.java, you'll see that it creates no less than 13 trackers, none of which use the lazy approach outlined above:

https://github.com/eclipse/eclipse.platform.runtime/blob/defae674f9adf7cb1dc91532280e9b1b7990eb1e/bundles/org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime/InternalPlatform.java#L647-L720 

In fact, doing the same search looking for (roughly) all uses of ServiceTracker where the third argument is null:

$ git submodule foreach "git grep -e ServiceTracker[\(\<][^,]*, || :" | wc -l                                                                                                                       9:20:02
     445

The regex is a bit weak; it won't find things over newlines, could return incorrect results etc. but an eyeball through the list shows the majority of cases exist. I'd argue it's a good first approximation for an order-of-magnitude on the code that I have checked out (platform, pde, jdt, egit etc).

My point is: it's trivial to do the non-lazy thing with ServiceTracker, and it's trivial to do the lazy thing with DS.
Comment 15 Alex Blewitt CLA 2020-07-03 04:33:17 EDT
I feel the intent of this bug has veered off into unnecessary debates as to the use of DS vs ServiceTracker, which wasn't the point of the enhancement.

The idea behind this enhancement was to provide a means of allowing third party bundles to publish an IResourceChangeListener, and have the core.resources bundle automatically wire it to the IWorkspace. This will allow the OSGi service runtime to handle registration/deregistration, and simplifies client-side use cases.

Whether or not this is handled with a DS component or adding to the uses of ServiceTracker/Customizer is an implementation detail.

There are 165 cases in my releng checkout that match this filter:

$ git submodule foreach "git grep getWorkspace\(\).addResourceChangeListener || :" | wc -l                                                                                                               165

They tend to look like:

Entering 'eclipse.platform.text'
org.eclipse.core.filebuffers/src/org/eclipse/core/internal/filebuffers/ResourceFileBuffer.java:                         fFile.getWorkspace().addResourceChangeListener(this);
org.eclipse.search/search/org/eclipse/search/internal/ui/SearchManager.java:            SearchPlugin.getWorkspace().addResourceChangeListener(this);
Entering 'eclipse.platform.ui'
bundles/org.eclipse.ui.ide/extensions/org/eclipse/ui/part/FileInPlaceEditorInput.java:                          getFile().getWorkspace().addResourceChangeListener(
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/IDEWorkbenchActivityHelper.java:             ResourcesPlugin.getWorkspace().addResourceChangeListener(listener);
bundles/org.eclipse.ui.ide/src/org/eclipse/ui/internal/ide/ResourceWorkingSetUpdater.java:              ResourcesPlugin.getWorkspace().addResourceChangeListener(this,

My argument is that we have a significant number of bundles that have explicit start-order dependencies because their activators trigger this pattern. To improve startup of Eclipse, we can move these client sites from 'please do this registration' to publication of a service, which then gets auto-wired up. Whether the client or server use explicit service registration or use DS is an implementation detail that shouldn't follow from this enhancement request.

org.eclipse.ant.ui
org.eclipse.core.tests.resources
org.eclipse.core.tools.resources
org.eclipse.e4.tools.emf.ui
org.eclipse.pde.api.tools
org.eclipse.pde.api.tools.tests
org.eclipse.pde.core
org.eclipse.pde.ds.annotations
org.eclipse.pde.ui
org.eclipse.pde.ui.templates
org.eclipse.platform.doc.isv
org.eclipse.releng.tools
org.eclipse.team.core
org.eclipse.team.cvs.core
org.eclipse.team.cvs.ui
org.eclipse.team.examples.filesystem
org.eclipse.team.ui
org.eclipse.ui.examples.navigator
org.eclipse.ui.ide
org.eclipse.ui.navigator.resources
org.eclipse.ui.tests.navigator
org.eclipse.ui.win32

---

What I'd like to propose is that if we have no concerns with the approach (of publishing and auto-subscribing the service) then we get this functionality in to 4.17-M1 which will allow us to experiment with publishing these changes. The existence of this functionality will not negatively affect any existing code, since they don't publish those services. We can mark it as experimental and not guarantee that it will be present in future milestones.

If there are desires for/against using DS I can provide an alternative implementation that uses ServiceTracker/Customiser in the existing ResourcesPlugin activator.
Comment 16 Christoph Laeubrich CLA 2020-07-03 04:36:50 EDT
(In reply to Alex Blewitt from comment #14)
> My point is: it's trivial to do the non-lazy thing with ServiceTracker, and
> it's trivial to do the lazy thing with DS.

I never seen that DS user-code regularly binds via ServiceReference and lazy-service fetching. DS do not any proxy magic at all if you have a reference to a service it will be fetched as well as it will be fetched when using null as the servicetracker, so your point simply compares different things.

And using immediate="true" contradicts the whole thing because your service will be classlaoded, and all the dependent classes will be loaded and so on what you initially wanted to avoid regardless of anyone ever will use/call the service.

Simply add an immediate service consuming your runner like you have done in the activator and you will see that there is no gain at all and your runner component will be activated as well.
Comment 17 Christoph Laeubrich CLA 2020-07-03 04:47:38 EDT
Sure it won't hurt (much), but will you archive the desired goal(s)?

Just taking a look at the source of FileSynchronizer it is a private inner class that has a reference to the outer class that supplies objects of interest.

The outer class itself (ResourceFileBuffer) is abstract and thus the needs to handle the bookkeeping outside in some way or the other.

And in fact there is no start-order dependency anyway, this would have mean that if your bundle start before the resource bundle it would fail what is not the case here.

There is a bundle-dependency but this only arises if your bundle can do useful work without the resource-bundle and all its classes.
Comment 18 Lars Vogel CLA 2020-07-03 05:39:29 EDT
Instead of discussing this on a theoretical level, I suggest to push a few Gerrit which are using this approach to see the change used in real code.

Also https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/165750 could IMHO be committed for testing, we can still change it or remove it in a later milestone.
Comment 19 Alex Blewitt CLA 2020-07-03 09:57:27 EDT
(In reply to Lars Vogel from comment #18)
> Instead of discussing this on a theoretical level, I suggest to push a few
> Gerrit which are using this approach to see the change used in real code.
> 
> Also
> https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/165750
> could IMHO be committed for testing, we can still change it or remove it in
> a later milestone.

Sounds good to me. Let me update the commit message so that it's mergeable. We can check that the component starts as expected when the core resource bundle starts, and that it doesn't interfere with the prompt for the location as highlighted by Andrei in #7.
Comment 20 Alex Blewitt CLA 2020-07-03 11:57:27 EDT
I've pushed up a new version of the DS configuration that removes the immediate="true" and then has a 1..n dependency on the IResourceChangeListeners, which means that the component will stay in an unsatisfied state until the first IResourceChangeListener is published. And since it has a static relationship with the IWorkspace, when the workspace goes away or all the resource change listeners go away then this component will become unsatisfied again.

Christoph/Alexei are you happier now?

Clients that wish to use this now can replace:

ResourcesPlugin.getWorkspace().addResourceChangeListener(xxx)

with

context.registerService(IResourceChangeListener.class,xxx,null)

and

ResourcesPlugin.getWorkspace().addResourceChangeListener(xxx, mask)

with

properties.put("event.mask",mask);
context.registerService(IResourceChangeListener.class,xxx,properties)

(or equivalent DS configuration)
Comment 21 Thomas Watson CLA 2020-07-07 08:36:18 EDT
To keep the discussion in the bug from the dev mailing list.  There was a suggestion to look at the use of OSGi Event Admin for event listeners  Here is one of my comments about the use of OSGi Event Admin for scenarios like IResourceChangeListener:

I do recall discussions to use event admin for some more events in the platform.  I don't recall if it was for resource change listeners.  Anyway, some of the complications with that were around scaling across all the other topics that may have events flying through the event admin pipeline.  At the moment the Equinox event admin async event "bus" (post vs send) is a single thread that services all topics.  This can quickly lead to a bottleneck if we have multiple topics that have large amounts of events being posted.

I'm do not know if the events sent to IResourceChangeListener are synchronous or asynchronous.  If asynchronous this could become a problem.
Comment 23 Lars Vogel CLA 2020-07-14 07:41:54 EDT
Can this one be closed? Do we have a follow-up bug to use this new approach in platform code?
Comment 24 Andrey Loskutov CLA 2020-07-14 07:46:05 EDT
(In reply to Lars Vogel from comment #23)
> Can this one be closed? Do we have a follow-up bug to use this new approach
> in platform code?

Two things: 
1) The added test seem not to be executed
2) Yes, a real client using this code in SDK would be good.
Comment 25 Alex Blewitt CLA 2020-07-14 09:47:32 EDT
Yes, my plan is now that it's merged to start making changes where it makes sense to use this pattern instead, so there will be follow up bugs.

The test shows up as having successfully run on e.g. https://download.eclipse.org/eclipse/downloads/drops4/I20200713-2230/testresults/html/org.eclipse.core.tests.resources_ep417I-unit-mac64-java11_macosx.cocoa.x86_64_11.html#org.eclipse.core.tests.resources.IResourceChangeListenerTest -- search for "testAutoPublishService"
Comment 26 Mickael Istria CLA 2021-03-31 11:45:34 EDT
@Alex: I noticed this didn't make it to the N&N documentation. Should we add it now?
Comment 27 Alex Blewitt CLA 2021-03-31 15:37:27 EDT
Yes, we probably should. Sorry, my bad ...