eclipse-platform / eclipse.platform.resources

Eclipse Public License 2.0
3 stars 18 forks source link

Cyclic dependency between ProjectPreferences and Workspace init #86

Closed laeubi closed 2 years ago

laeubi commented 2 years ago

Open the workspace has a cyclic dependency to the ProjectPreferences:

at org.eclipse.core.resources.ResourcesPlugin.getWorkspace(ResourcesPlugin.java:427)
    at org.eclipse.core.internal.resources.ProjectPreferences.<init>(ProjectPreferences.java:377)
    at org.eclipse.core.internal.resources.ProjectPreferences.internalCreate(ProjectPreferences.java:489)
    at org.eclipse.core.internal.preferences.EclipsePreferences.create(EclipsePreferences.java:350)
    at org.eclipse.core.internal.preferences.EclipsePreferences.internalNode(EclipsePreferences.java:620)
    at org.eclipse.core.internal.preferences.EclipsePreferences.node(EclipsePreferences.java:759)
    at org.eclipse.core.internal.preferences.PreferencesService$5.run(PreferencesService.java:620)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
    at org.eclipse.core.internal.preferences.PreferencesService.getNodes(PreferencesService.java:600)
    at org.eclipse.core.internal.preferences.PreferencesService.getString(PreferencesService.java:686)
    at org.eclipse.core.internal.resources.Workspace.setExplicitWorkspaceEncoding(Workspace.java:2280)
    at org.eclipse.core.internal.resources.Workspace.open(Workspace.java:2240)
laeubi commented 2 years ago

@iloveeclipse we have here a cyclic dependency that assumes that the Workspace object is visible to the static getter before it is opened. Actually it is not interested in the Workspace itself but the workspace root to get a reference to a project here.

iloveeclipse commented 2 years ago

Not nice but not really dangerous - that is part of internal init code that happens (today) in the same init thread guarded by the resources bundle activation lock.

laeubi commented 2 years ago

The bad think is that the static getter needs to make an not fully initialized workspace available to the outside world. Do you know if setExplicitWorkspaceEncoding really needs to be set on Workspace#open or we might call ProjectPreferences directly instead of going through the preference service jsut to be redirected to our own code again?

laeubi commented 2 years ago

By the way it currently only works because the workspace is assigned before open here, even though the comment claims it is just for debugging purpose... https://github.com/eclipse-platform/eclipse.platform.resources/blob/06a0513082598a8e976ed5179e2e978f9b47baea/bundles/org.eclipse.core.resources/src/org/eclipse/core/resources/ResourcesPlugin.java#L474-L476

iloveeclipse commented 2 years ago

I would assume workspace location need to be known if one wants to check workspace settings.

Please note, exposing workspace for resources internal init code is OK, and again, that is done today guarded by the equinox init sequence (the no one can access bundle classes until it is fully activated).

If we refactor the code so the workspace is early accessible to external clients, it should be either guarded by some lock or fully initialized before.

laeubi commented 2 years ago

I would assume workspace location need to be known if one wants to check workspace settings.

The location is know, but not to the preference service (at that time) so I wonder if we simply could call the setExplicitWorkspaceEncoding right after we have opened the workspace?

Please note, exposing workspace for resources internal init code is OK, and again, that is done today guarded by the equinox init sequence

I just think exposing it through the static assessor is bad, because the "guarded" is limited to other threads, if we call code in the same thread like here, then other code can (if implicitly called) access the workspace already as shown in the stack trace.

If we refactor the code so the workspace is early accessible to external clients

I don't think that's necessary, I'll adjust the code to support this last side-band communication but think we better find a way that do not need this.