Skip to content

Defer IWorkspace service registration onto a background job #5

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.

Sign up for GitHub

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

Open
vogella wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Defer IWorkspace service registration onto a background job #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 38 additions & 6 deletions ...es/bundles/org.eclipse.core.resources/src/org/eclipse/core/resources/ResourcesPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,8 @@ public void start(BundleContext context) throws Exception {
private final class WorkspaceInitCustomizer implements ServiceTrackerCustomizer<Location, Workspace> {
private final BundleContext context;
private volatile Workspace workspace;
private ServiceRegistration<IWorkspace> workspaceRegistration;
private volatile ServiceRegistration<IWorkspace> workspaceRegistration;
private volatile Job workspaceRegistrationJob;

private WorkspaceInitCustomizer(BundleContext context) {
this.context = context;
Expand All @@ -592,7 +593,27 @@ public Workspace addingService(ServiceReference<Location> reference) {
if (!result.isOK()) {
getLog().log(result);
}
workspaceRegistration = context.registerService(IWorkspace.class, workspace, null);
// Publish IWorkspace asynchronously: registerService dispatches
// ServiceEvent.REGISTERED synchronously to every
// ServiceTracker<IWorkspace> listener, and several of them
// (e.g. save-participant registrations) do non-trivial work
// that would otherwise block the startup critical path.
// Static callers using ResourcesPlugin.getWorkspace() are
// unaffected since it reads the field directly.
Job job = Job.create("Register IWorkspace service", (IProgressMonitor monitor) -> { //$NON-NLS-1$
Workspace ws = workspace;
if (ws != null) {
try {
workspaceRegistration = context.registerService(IWorkspace.class, ws, null);
} catch (IllegalStateException e) {
// bundle context became invalid during shutdown
}
}
return Status.OK_STATUS;
});
Comment on lines +603 to +613
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential service leaks during rapid startup/shutdown or service removal, the background job should check if it has been canceled before proceeding with the service registration. This works in conjunction with calling job.cancel() in removedService.

Suggested change
Job job = Job.create("Register IWorkspace service", (IProgressMonitor monitor) -> { //$NON-NLS-1$
Workspace ws = workspace;
if (ws != null) {
try {
workspaceRegistration = context.registerService(IWorkspace.class, ws, null);
} catch (IllegalStateException e) {
// bundle context became invalid during shutdown
}
}
return Status.OK_STATUS;
});
Job job = Job.create("Register IWorkspace service", (IProgressMonitor monitor) -> { //$NON-NLS-1$
Workspace ws = workspace;
if (ws != null && !monitor.isCanceled()) {
try {
workspaceRegistration = context.registerService(IWorkspace.class, ws, null);
} catch (IllegalStateException e) {
// bundle context became invalid during shutdown
}
}
return Status.OK_STATUS;
});

job.setSystem(true);
workspaceRegistrationJob = job;
job.schedule();
return workspace;
} catch (CoreException e) {
getLog().log(e.getStatus());
Expand All @@ -611,10 +632,21 @@ public void modifiedService(ServiceReference<Location> reference, Workspace serv
@Override
public void removedService(ServiceReference<Location> reference, Workspace service) {
if (service == workspace) {
try {
workspaceRegistration.unregister();
} catch (RuntimeException e) {
getLog().log(Status.warning("Unregistering workspaces throws an exception", e)); //$NON-NLS-1$
Job job = workspaceRegistrationJob;
if (job != null) {
try {
job.join();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
ServiceRegistration<IWorkspace> registration = workspaceRegistration;
if (registration != null) {
try {
registration.unregister();
} catch (RuntimeException e) {
getLog().log(Status.warning("Unregistering workspaces throws an exception", e)); //$NON-NLS-1$
}
}
Comment on lines +635 to 650
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It is recommended to cancel the registration job before joining it to ensure that if the job hasn't started yet, it won't run at all. Additionally, nulling out the registration fields after unregistering is a good practice to avoid stale references and potential double-unregistration issues.

				Job job = workspaceRegistrationJob;
				if (job != null) {
					job.cancel();
					try {
						job.join();
					} catch (InterruptedException e) {
						Thread.currentThread().interrupt();
					}
					workspaceRegistrationJob = null;
				}
				ServiceRegistration<IWorkspace> registration = workspaceRegistration;
				if (registration != null) {
					workspaceRegistration = null;
					try {
						registration.unregister();
					} catch (RuntimeException e) {
						getLog().log(Status.warning("Unregistering workspaces throws an exception", e)); //$NON-NLS-1$
					}
				}

try {
service.close(null);
Expand Down