aries-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From cschnei...@apache.org
Subject [3/3] aries-rsa git commit: [ARIES-1763] Correctly close exports when service goes away
Date Mon, 05 Feb 2018 09:45:05 GMT
[ARIES-1763] Correctly close exports when service goes away


Project: http://git-wip-us.apache.org/repos/asf/aries-rsa/repo
Commit: http://git-wip-us.apache.org/repos/asf/aries-rsa/commit/55e2c1cb
Tree: http://git-wip-us.apache.org/repos/asf/aries-rsa/tree/55e2c1cb
Diff: http://git-wip-us.apache.org/repos/asf/aries-rsa/diff/55e2c1cb

Branch: refs/heads/master
Commit: 55e2c1cb4767d5155a171e349514c91a7ac466f3
Parents: a5c8d7d
Author: Christian Schneider <cschneid@adobe.com>
Authored: Mon Feb 5 10:44:54 2018 +0100
Committer: Christian Schneider <cschneid@adobe.com>
Committed: Mon Feb 5 10:44:54 2018 +0100

----------------------------------------------------------------------
 .../aries/rsa/core/ExportRegistrationImpl.java  | 18 ++++++-
 .../aries/rsa/core/RemoteServiceAdminCore.java  | 23 +++++++--
 .../core/DistributionProviderTrackerTest.java   | 34 +++++++-----
 .../rsa/core/RemoteServiceAdminCoreTest.java    | 54 ++++++++------------
 .../exporter/TopologyManagerExport.java         | 24 ++++-----
 5 files changed, 91 insertions(+), 62 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/aries-rsa/blob/55e2c1cb/rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java
----------------------------------------------------------------------
diff --git a/rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java b/rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java
index 81bb859..a02ff3a 100644
--- a/rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java
+++ b/rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java
@@ -21,6 +21,7 @@ package org.apache.aries.rsa.core;
 import java.io.Closeable;
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 
@@ -30,6 +31,7 @@ import org.osgi.framework.ServiceReference;
 import org.osgi.service.remoteserviceadmin.EndpointDescription;
 import org.osgi.service.remoteserviceadmin.ExportReference;
 import org.osgi.service.remoteserviceadmin.ExportRegistration;
+import org.osgi.service.remoteserviceadmin.RemoteConstants;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -110,6 +112,7 @@ public class ExportRegistrationImpl implements ExportRegistration {
     }
 
     public final void close() {
+        closeHandler.onClose(this);
         synchronized (this) {
             if (closed) {
                 return;
@@ -117,7 +120,6 @@ public class ExportRegistrationImpl implements ExportRegistration {
             closed = true;
         }
 
-        closeHandler.onClose(this);
         if (exportReference != null) {
             exportReference.close();
         }
@@ -181,9 +183,21 @@ public class ExportRegistrationImpl implements ExportRegistration {
             return null;
         }
         ServiceReference<?> sref = getExportReference().getExportedService();
-        EndpointDescription epd = new EndpointDescription(sref, properties);
+        
+        HashMap<String, Object> props = new HashMap<>(properties);
+        EndpointDescription oldEpd = getExportReference().getExportedEndpoint();
+        copyIfNull(props, oldEpd, RemoteConstants.ENDPOINT_ID);
+        copyIfNull(props, oldEpd, RemoteConstants.SERVICE_IMPORTED_CONFIGS);
+
+        EndpointDescription epd = new EndpointDescription(sref, props);
         exportReference = new ExportReferenceImpl(sref, epd);
         this.sender.notifyUpdate(this.getExportReference());
         return epd;
     }
+
+    private void copyIfNull(HashMap<String, Object> props, EndpointDescription oldEpd,
String key) {
+        if (props.get(key) == null) {
+            props.put(key, oldEpd.getProperties().get(key));
+        }
+    }
 }

http://git-wip-us.apache.org/repos/asf/aries-rsa/blob/55e2c1cb/rsa/src/main/java/org/apache/aries/rsa/core/RemoteServiceAdminCore.java
----------------------------------------------------------------------
diff --git a/rsa/src/main/java/org/apache/aries/rsa/core/RemoteServiceAdminCore.java b/rsa/src/main/java/org/apache/aries/rsa/core/RemoteServiceAdminCore.java
index 98fca14..f8df7c8 100644
--- a/rsa/src/main/java/org/apache/aries/rsa/core/RemoteServiceAdminCore.java
+++ b/rsa/src/main/java/org/apache/aries/rsa/core/RemoteServiceAdminCore.java
@@ -39,6 +39,7 @@ import org.apache.aries.rsa.util.EndpointHelper;
 import org.apache.aries.rsa.util.StringPlus;
 import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceEvent;
 import org.osgi.framework.ServiceListener;
 import org.osgi.framework.ServiceReference;
 import org.osgi.framework.ServiceRegistration;
@@ -84,13 +85,26 @@ public class RemoteServiceAdminCore implements RemoteServiceAdmin {
         this.packageUtil = packageUtil;
         this.closeHandler = new CloseHandler() {
             public void onClose(ExportRegistration exportReg) {
-                removeExportRegistration((ExportRegistrationImpl) exportReg);
+                removeExportRegistration(exportReg);
             }
 
             public void onClose(ImportRegistration importReg) {
                 removeImportRegistration((ImportRegistrationImpl) importReg);
             }
         };
+        createServiceListener();
+    }
+    
+    // listen for exported services being unregistered so we can close the export
+    protected void createServiceListener() {
+        this.exportedServiceListener = new ServiceListener() {
+            public void serviceChanged(ServiceEvent event) {
+                if (event.getType() == ServiceEvent.UNREGISTERING) {
+                    removeServiceExports(event.getServiceReference());
+                }
+            }
+        };
+        this.bctx.addServiceListener(exportedServiceListener);
     }
 
     @Override
@@ -216,6 +230,9 @@ public class RemoteServiceAdminCore implements RemoteServiceAdmin {
                 return null;
             }
             return new ExportRegistrationImpl(serviceReference, endpoint, closeHandler, eventProducer);
+        } catch (IllegalArgumentException e) {
+            // TCK expects this for garbage input
+            throw e;
         } catch (Exception e) {
             return new ExportRegistrationImpl(e, closeHandler, eventProducer);
         }
