Make ResourcesPlugin more dynamic and better handling early start-up

[laeubi]

Currently ResourcesPlugin is very sensible in respect to be started "too early", and badly fails then.

One path is show in this stack-trace:

Caused by: org.osgi.framework.BundleException: Exception in org.eclipse.core.resources.ResourcesPlugin.start() of bundle org.eclipse.core.resources.
    at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:834)
    at org.eclipse.osgi.internal.framework.BundleContextImpl.start(BundleContextImpl.java:762)
    at org.eclipse.osgi.internal.framework.EquinoxBundle.startWorker0(EquinoxBundle.java:1032)
    at org.eclipse.osgi.internal.framework.EquinoxBundle$EquinoxModule.startWorker(EquinoxBundle.java:371)
    at org.eclipse.osgi.container.Module.doStart(Module.java:605)
    at org.eclipse.osgi.container.Module.start(Module.java:468)
    at org.eclipse.osgi.framework.util.SecureAction.start(SecureAction.java:513)
    at org.eclipse.osgi.internal.hooks.EclipseLazyStarter.postFindLocalClass(EclipseLazyStarter.java:117)
    ... 94 more
Caused by: java.lang.NullPointerException
    at org.eclipse.core.internal.runtime.InternalPlatform.getLocation(InternalPlatform.java:347)
    at org.eclipse.core.runtime.Platform.getLocation(Platform.java:766)
    at org.eclipse.core.internal.resources.WorkspaceRoot.<init>(WorkspaceRoot.java:42)
    at org.eclipse.core.internal.resources.Workspace.<init>(Workspace.java:105)
    at org.eclipse.core.resources.ResourcesPlugin.start(ResourcesPlugin.java:474)
    at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:813)
    at org.eclipse.osgi.internal.framework.BundleContextImpl$2.run(BundleContextImpl.java:1)
    at java.base/java.security.AccessController.doPrivileged(Native Method)
    at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:805)

The reason for this is, that it uses a static access to other resources, that are not present at that time (e.g. the Workspace-Chooser dialog is open). OSGi encourages to use services instead of static singletons because of the following advantages:

1. One can wait for a service and get notified when its gone
2. If handled properly there is no risk in using stale or uninitialized objects
3. There is no start-order dependency and a service based approach generally behaves more responsive e.g. a component waiting for the database services does not blocks one that has no need for a database, while in a traditional setup one has to wait until the "setup-phase" has started the database as no one knows when the static method will be called.
4. It is much easier e.g. using DS to get know what a bundle/component requires to function properly
5. Testing is improved, as one can simply mock that service and there is no need to get headaches about how to get all that singletons to the right state and how to avoid one test is changing state of another one.

[ruspl-afed]

I understand your arguments, but practically removing of ResourcePlugin.getWorkspace() will mean a major break of API since Eclipse 3.x And the question is: do we value this improvement more than all the existing plug-ins for Eclipse IDE?

[laeubi]

It is not removed (just deprecated and marked for removal), and yes this will hopefully have the value that people stop using static access where it is not required and move to a service based model making the whole stuff more flexible, save and prevent the dreaded 'plugin started to early' problem.

[ruspl-afed]

It is not removed (just deprecated and marked for removal)

It is sufficient to indicate the plans and the way of further development. Please don't get me wrong, I'm not a fan of static access and always fighting with it. But for this particular case the impact of just deprecated and marked for removal for the whole Eclipse community could be more heavy than switching to Java 11. The reason is simple: the good times of investments are over for the majority of Eclipse-based code and the change like this will be equal to the "enf-of-life" for many plug-ins and Eclipse-based products.

You can try to collect feedback at cross-project mailing list and see what community thinks.

[merks]

Please no! I express much the same concerns here:

https://github.com/eclipse-platform/eclipse.platform.resources/pull/53#issuecomment-1099884586

You can do all those things if you want to do all those things, but please do not force everyone to do those things. We're all busy people and want to do other things, not the things you decide for us we should do because in principle it's such a good thing.

[laeubi]

You can do all those things if you want to do all those things, but please do not force everyone to do those things.

No one is forced to do anything, but without strongly discourage this plain convenience access no one will ever change.

As mentioned above the new way to access the IWorkspace is more than 13 year there ...

[merks]

I have warning free workspaces for all my projects, so I am forced to do something. Marking it for deletion is a screaming red flag on top of that. (Oh god no, when will it be deleted?!) So I will have to do something. Maybe lots of people will ignore it. How many people will do something about it? Will the platform project sweep through the code and eliminate all calls? If not, why not all? Can they all be replaced?

Please also assume that I'm stupid and I don't have a favorite replacement technique and moreover assume that I do not understand what a declarative service is, I do not know how to use a service tracker, and that I am under the impression that a blueprint is for building a building. I,e., assume that I am too stupid to know how to fix this method:

  /**
   * Returns the workspace root, or <code>null</code>, if the runtime environment is stand-alone.
   * @return the workspace root, or <code>null</code>.
   */
  public static IWorkspaceRoot getWorkspaceRoot()
  {
    if (workspaceRoot == null && IS_RESOURCES_BUNDLE_AVAILABLE && System.getProperty("org.eclipse.emf.ecore.plugin.EcorePlugin.doNotLoadResourcesPlugin") == null)
    {
      workspaceRoot = ResourcesPlugin.getWorkspace().getRoot();
    }
    return workspaceRoot;
  }

