Skip to content

Fix deadlock #937 #939

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

Draft
basilevs wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Fix deadlock #937 #939

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e76faf
Fix deadlock #937
basilevs Aug 31, 2025
032ddd0
Do not leak a reference to removed service #937
basilevs Aug 31, 2025
8fb5d1d
Fix typo #937
basilevs Aug 31, 2025
e065a30
Allow stop procedure to access still running services #937
basilevs Sep 1, 2025
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
201 changes: 101 additions & 100 deletions ...g.eclipse.equinox.p2.core/src/org/eclipse/equinox/internal/p2/core/ProvisioningAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.net.URI;
import java.util.*;
import java.util.function.Consumer;
import org.eclipse.equinox.p2.core.IAgentLocation;
import org.eclipse.equinox.p2.core.IProvisioningAgent;
import org.eclipse.equinox.p2.core.spi.IAgentService;
Expand All @@ -28,14 +29,14 @@
/**
* Represents a p2 agent instance.
*/
public class ProvisioningAgent implements IProvisioningAgent, ServiceTrackerCustomizer<IAgentServiceFactory, Object> {
public class ProvisioningAgent implements IProvisioningAgent {

private final Map<String, Object> agentServices = Collections.synchronizedMap(new HashMap<>());
private final Map<String, Object> agentServices = Collections.synchronizedMap(new LinkedHashMap<>());
private BundleContext context;
private volatile boolean stopped = false;
private ServiceRegistration<IProvisioningAgent> reg;
private final Map<ServiceReference<IAgentServiceFactory>, ServiceTracker<IAgentServiceFactory, Object>> trackers = Collections
.synchronizedMap(new HashMap<>());
private final Map<String, ServiceTracker<IAgentServiceFactory, Object>> trackers = Collections
Copy link
Copy Markdown
Member

@laeubi laeubi Sep 1, 2025

Choose a reason for hiding this comment

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

I would here today use a ConcurrentHashMap, also instead of storing a ServiceTracker object it would be better to use a dedicated class (that internal holds / manages a ServiceTracker), then one can use a quite nice pattern in a way that one first computes that class and then sync on the methods of that particular class. That way the map can work completely lock-free.

Copy link
Copy Markdown
Contributor Author

@basilevs basilevs Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While lock-free map is nice, it is not critical in this case, because long-running operations are already done and managed by ServiceTracker outside of locks. The map only holds an instance of ServiceTracker, creation of which does not require any synchronization. ServiceTracker also provides necessary method synchronization, so no additional wrapping is needed. Indeed, ServiceTracker was designed for this exact purpose.

Registration order is important for correct disposal of interdependent services and LinkedHashMap preserves it unlike ConcurrentHashMap.

Also, performance is not a concern here, but computeIfAbsent() for ConcurrentHashMap carries same deadlock risks as Collections.synchronizedMap(), just hides some of conflicts.

.synchronizedMap(new LinkedHashMap<>());

/**
* Instantiates a provisioning agent.
Expand All @@ -50,42 +51,32 @@ public ProvisioningAgent() {
public Object getService(String serviceName) {
//synchronize so concurrent gets always obtain the same service
synchronized (agentServices) {
checkRunning();
Object service = agentServices.get(serviceName);
if (service != null) {
return service;
}
//attempt to get factory service from service registry
Collection<ServiceReference<IAgentServiceFactory>> refs;
try {
refs = context.getServiceReferences(IAgentServiceFactory.class,
String.format("(|(%s=%s)(p2.agent.servicename=%s))", //$NON-NLS-1$ use old property as fallback
IAgentServiceFactory.PROP_AGENT_SERVICE_NAME, serviceName, serviceName));
} catch (InvalidSyntaxException e) {
e.printStackTrace();
return null;
}
if (refs == null || refs.isEmpty()) {
return null;
}
ServiceReference<IAgentServiceFactory> firstRef = Collections.max(refs);
//track the factory so that we can automatically remove the service when the factory goes away
ServiceTracker<IAgentServiceFactory, Object> tracker = new ServiceTracker<>(context, firstRef, this);
tracker.open();
IAgentServiceFactory factory = (IAgentServiceFactory) tracker.getService();
if (factory == null) {
tracker.close();
return null;
}
service = factory.createService(this);
if (service == null) {
tracker.close();
return null;
}
registerService(serviceName, service);
trackers.put(firstRef, tracker);
return service;
}
ServiceTracker<IAgentServiceFactory, Object> tracker = trackers.computeIfAbsent(serviceName,
ref -> {
try {
if (stopped) {
return null;
}
Filter filter = context.createFilter(
String.format("(&(%s=%s)(|(%s=%s)(p2.agent.servicename=%s)))", //$NON-NLS-1$
Constants.OBJECTCLASS, IAgentServiceFactory.class.getName(), //
IAgentServiceFactory.PROP_AGENT_SERVICE_NAME, serviceName, //
serviceName)); // use old property as fallback
return new ServiceTracker<>(context, filter, trackerCustomizer);
} catch (InvalidSyntaxException e) {
throw new AssertionError(e);
Copy link
Copy Markdown
Contributor Author

@basilevs basilevs Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As serviceName is not validated, a syntax error may happen without any other error in agent code. AssertionError should only be thrown on internal errors. Either validate serviceName or throw IllegalArgumentException.

}
});
if (tracker == null) {
return null;
}
tracker.open();
return tracker.getService();
}

private void checkRunning() {
Expand Down Expand Up @@ -133,94 +124,104 @@ public void setLocation(URI location) {

@Override
public void unregisterService(String serviceName, Object service) {
synchronized (agentServices) {
if (stopped) {
return;
}
if (agentServices.get(serviceName) == service) {
agentServices.remove(serviceName);
}
}
agentServices.remove(serviceName, service);
Copy link
Copy Markdown
Member

@laeubi laeubi Sep 1, 2025

Choose a reason for hiding this comment

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

Also for the agentServices I would use ConcurrentHashMap

Copy link
Copy Markdown
Contributor Author

@basilevs basilevs Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ConcurrentHashMap does not preserve registration order, which is used to dispose services. See stop()

if (service instanceof IAgentService) {
((IAgentService) service).stop();
}
}

@Override
public void stop() {
List<Object> toStop;
synchronized (agentServices) {
toStop = new ArrayList<>(agentServices.values());
}
//give services a chance to do their own shutdown
for (Object service : toStop) {
if (service instanceof IAgentService) {
if (service != this) {
((IAgentService) service).stop();
}
}
}
stopped = true;
//close all service trackers
synchronized (trackers) {
for (ServiceTracker<IAgentServiceFactory, Object> t : trackers.values()) {
t.close();
try {
disposeInTurns(agentServices.entrySet(), entry -> {
unregisterService(entry.getKey(), entry.getValue());
});
} finally {
try {
disposeInTurns(trackers.values(), ServiceTracker::close);
} finally {
if (reg != null) {
reg.unregister();
reg = null;
}
}
trackers.clear();
}
if (reg != null) {
reg.unregister();
reg = null;
}
}

public void setServiceRegistration(ServiceRegistration<IProvisioningAgent> reg) {
this.reg = reg;
}

@Override
public Object addingService(ServiceReference<IAgentServiceFactory> reference) {
if (stopped) {
return null;
private final ServiceTrackerCustomizer<IAgentServiceFactory, Object> trackerCustomizer = new ServiceTrackerCustomizer<>() {
@Override
public Object addingService(ServiceReference<IAgentServiceFactory> reference) {
if (stopped) {
return null;
}
Object result = context.getService(reference).createService(ProvisioningAgent.this);
if (result instanceof IAgentService agentService) {
agentService.start();
}
return result;
}
return context.getService(reference);
}

@Override
public void modifiedService(ServiceReference<IAgentServiceFactory> reference, Object service) {
//nothing to do
}

@Override
public void removedService(ServiceReference<IAgentServiceFactory> reference, Object factoryService) {
if (stopped) {
return;
@Override
public void modifiedService(ServiceReference<IAgentServiceFactory> reference, Object service) {
// nothing to do
Copy link
Copy Markdown
Contributor Author

@basilevs basilevs Nov 14, 2025

Choose a reason for hiding this comment

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

Explain why service reset is not required here.

}
String serviceName = getAgentServiceName(reference);
if (serviceName == null) {
return;
}
Object registered = agentServices.get(serviceName);
if (registered == null) {
return;
}
if (FrameworkUtil.getBundle(registered.getClass()) == FrameworkUtil.getBundle(factoryService.getClass())) {
//the service we are holding is going away
unregisterService(serviceName, registered);
ServiceTracker<IAgentServiceFactory, Object> toRemove = trackers.remove(reference);
if (toRemove != null) {
toRemove.close();

@Override
public void removedService(ServiceReference<IAgentServiceFactory> reference, Object service) {
if (service instanceof IAgentService agentService) {
agentService.stop();
}
if (service != null) {
context.ungetService(reference);
}
}
}

private String getAgentServiceName(ServiceReference<IAgentServiceFactory> reference) {
Object property = reference.getProperty(IAgentServiceFactory.PROP_AGENT_SERVICE_NAME);
if (property instanceof String s) {
return s;
};

/**
* Given a thread-safe collection, drain all elements one-by-one, disposing each
* without holding locks and allowing concurrent modification form. Other
* threads should not attempt to add elements to collection.<br>
* Usage example:
*
* <pre>{@code
* volatile boolean stopped = false;
* ...
* stopped = true; // prevent other threads from adding to the collection
* disposeInTurns(agentServices.entrySet(), entry -> {
* unregisterService(entry.getKey(), entry.getValue());
* });
* }</pre>
*
*/
@SuppressWarnings("unchecked")
private <T> void disposeInTurns(Collection<T> input, Consumer<T> dispose) {
RuntimeException error = null;
while (!input.isEmpty()) {
Object[] toStop = input.toArray(Object[]::new);
Collections.reverse(Arrays.asList(toStop)); // Remove in an inverse order of adding
for (Object item : toStop) {
if (input.remove(item)) {
try {
dispose.accept((T) item);
} catch (RuntimeException e) {
if (error == null) {
error = e;
} else {
error.addSuppressed(e);
}
}
}
}
}
if (error != null) {
throw error;
}
// backward compatibility
return (String) reference.getProperty("p2.agent.servicename"); //$NON-NLS-1$
}

}
Loading