-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ResourcesPlugin: Multithreaded lazy start #1219
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,13 +27,21 @@ | |
import org.eclipse.core.internal.resources.Workspace; | ||
import org.eclipse.core.internal.utils.Messages; | ||
import org.eclipse.core.internal.utils.Policy; | ||
import org.eclipse.core.runtime.*; | ||
import org.eclipse.core.runtime.CoreException; | ||
import org.eclipse.core.runtime.IProgressMonitor; | ||
import org.eclipse.core.runtime.IStatus; | ||
import org.eclipse.core.runtime.Platform; | ||
import org.eclipse.core.runtime.Plugin; | ||
import org.eclipse.core.runtime.Status; | ||
import org.eclipse.core.runtime.jobs.IJobManager; | ||
import org.eclipse.core.runtime.jobs.Job; | ||
import org.eclipse.osgi.service.datalocation.Location; | ||
import org.eclipse.osgi.service.debug.DebugOptions; | ||
import org.eclipse.osgi.service.debug.DebugOptionsListener; | ||
import org.osgi.framework.*; | ||
import org.osgi.framework.BundleActivator; | ||
import org.osgi.framework.BundleContext; | ||
import org.osgi.framework.ServiceReference; | ||
import org.osgi.framework.ServiceRegistration; | ||
import org.osgi.util.tracker.ServiceTracker; | ||
import org.osgi.util.tracker.ServiceTrackerCustomizer; | ||
|
||
|
@@ -419,7 +427,7 @@ public final class ResourcesPlugin extends Plugin { | |
* @see #getSystemEncoding() | ||
*/ | ||
public static String getEncoding() { | ||
ResourcesPlugin resourcesPlugin = plugin; | ||
ResourcesPlugin resourcesPlugin = getPlugin(); | ||
if (resourcesPlugin == null) { | ||
return getSystemEncoding(); | ||
} | ||
|
@@ -475,7 +483,21 @@ private static String getSystemEncoding() { | |
* @return the single instance of this plug-in runtime class | ||
*/ | ||
public static ResourcesPlugin getPlugin() { | ||
return plugin; | ||
if (plugin == null) { | ||
return null; | ||
} | ||
return plugin.initialized(); | ||
} | ||
|
||
private ResourcesPlugin initialized() { | ||
ServiceTracker<Location, Workspace> t = instanceLocationTracker; | ||
if (t != null && t.size() == 0) { | ||
synchronized (t) { | ||
// lazy initialize | ||
t.open(); | ||
} | ||
} | ||
return this; | ||
} | ||
|
||
/** | ||
|
@@ -491,10 +513,8 @@ public static ResourcesPlugin getPlugin() { | |
* class. | ||
*/ | ||
public static IWorkspace getWorkspace() { | ||
ResourcesPlugin resourcesPlugin = plugin; | ||
ResourcesPlugin resourcesPlugin = getPlugin(); | ||
if (resourcesPlugin == null) { | ||
// this happens when the resource plugin is shut down already... or never | ||
// started! | ||
throw new IllegalStateException(Messages.resources_workspaceClosedStatic); | ||
} | ||
Workspace workspace = resourcesPlugin.workspaceInitCustomizer.workspace; | ||
|
@@ -520,6 +540,7 @@ public void stop(BundleContext context) throws Exception { | |
// save the preferences for this plug-in | ||
getPlugin().savePluginPreferences(); | ||
plugin = null; | ||
instanceLocationTracker = null; | ||
} | ||
|
||
/** | ||
|
@@ -542,7 +563,7 @@ public void start(BundleContext context) throws Exception { | |
workspaceInitCustomizer); | ||
plugin = this; // must before open the tracker, as this can cause the registration of the | ||
// workspace and this might trigger code that calls the static method then. | ||
instanceLocationTracker.open(); | ||
new Thread(this::initialized, "Async ResourcesPlugin start").start(); //$NON-NLS-1$ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still wrong, the contract of the Resources plugin is that it tracks the workspace in the activator, as the activation usually (but that's not guaranteed!) happens by a lazy-activation and in this case must be guarded by the classloader lock. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Cristoph. The code expected to have workspace initialized after activator is done, not asynchronously after that. Please put breakpoint at Initialization of ResourcesPlugin should be changed very carefully, there are different actors playing here and not everything can be validated via automated tests. |
||
} | ||
|
||
private final class WorkspaceInitCustomizer implements ServiceTrackerCustomizer<Location, Workspace> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must not call
get()
here as it blocks, and you don't want to be blocked. Instead you should either return aStream<..>
with anonClose( ... )
, aFuture<..>
or similar or even better aCompletableFuture<..>
then you can use that in an event driven non blocking way.