@@ -476,14 +493,14 @@ public class RemoteServiceAdminCore implements RemoteServiceAdmin {
      *
      * @param eri the export registration to remove
      */
-    protected void removeExportRegistration(ExportRegistrationImpl eri) {
+    protected void removeExportRegistration(ExportRegistration eri) {
         synchronized (exportedServices) {
             for (Iterator<Collection<ExportRegistration>> it = exportedServices.values().iterator();
it.hasNext();) {
                 Collection<ExportRegistration> value = it.next();
                 for (Iterator<ExportRegistration> it2 = value.iterator(); it2.hasNext();)
{
                     ExportRegistration er = it2.next();
                     if (er.equals(eri)) {
-                        eventProducer.notifyRemoval(eri.getExportReferenceAlways());
+                        eventProducer.notifyRemoval(eri.getExportReference());
                         it2.remove();
                         if (value.isEmpty()) {
                             it.remove();

http://git-wip-us.apache.org/repos/asf/aries-rsa/blob/55e2c1cb/rsa/src/test/java/org/apache/aries/rsa/core/DistributionProviderTrackerTest.java
----------------------------------------------------------------------
diff --git a/rsa/src/test/java/org/apache/aries/rsa/core/DistributionProviderTrackerTest.java
b/rsa/src/test/java/org/apache/aries/rsa/core/DistributionProviderTrackerTest.java
index 1468b8b..a18dae5 100644
--- a/rsa/src/test/java/org/apache/aries/rsa/core/DistributionProviderTrackerTest.java
+++ b/rsa/src/test/java/org/apache/aries/rsa/core/DistributionProviderTrackerTest.java
@@ -18,6 +18,11 @@
  */
 package org.apache.aries.rsa.core;
 
+import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.anyString;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.expectLastCall;
+
 import java.util.Dictionary;
 
 import org.apache.aries.rsa.spi.DistributionProvider;
@@ -29,6 +34,7 @@ import org.osgi.framework.Filter;
 import org.osgi.framework.FrameworkUtil;
 import org.osgi.framework.InvalidSyntaxException;
 import org.osgi.framework.ServiceFactory;
+import org.osgi.framework.ServiceListener;
 import org.osgi.framework.ServiceReference;
 import org.osgi.framework.ServiceRegistration;
 import org.osgi.service.remoteserviceadmin.RemoteConstants;
@@ -45,19 +51,21 @@ public class DistributionProviderTrackerTest {
         DistributionProvider provider = c.createMock(DistributionProvider.class);
         
         ServiceReference<DistributionProvider> providerRef = c.createMock(ServiceReference.class);
-        EasyMock.expect(providerRef.getProperty(RemoteConstants.REMOTE_INTENTS_SUPPORTED)).andReturn("");
-        EasyMock.expect(providerRef.getProperty(RemoteConstants.REMOTE_CONFIGS_SUPPORTED)).andReturn("");
+        expect(providerRef.getProperty(RemoteConstants.REMOTE_INTENTS_SUPPORTED)).andReturn("");
+        expect(providerRef.getProperty(RemoteConstants.REMOTE_CONFIGS_SUPPORTED)).andReturn("");
 
         BundleContext context = c.createMock(BundleContext.class);
         String filterSt = String.format("(objectClass=%s)", DistributionProvider.class.getName());
         Filter filter = FrameworkUtil.createFilter(filterSt);
-        EasyMock.expect(context.createFilter(filterSt)).andReturn(filter);
-        EasyMock.expect(context.getService(providerRef)).andReturn(provider);
+        expect(context.createFilter(filterSt)).andReturn(filter);
+        expect(context.getService(providerRef)).andReturn(provider);
         ServiceRegistration rsaReg = c.createMock(ServiceRegistration.class);
-        EasyMock.expect(context.registerService(EasyMock.isA(String.class), EasyMock.isA(ServiceFactory.class),

+        expect(context.registerService(EasyMock.isA(String.class), EasyMock.isA(ServiceFactory.class),

                                                 EasyMock.isA(Dictionary.class)))
             .andReturn(rsaReg).atLeastOnce();
-
+        context.addServiceListener(anyObject(ServiceListener.class));
+        expectLastCall().anyTimes();
+        
         final BundleContext apiContext = c.createMock(BundleContext.class);
         c.replay();
         DistributionProviderTracker tracker = new DistributionProviderTracker(context) {
@@ -83,19 +91,21 @@ public class DistributionProviderTrackerTest {
         DistributionProvider provider = c.createMock(DistributionProvider.class);
 
         ServiceReference<DistributionProvider> providerRef = c.createMock(ServiceReference.class);
-        EasyMock.expect(providerRef.getProperty(RemoteConstants.REMOTE_INTENTS_SUPPORTED)).andReturn(null);
-        EasyMock.expect(providerRef.getProperty(RemoteConstants.REMOTE_CONFIGS_SUPPORTED)).andReturn(null);
+        expect(providerRef.getProperty(RemoteConstants.REMOTE_INTENTS_SUPPORTED)).andReturn(null);
+        expect(providerRef.getProperty(RemoteConstants.REMOTE_CONFIGS_SUPPORTED)).andReturn(null);
 
         BundleContext context = c.createMock(BundleContext.class);
         String filterSt = String.format("(objectClass=%s)", DistributionProvider.class.getName());
         Filter filter = FrameworkUtil.createFilter(filterSt);
-        EasyMock.expect(context.createFilter(filterSt)).andReturn(filter);
-        EasyMock.expect(context.getService(providerRef)).andReturn(provider);
+        expect(context.createFilter(filterSt)).andReturn(filter);
+        expect(context.getService(providerRef)).andReturn(provider);
         ServiceRegistration rsaReg = c.createMock(ServiceRegistration.class);
-        EasyMock.expect(context.registerService(EasyMock.isA(String.class), EasyMock.isA(ServiceFactory.class),
+        expect(context.registerService(EasyMock.isA(String.class), EasyMock.isA(ServiceFactory.class),
                                                 EasyMock.isA(Dictionary.class)))
             .andReturn(rsaReg).atLeastOnce();
-
+        context.addServiceListener(anyObject(ServiceListener.class));
+        expectLastCall().anyTimes();
+        
         final BundleContext apiContext = c.createMock(BundleContext.class);
         c.replay();
         DistributionProviderTracker tracker = new DistributionProviderTracker(context) {

http://git-wip-us.apache.org/repos/asf/aries-rsa/blob/55e2c1cb/rsa/src/test/java/org/apache/aries/rsa/core/RemoteServiceAdminCoreTest.java
----------------------------------------------------------------------
diff --git a/rsa/src/test/java/org/apache/aries/rsa/core/RemoteServiceAdminCoreTest.java b/rsa/src/test/java/org/apache/aries/rsa/core/RemoteServiceAdminCoreTest.java
index 09dffc2..6200d45 100644
--- a/rsa/src/test/java/org/apache/aries/rsa/core/RemoteServiceAdminCoreTest.java
+++ b/rsa/src/test/java/org/apache/aries/rsa/core/RemoteServiceAdminCoreTest.java
@@ -20,7 +20,10 @@ package org.apache.aries.rsa.core;
 
 import static org.easymock.EasyMock.anyObject;
 import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.expectLastCall;
+import static org.hamcrest.Matchers.array;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -28,7 +31,7 @@ import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
-import java.lang.reflect.Field;
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Dictionary;
@@ -50,6 +53,7 @@ import org.osgi.framework.Bundle;
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.Constants;
 import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceListener;
 import org.osgi.framework.ServiceReference;
 import org.osgi.framework.Version;
 import org.osgi.service.remoteserviceadmin.EndpointDescription;
@@ -93,9 +97,7 @@ public class RemoteServiceAdminCoreTest {
             };
         };
         rsaCore = new RemoteServiceAdminCore(rsaContext, apiContext, eventProducer, provider,
packageUtil) {
-            protected void createExportedServicesListener() {
-                // Skip
-            }
+            protected void createServiceListener() {};
         };
     }
 
@@ -224,10 +226,8 @@ public class RemoteServiceAdminCoreTest {
         Map<String, Object> edProps = endpoint.getProperties();
         assertEquals("http://something", edProps.get("endpoint.id"));
         assertNotNull(edProps.get("service.imported"));
-        assertTrue(Arrays.equals(new String[] {"java.lang.Runnable"},
-                                 (Object[]) edProps.get("objectClass")));
-        assertTrue(Arrays.equals(new String[] {MYCONFIG},
-                                 (Object[]) edProps.get("service.imported.configs")));
+        assertThat((String[]) edProps.get("objectClass"), array(equalTo("java.lang.Runnable")));
+        assertThat((String[]) edProps.get("service.imported.configs"), array(equalTo(MYCONFIG)));
 
         // Ask to export the same service again, this should not go through the whole process
again but simply return
         // a copy of the first instance.
@@ -239,22 +239,17 @@ public class RemoteServiceAdminCoreTest {
         assertEquals(ereg.getExportReference().getExportedEndpoint().getProperties(),
                 ereg2.getExportReference().getExportedEndpoint().getProperties());
 
-        // Look at the exportedServices data structure
-        Map<Map<String, Object>, Collection<ExportRegistration>> exportedServices
= getInternalExportedServices();
-
-        assertEquals("One service was exported", 1, exportedServices.size());
-        Collection<ExportRegistration> firstRegs = exportedServices.values().iterator().next();
-        assertEquals("There are 2 export registrations (identical copies)",
-                2, firstRegs.size());
+        assertNumExports(2);
 
-        // Unregister one of the exports
-        rsaCore.removeExportRegistration((ExportRegistrationImpl) eregs.get(0));
-        assertEquals("One service was exported", 1, exportedServices.size());
-        assertEquals("There 1 export registrations left", 1, firstRegs.size());
+        ereg.close();
+        assertNumExports(1);
+        
+        ereg2.close();
+        assertNumExports(0);
+    }
 
-        // Unregister the other export
-        rsaCore.removeExportRegistration((ExportRegistrationImpl) eregs2.get(0));
-        assertEquals("No more exported services", 0, exportedServices.size());
+    private void assertNumExports(int expectedNum) {
+        assertThat("Number of export references", rsaCore.getExportedServices().size(), equalTo(expectedNum));
     }
 
     @Test
@@ -328,22 +323,15 @@ public class RemoteServiceAdminCoreTest {
         c.verify();
     }
 
-    private Map<Map<String, Object>, Collection<ExportRegistration>> getInternalExportedServices()
-            throws NoSuchFieldException, IllegalAccessException {
-        Field field = RemoteServiceAdminCore.class.getDeclaredField("exportedServices");
-        field.setAccessible(true);
-        Map<Map<String, Object>, Collection<ExportRegistration>> exportedServices
=
-                (Map<Map<String, Object>, Collection<ExportRegistration>>)
field.get(rsaCore);
-        return exportedServices;
-    }
-
-    private Endpoint createEndpoint(final Map<String, Object> sProps) {
+    private Endpoint createEndpoint(final Map<String, Object> sProps) throws IOException
{
         Map<String, Object> eProps = new HashMap<String, Object>(sProps);
         eProps.put("endpoint.id", "http://something");
         eProps.put("service.imported.configs", new String[] {MYCONFIG});
         final EndpointDescription epd = new EndpointDescription(eProps);
         Endpoint er = c.createMock(Endpoint.class);
-        expect(er.description()).andReturn(epd);
+        expect(er.description()).andReturn(epd).anyTimes();
+        er.close();
+        expectLastCall();
         return er;
     }
 

http://git-wip-us.apache.org/repos/asf/aries-rsa/blob/55e2c1cb/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/exporter/TopologyManagerExport.java
----------------------------------------------------------------------
diff --git a/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/exporter/TopologyManagerExport.java
b/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/exporter/TopologyManagerExport.java
index 257c924..0d62b2b 100644
--- a/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/exporter/TopologyManagerExport.java
+++ b/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/exporter/TopologyManagerExport.java
@@ -88,11 +88,15 @@ public class TopologyManagerExport implements ServiceListener {
     // track all service registrations so we can export any services that are configured
to be exported
     // ServiceListener events may be delivered out of order, concurrently, re-entrant, etc.
(see spec or docs)
     public void serviceChanged(ServiceEvent event) {
-        LOG.info("Received ServiceEvent type: {}, sref: {}", getTypeName(event), event.getServiceReference());
         ServiceReference<?> sref = event.getServiceReference();
+        if (!shouldExport(sref)) {
+            LOG.debug("Skipping service {}", sref);
+            return;
+        }
+        LOG.info("Received ServiceEvent type: {}, sref: {}", getTypeName(event), sref);
         switch (event.getType()) {
         case ServiceEvent.REGISTERED:
-            exportInBackground(sref);
+            doExport(sref);
             break;
 
         case ServiceEvent.MODIFIED:
@@ -151,11 +155,6 @@ public class TopologyManagerExport implements ServiceListener {
     }
 
     private void doExport(final ServiceReference<?> sref) {
-        Map<String, ?> addProps = policy.additionalParameters(sref);
-        if (!shouldExport(sref, addProps)) {
-            LOG.debug("Skipping service {}", sref);
-            return;
-        }
         LOG.debug("Exporting service {}", sref);
         toBeExported.add(sref);
         if (endpointRepo.size() == 0) {
@@ -167,15 +166,14 @@ public class TopologyManagerExport implements ServiceListener {
 
         for (RemoteServiceAdmin remoteServiceAdmin : endpointRepo.keySet()) {
             ServiceExportsRepository repo = endpointRepo.get(remoteServiceAdmin);
-            Collection<ExportRegistration> regs = exportService(remoteServiceAdmin,
sref, addProps);
+            Collection<ExportRegistration> regs = exportService(remoteServiceAdmin,
sref);
             repo.addService(sref, regs);
         }
     }
     
-    private static Collection<ExportRegistration> exportService(
+    private Collection<ExportRegistration> exportService(
             final RemoteServiceAdmin rsa,
-            final ServiceReference<?> sref, 
-            final Map<String, ?> addProps) {
+            final ServiceReference<?> sref) {
         // abort if the service was unregistered by the time we got here
         // (we check again at the end, but this optimization saves unnecessary heavy processing)
         if (sref.getBundle() == null) {
@@ -184,6 +182,7 @@ public class TopologyManagerExport implements ServiceListener {
         }
 
         LOG.debug("exporting Service {} using RemoteServiceAdmin {}", sref, rsa.getClass().getName());
+        Map<String, ?> addProps = policy.additionalParameters(sref);
         Collection<ExportRegistration> exportRegs = rsa.exportService(sref, addProps);
 
         // process successful/failed registrations
@@ -209,7 +208,8 @@ public class TopologyManagerExport implements ServiceListener {
         return exportRegs;
     }
 
-    private boolean shouldExport(ServiceReference<?> sref, Map<String, ?> addProps)
{
+    private boolean shouldExport(ServiceReference<?> sref) {
+        Map<String, ?> addProps = policy.additionalParameters(sref);
         List<String> exported= StringPlus.normalize(sref.getProperty(RemoteConstants.SERVICE_EXPORTED_INTERFACES));
         List<String> addExported = StringPlus.normalize(addProps.get(RemoteConstants.SERVICE_EXPORTED_INTERFACES));
         return sizeOf(exported) + sizeOf(addExported) > 0;


Mime
View raw message