With the risk of sounding like a school teacher (which I'm trying to avoid)...
Browsing through some of the new tenant code, which is still a work in progress
to be fair, I found the following piece of code that I wanted to share with you:
public class TenantServiceFactory implements ManagedServiceFactory {
public static final String PID = "org.amdatu.core.tenant.factory";
public static final String ID_KEY = "id";
public static final String NAME_KEY = "name";
public static final String HOST_KEY = "host";
private final Map<String, Component> m_components = new HashMap<String,
Component>();
private volatile DependencyManager m_dependencyManager;
private volatile LogService m_logService;
public String getName() {
return PID;
}
public synchronized void updated(String pid, Dictionary/* <String, String>
*/properties)
throws ConfigurationException {
This always triggers my interest, when a whole method is being synchronized,
because that means that nothing inside this method should take a long time or
invoke other methods, unless we can 100% guarantee that what those methods do
is okay. If the code invokes any other service or any OSGi API including the
dependency manager, you are almost certainly setting yourself up for hard to
find deadlocks.
if (m_components.containsKey(pid)) {
m_logService.log(LogService.LOG_DEBUG, "pid already registered for
tenant '" + properties.get(ID_KEY)
+ "', skipping registration");
return;
}
As a side note, this is not the way to deal with configuration updates, but I
put that as a TODO in the documentation on Confluence for now.
String id = (String) properties.get(ID_KEY);
String name = (String) properties.get(NAME_KEY);
String host = (String) properties.get(HOST_KEY);
Properties serviceProperties = new Properties();
serviceProperties.put(Tenant.TENANT_ID_SERVICEPROPERTY, id);
serviceProperties.put(Tenant.TENANT_NAME_SERVICEPROPERTY, name);
serviceProperties.put(Tenant.TENANT_SERVICEPROPERTY + "hostname", host);
Tenant tenant = new TenantImpl(id, name);
tenant.putProperty("hostname", host);
Component component = m_dependencyManager.createComponent()
.setInterface(Tenant.class.getName(), serviceProperties)
.setImplementation(tenant);
Up to here we're fine. We are invoking the dependency manager, but only
declaring a component without actually handing it to the manager is fine.
m_dependencyManager.add(component);
This most certainly won't work, as adding a component to the manager can
generate a call chain that, via all kinds of services that you have no clue
about, might trigger a call to your own service. With the right mix of
multithreading and locking this will at some point go wrong. This must be
invoked without holding any locks.
m_components.put(pid, component);
}
public synchronized void deleted(String pid) {
The same applies here, you cannot call the log service while holding a lock.
Besides that, the framework already logs and sends events when services go
away, so there is little point in actually logging this ourselves here.
m_logService.log(LogService.LOG_INFO, "Unregistering tenant service
with pid: '" + pid + "'");
For removing things from the dependency manager, the same applies as adding
things in the sense that you just don't know what will be called because of
this, so you cannot hold the lock.
m_dependencyManager.remove(m_components.remove(pid));
}
}
Greetings, Marcel
_______________________________________________
Amdatu-developers mailing list
[email protected]
http://lists.amdatu.org/mailman/listinfo/amdatu-developers
