[The message body disappeared for some reason, resending...]
Hi,
While trying to use DOSGi (1.4.0, in Karaf 2.3.1) I came across some
serious issues, namely when starting up a fresh Karaf instance
repeatedly, only about 50% of the time the service actually got exported
(as verified with a browser pointing to the appropriate WSDL URL). This
is just on a single instance on a local host, before trying to do
anything remotely. I started investigating into the matter, which
required quite a lot of debugging and code reviewing, and found various
issues with the (strangely named?) TopologyManagerExport class which
seems to be at the heart of the matter. This took several iterations and
includes also non-critical cleanups I did along the way - I'm attaching
the final version I arrived at which hopefully addresses the issues,
though due to the law of clustering of bugs, I wouldn't be surprised if
there are other new or old bugs still lurking there... any feedback and
additional code reviews would be greatly appreciated.
Here is a summary of all the changes, big and small:
- fixed typos in docs and logs.
- fixed formatting to be more consistent and readable.
- added debug logs to RemoteServiceAdminLifeCycleListener.added/removed
callbacks (this proved the serious tracker bug below, but is also useful
for general debugging).
- simplified shouldExportService returned boolean expression.
- replaced iterations on map keys followed by a get of the corresponding
value with an iteration on Map.Entry (prevents unnecessary lookups).
- replaced map containsKey checks followed by get with a get followed by
null check (prevents unnecessary lookups, and theoretically race
conditions too).
- fixed tracker misuse: when tracker callback (added) is called, the
tracker itself does not yet reflect the new state, so accessing services
(or size) on the tracker from within the callback will fail to include
the new service. Although the actual service export triggered by the
callback is performed on a separate thread, this still leaves a race
condition between the two which sometimes causes the export to fail
(with "No RemoteServiceAdmin available!" log). To fix this, the newly
added RSA is passed down the method chain to the doExportService method
explicitly. doExportService then uses the reference if it is given, or
if null (as when called from elsewhere), uses the tracker to get the
service list as it did in the past.
- fixed exportService always overwriting a service's entry in
exportedServices with a new empty data structure - now it only does so
if it does not already exist.
- fixed exportedServices inner maps to be created as synchronized maps -
previously doExportService wrapped it in a synchronized map on each
invocation, which results effectively in no synchronization, since the
synchronized wrapper map is local only so each thread accessing the map
uses a different lock. Also fixed other accesses of the inner maps to be
synchronized (Collections.synchronizedMap requires external
synchronization on it when being iterated, as specified in the docs).
- fixed exports null check in doExportService (in the old code,
Collections.synchronizedMap never returns null; In the new code it makes
more sense.)
- removed redundant remoteServiceAdminTracker null check in
doExportService (it is already used in the constructor, so can't be null
at this point).
- added cast of null to String when calling getServiceReferences in
exportExistingServices, so that it will compile also with the OSGi 4.3.0
and later API (which added an overloaded method accepting a Class). Does
not affect previous versions.
- removed removeExportRegistration, removeExportReference and
remoteAdminEvent methods, all of which are both unused and have a no-op
implementation.
Feel free to pick out the changes u like...
In addition, I had a few related questions/thoughts, I hope someone here
can help with them:
1. Is DOSGi being updated to work with OSGi 4.3.0/Felix 4.x/Karaf 2.3.x
etc.? It seems to be slightly outdated. At a glance, it looks like
making it compile against OSGi 4.3.0 requires just a few
backward-compatible syntax changes, and hopefully the semantics have not
changed too much.
2. The website says on the Karaf feature page: "CXF DOSGi does not work
with Karaf 2.3.0", however it does not mention whether this applies to
2.3.1 as well (is it a DOSGi issue or Karaf issue that prevents it from
working?), nor does it say anything about what the problem is, where it
might be relevant, or link to any issue in JIRA - could someone in the
know please update the text to be a bit more informative? Also, is this
indeed just an issue with installation via the feature, or with
compatibility between the projects/versions in general regardless of
installation method?
3. After all the fixes above, the export success rate is definitely
better than 50%, but still isn't 100% - every once in a while the export
still fails. Looking through the logs, I found the message
"RemoteServiceAdmin Implementation is shutting down now" - during Karaf
startup! I'm still not sure what exactly is going on, but it appears
that the RSA is started, services exported properly etc., but then it
suddenly commits suicide a few moments later with no other error or
indication of who/what initiated the shutdown or why. The initial theory
is that the dsw Activator might be getting a ConfigurationAdmin updated
event with null configuration, which would trigger a shutdown - but I
haven't yet confirmed that this is the case, nor why it would happen or
what the proper behavior in such a case should be.
I'm totally new to CXF and DOSGi (and pretty new to OSGi too), so please
forgive any silly mistakes or assumptions above... but I do hope this
will be useful in making it a better product and making it work better
out of the box :-)
Thanks to anyone who read this far!
Amichai
Index: dsw/cxf-topology-manager/src/main/java/org/apache/cxf/dosgi/topologymanager/exporter/TopologyManagerExport.java
===================================================================
--- dsw/cxf-topology-manager/src/main/java/org/apache/cxf/dosgi/topologymanager/exporter/TopologyManagerExport.java (revision 1457734)
+++ dsw/cxf-topology-manager/src/main/java/org/apache/cxf/dosgi/topologymanager/exporter/TopologyManagerExport.java (working copy)
@@ -19,6 +19,7 @@
package org.apache.cxf.dosgi.topologymanager.exporter;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
@@ -36,11 +37,9 @@
import org.osgi.framework.ServiceEvent;
import org.osgi.framework.ServiceListener;
import org.osgi.framework.ServiceReference;
-import org.osgi.service.remoteserviceadmin.ExportReference;
import org.osgi.service.remoteserviceadmin.ExportRegistration;
import org.osgi.service.remoteserviceadmin.RemoteConstants;
import org.osgi.service.remoteserviceadmin.RemoteServiceAdmin;
-import org.osgi.service.remoteserviceadmin.RemoteServiceAdminEvent;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -61,22 +60,22 @@
/**
* Holds all services that are exported by this TopologyManager for each ServiceReference that should be
* exported a map is maintained which contains information on the endpoints for each RemoteAdminService
- *
+ *
* <pre>
* Bsp.:
- * ServiceToExort_1
+ * ServiceToExport_1
* ---> RemoteAdminService_1 (CXF HTTP)
* --------> List<EndpointDescription> -> {http://localhost:1234/greeter, http://localhost:8080/hello}
* ---> RemoteAdminService_2 (ActiveMQ JMS/OpenWire)
* --------> List<EndpointDescription> -> {OpenWire://127.0.0.1:123/testQueue}
- * ServiceToExort_2
+ * ServiceToExport_2
* ---> RemoteAdminService_1 (CXF HTTP)
* --------> List<EndpointDescription> -> {empty} // not exported yet or not suitable
- *
+ *
* </pre>
*/
- private final Map<ServiceReference,
- Map<RemoteServiceAdmin, Collection<ExportRegistration>>> exportedServices =
+ private final Map<ServiceReference,
+ Map<RemoteServiceAdmin, Collection<ExportRegistration>>> exportedServices =
new LinkedHashMap<ServiceReference, Map<RemoteServiceAdmin, Collection<ExportRegistration>>>();
public TopologyManagerExport(BundleContext ctx, RemoteServiceAdminTracker rsaTracker) {
@@ -86,10 +85,12 @@
this.remoteServiceAdminTracker.addListener(new RemoteServiceAdminLifeCycleListener() {
public void added(RemoteServiceAdmin rsa) {
+ LOG.debug("RemoteServiceAdminTracker added {}, size is {}", rsa, remoteServiceAdminTracker.size());
triggerExportForRemoteServiceAdmin(rsa);
}
public void removed(RemoteServiceAdmin rsa) {
+ LOG.debug("RemoteServiceAdminTracker removed {}, size is {}", rsa, remoteServiceAdminTracker.size());
removeRemoteServiceAdmin(rsa);
}
});
@@ -99,7 +100,7 @@
if (event.getType() == ServiceEvent.REGISTERED) {
LOG.debug("Received REGISTERED ServiceEvent: {}", event);
if (shouldExportService(sref)) {
- exportService(sref);
+ exportService(sref, null);
}
} else if (event.getType() == ServiceEvent.UNREGISTERING) {
LOG.debug("Received UNREGISTERING ServiceEvent: {}", event);
@@ -107,18 +108,15 @@
}
}
};
-
+
epListenerNotifier = new EndpointListenerNotifier(ctx, this);
}
-
+
/**
* checks if a Service is intended to be exported
*/
private boolean shouldExportService(ServiceReference sref) {
- if (sref.getProperty(RemoteConstants.SERVICE_EXPORTED_INTERFACES) != null) {
- return true;
- }
- return false;
+ return sref.getProperty(RemoteConstants.SERVICE_EXPORTED_INTERFACES) != null;
}
/**
@@ -127,10 +125,9 @@
*/
protected void removeRemoteServiceAdmin(RemoteServiceAdmin rsa) {
synchronized (exportedServices) {
- for (Map<RemoteServiceAdmin, Collection<ExportRegistration>> exports : exportedServices
- .values()) {
- if (exports.containsKey(rsa)) {
- Collection<ExportRegistration> endpoints = exports.get(rsa);
+ for (Map<RemoteServiceAdmin, Collection<ExportRegistration>> exports : exportedServices.values()) {
+ Collection<ExportRegistration> endpoints = exports.get(rsa);
+ if (endpoints != null) {
this.epListenerNotifier.notifyAllListenersOfRemoval(endpoints);
exports.remove(rsa);
}
@@ -139,19 +136,19 @@
}
protected void triggerExportForRemoteServiceAdmin(RemoteServiceAdmin rsa) {
- LOG.debug("triggerExportImportForRemoteSericeAdmin()");
+ LOG.debug("triggerExportForRemoteServiceAdmin()");
synchronized (exportedServices) {
- for (ServiceReference serviceRef : exportedServices.keySet()) {
- Map<RemoteServiceAdmin, Collection<ExportRegistration>> rsaExports = exportedServices.get(serviceRef);
- String bundleName = serviceRef.getBundle().getSymbolicName();
- if (rsaExports.containsKey(rsa)) {
+ for (Map.Entry<ServiceReference, Map<RemoteServiceAdmin, Collection<ExportRegistration>>> entry :
+ exportedServices.entrySet()) {
+ String bundleName = entry.getKey().getBundle().getSymbolicName();
+ if (entry.getValue().containsKey(rsa)) {
// already handled....
LOG.debug("service from bundle {} is already handled by this RSA", bundleName);
} else {
// trigger export of this service....
LOG.debug("service from bundle {} is to be exported by this RSA", bundleName);
- exportService(serviceRef);
+ exportService(entry.getKey(), rsa);
}
}
}
@@ -178,15 +175,17 @@
void removeService(ServiceReference sref) {
synchronized (exportedServices) {
- if (exportedServices.containsKey(sref)) {
- Map<RemoteServiceAdmin, Collection<ExportRegistration>> rsas = exportedServices.get(sref);
- for (Map.Entry<RemoteServiceAdmin, Collection<ExportRegistration>> entry : rsas.entrySet()) {
- if (entry.getValue() != null) {
- Collection<ExportRegistration> registrations = entry.getValue();
- this.epListenerNotifier.notifyListenersOfRemoval(registrations);
- for (ExportRegistration exReg : registrations) {
- if (exReg != null) {
- exReg.close();
+ Map<RemoteServiceAdmin, Collection<ExportRegistration>> rsas = exportedServices.get(sref);
+ if (rsas != null) {
+ synchronized (rsas) {
+ for (Map.Entry<RemoteServiceAdmin, Collection<ExportRegistration>> entry : rsas.entrySet()) {
+ if (entry.getValue() != null) {
+ Collection<ExportRegistration> registrations = entry.getValue();
+ this.epListenerNotifier.notifyListenersOfRemoval(registrations);
+ for (ExportRegistration exReg : registrations) {
+ if (exReg != null) {
+ exReg.close();
+ }
}
}
}
@@ -196,48 +195,48 @@
}
}
}
-
- protected void exportService(ServiceReference sref) {
+
+ protected void exportService(ServiceReference sref, RemoteServiceAdmin rsa) {
// add to local list of services that should/are be exported
synchronized (exportedServices) {
LOG.info("TopologyManager: adding service to exportedServices list to export it --- from bundle: "
+ sref.getBundle().getSymbolicName());
- exportedServices.put(sref,
- new LinkedHashMap<RemoteServiceAdmin, Collection<ExportRegistration>>());
+ if (!exportedServices.containsKey(sref))
+ exportedServices.put(sref, Collections.synchronizedMap(
+ new LinkedHashMap<RemoteServiceAdmin, Collection<ExportRegistration>>()));
}
- triggerExport(sref);
+ triggerExport(sref, rsa);
}
- private void triggerExport(final ServiceReference sref) {
+ private void triggerExport(final ServiceReference sref, final RemoteServiceAdmin rsa) {
execService.execute(new Runnable() {
public void run() {
- doExportService(sref);
+ doExportService(sref, rsa);
}
});
}
- private void doExportService(final ServiceReference sref) {
+ private void doExportService(final ServiceReference sref, RemoteServiceAdmin rsa) {
LOG.debug("Exporting service");
- Map<RemoteServiceAdmin, Collection<ExportRegistration>> exports = null;
+ List<RemoteServiceAdmin> rsrs = rsa != null ? Arrays.asList(rsa) : remoteServiceAdminTracker.getList();
- synchronized (exportedServices) {
- exports = Collections.synchronizedMap(exportedServices.get(sref));
- }
- // FIXME: Not thread safe...?
- if (exports == null) {
- return;
- }
- if (remoteServiceAdminTracker == null || remoteServiceAdminTracker.size() == 0) {
+ if (rsrs.isEmpty()) {
LOG.error(
"No RemoteServiceAdmin available! Unable to export service from bundle {}, interfaces: {}",
sref.getBundle().getSymbolicName(),
sref.getProperty(org.osgi.framework.Constants.OBJECTCLASS));
}
+ Map<RemoteServiceAdmin, Collection<ExportRegistration>> exports;
+ synchronized (exportedServices) {
+ exports = exportedServices.get(sref);
+ }
+ if (exports == null) {
+ return;
+ }
- for (final RemoteServiceAdmin remoteServiceAdmin : remoteServiceAdminTracker
- .getList()) {
+ for (final RemoteServiceAdmin remoteServiceAdmin : rsrs) {
LOG.info("TopologyManager: handling remoteServiceAdmin "
+ remoteServiceAdmin);
@@ -247,14 +246,13 @@
} else {
// TODO: additional parameter Map ?
LOG.debug("exporting ...");
- Collection<ExportRegistration> endpoints = remoteServiceAdmin
- .exportService(sref, null);
+ Collection<ExportRegistration> endpoints = remoteServiceAdmin.exportService(sref, null);
if (endpoints == null) {
// TODO export failed -> What should be done here?
LOG.error("export failed");
exports.put(remoteServiceAdmin, null);
} else {
- LOG.info("TopologyManager: export sucessful Endpoints: {}", endpoints);
+ LOG.info("TopologyManager: export successful Endpoints: {}", endpoints);
// enqueue in local list of endpoints
exports.put(remoteServiceAdmin, endpoints);
@@ -263,14 +261,16 @@
}
}
}
-
+
public Collection<ExportRegistration> getAllExportRegistrations() {
List<ExportRegistration> registrations = new ArrayList<ExportRegistration>();
synchronized (exportedServices) {
for (Map<RemoteServiceAdmin, Collection<ExportRegistration>> exports : exportedServices.values()) {
- for (Collection<ExportRegistration> regs : exports.values()) {
- if (regs != null) {
- registrations.addAll(regs);
+ synchronized (exports) {
+ for (Collection<ExportRegistration> regs : exports.values()) {
+ if (regs != null) {
+ registrations.addAll(regs);
+ }
}
}
}
@@ -280,46 +280,12 @@
private void exportExistingServices() throws InvalidSyntaxException {
String filter = "(" + RemoteConstants.SERVICE_EXPORTED_INTERFACES + "=*)";
- ServiceReference[] references = bctx.getServiceReferences(null, filter);
+ ServiceReference[] references = bctx.getServiceReferences((String)null, filter);
if (references != null) {
for (ServiceReference sref : references) {
- exportService(sref);
+ exportService(sref, null);
}
}
}
-
- public void removeExportRegistration(ExportRegistration exportRegistration) {
- ServiceReference sref = exportRegistration.getExportReference().getExportedService();
- if (sref != null) {
- synchronized (exportedServices) {
-
- Map<RemoteServiceAdmin, Collection<ExportRegistration>> ex = exportedServices.get(sref);
- if (ex != null) {
- for (Map.Entry<RemoteServiceAdmin, Collection<ExportRegistration>> export : ex.entrySet()) {
- export.getValue().contains(exportRegistration);
- }
- }
- }
- } else {
- // the manager will be notified by its own service listener about this case
- }
- }
-
- /**
- * This method is called once a RemoteServiceAdminEvent for an removed export reference is received.
- * However the current implementation has no special support for multiple topology managers, therefore this method
- * does nothing for the moment.
- */
- public void removeExportReference(ExportReference anyObject) {
- // TODO Auto-generated method stub
- // LOG.severe("NOT implemented !!!");
- }
-
- public void remoteAdminEvent(RemoteServiceAdminEvent event) {
- if (event.getType() == RemoteServiceAdminEvent.EXPORT_UNREGISTRATION) {
- removeExportReference(event.getExportReference());
- }
- }
-
}