And I mean this literally, I haven't a clue.

[laeubi]

Can they all be replaced?

Will the world explode tomorrow? We never know... anyways this static access to the Resource Plugin is one of the worst things in eclipse platform and many effort has been taken to circumvent this bad design decision. So if one want to have software that not just works under some specific circumstance one better starts now to adapt-

assume that I am too stupid to know how to fix this method:

This code is broken anyways as referencing IWorkspaceRootwill

1. require that the ResourcePlugin (or some surrogate) is already there in contrast to what it claims with IS_RESOURCES_BUNDLE_AVAILABLE
2. load the resource plugin anyways as it is marked as lazy in contrast to what org.eclipse.emf.ecore.plugin.EcorePlugin.doNotLoadResourcesPlugin claims to archive

beside that this perfectly show the flawed design of the static access as you better would have traced the service as an optional dependency without any need of all those workarounds.

And I mean this literally, I haven't a clue.

It heavily depends on who is calling that and whats the purpose of the call (e.g. what should happen if the IWorkspaceRoot is null) on how to do it best. In the simplest case you can just use your existing activator to track that service and return it in a static way.

[laeubi]

By the way just a good example is the following commit: https://github.com/eclipse-platform/eclipse.platform.resources/pull/53/commits/58e9ad095625a2bcfc9a5bbf7167d8b95e6b2334

As one can see the IWorkspace is already there, but still the static method is called. We all know there should be only one IWorkspace ever, this results in possibly using a different object in the one class versus the other (just assume a test want to mock different workspace here) and even if the caller passes a mocked IWorkspace the code downstream will work a different one or more probably simply fail with an Exception...

By the @vogella complained some time back about the usage of Activators and slow start up of the IDE, currently a plugin can't start while the Workspace Chooser is displayed if it references any class from the resources bundle even though it does not care about the workspace at that point of time....

[merks]

If I have to answer questions about the 'world exploding nor not,' I know that we have strayed from a technical discussion about the impact of your changes on the community. That makes me feel that further comments will just as likely be ridiculed as they are to be taken into reasonable consideration.

[laeubi]

If I have to answer questions about the 'world exploding nor not,'

Even if I knew that tomorrow the world would go to pieces, I would still open a PR at eclipse today. ;-)

the impact of your changes on the community.

To show that I care about all community concerns I just removed the deprecation and replaced it with a kind hint to consider alternative techniques.

[merks]

@laeubi

Thank you.

I do agree that the singleton is a poor design (that is unfortunately reflected in EMF's APIs) where it has caused exactly the kind of problems you describe above and I've had to be more careful to access it lazily in the places it's needed/used.

[mickaelistria]

I have warning free workspaces for all my projects, so I am forced to do something.

It's again the same brake as many API or workflow enhancement that were suggested and rejected earlier (generifying JFace, more error reporting...). But it's not really fair to the project: the fact that you have configured your workspace and workflow in a way that makes you intolerant to any evolution that would surface as a new warning in your code cannot be fairly interpreted as a reason for the project to reject a deprecation or evolution proposal. The Platform code should be treated in a way that's conform it it's contract, it's API contract. And nothing says "nothing should be changed if it shows new warnings". Warnings are exactly meant to make people change their code towards something better, without breaking them if they don't. They're useful, trying to prevent new warnings from appearing is actually preventing from software in general to better.

That said, I agree with some shorter-term and specific concerns; this is a critical API, one of the most critical entry points in Platform code and we should not go too fast, and ensure all code we maintain can work without this API before deprecating it. And even if we deprecate it, I'm not sure it's even meant to be ever removed (so forRemoval=true shouldn't be set anyway) as we want to ensure backward compatibility. So IMO the plan would be to progressively ensure we get rid of ResourcesPlugin.getWorkspace() in all Platform/JDT/PDE code and then consider deprecation. This would serve the goal of new or maintained code adopting better code, without breaking compatibility. So +1 to change Platform code to avoid ResourcesPlugin.getWorkspace() and -1 to deprecate this method before we don't use it any more in Platform.

[merks]

@mickaelistria I generally agree with your assessment and commentary. I don't think generifying JFace was a case in point because arrays and generics don't interact nicely so there were intrinsic technical reasons, but that's not relevant to this discussion. Also fairness is a two-edged sword; when expecting fairness one must dish out fairness, and I'm not suggesting that you're not be fair, it's just a generalized observation and in fact I think you are being very fair and reasonable. I definitely agree that there are many situations to deprecate things, and I too want the platform to remain vibrant and to demonstrate best practices.

[laeubi]

I have created an example her for m2e getting rid of some of the static references: https://github.com/eclipse-m2e/m2e-core/pull/642/files

[laeubi]

This is currently blocked by: https://github.com/eclipse-equinox/equinox.framework/issues/38