Bug fix: when a Service uses a publisher attribute in order to take control of when the service is registeted into the OSGi registry, 
and if the publisher's runnable is invoked BEFORE the service is started, then the service publication must be delayed until the 
service is fully activated (that is until the service's start method is invoked).

Bug fix: extra service properties returned from the @Start annotation callback were not taken into account when a publisher attribute was used. 

Also Added a new ServiceTestWthEarlyPublisher testcase.


git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@984574 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/dependencymanager/runtime/src/main/java/org/apache/felix/dm/runtime/ServicePublisher.java b/dependencymanager/runtime/src/main/java/org/apache/felix/dm/runtime/ServicePublisher.java
index e177dd0..2ae47c2 100644
--- a/dependencymanager/runtime/src/main/java/org/apache/felix/dm/runtime/ServicePublisher.java
+++ b/dependencymanager/runtime/src/main/java/org/apache/felix/dm/runtime/ServicePublisher.java
@@ -68,6 +68,13 @@
     {
         Log.instance().log(LogService.LOG_DEBUG, "registering Publisher for services %s", 
                            Arrays.toString(m_services));
+        
+        // First, store the service properties in the service itself. We do this because when
+        // our lifecycle handler will invoke the service's start callback, it will eventually
+        // append the eventual properties returned by the start() method into our current service
+        // properties. 
+        m_srv.setServiceProperties(m_properties);
+        
         Publisher publisher = new Publisher();
         m_srv.add(dm.createServiceDependency()
                   .setService(Runnable.class, "(dm.publisher=" + System.identityHashCode(this) + ")")
@@ -89,29 +96,24 @@
 
     private class Publisher implements Runnable, ServiceStateListener
     {
+        private boolean m_started; // true if the service has started
+        
         public void run()
         {
             if (m_published.compareAndSet(false, true))
             {
-                try
-                {
-                    Log.instance().log(LogService.LOG_DEBUG, "publishing services %s",
-                                       Arrays.toString(m_services));
+                // Only register the service if it has been started. Otherwise delay the registration
+                // until the service start callback has been invoked.
+                synchronized (this) {
+                    if (! m_started)
+                    {
+                        Log.instance().log(LogService.LOG_DEBUG, "Delaying service publication for services %s (service not yet started)",
+                                           Arrays.toString(m_services));
 
-                    m_registration = m_bc.registerService(m_services, m_srv.getService(), m_properties);
-                }
-                catch (Throwable t)
-                {
-                    m_published.set(false);
-                    if (t instanceof RuntimeException)
-                    {
-                        throw (RuntimeException) t;
-                    }
-                    else
-                    {
-                        throw new RuntimeException("Could not register services", t);
+                        return;
                     }
                 }
+                publish();
             }
         }
 
@@ -121,12 +123,25 @@
 
         public void started(Service service)
         {
-            // TODO Auto-generated method stub
-
+            synchronized (this)
+            {
+                m_started = true;
+            }
+            if (m_published.get())
+            {
+                // Our runnable has been invoked before the service start callback has been called: 
+                // Now that we are started, we fire the service registration.
+                publish();
+            }
         }
 
         public void stopping(Service service)
         {
+            synchronized (this)
+            {
+                m_started = false;
+            }
+
             if (m_published.compareAndSet(true, false))
             {
                 if (m_registration != null)
@@ -143,6 +158,28 @@
         public void stopped(Service service)
         {
         }
+        
+        private void publish()
+        {
+            try
+            {
+                Log.instance().log(LogService.LOG_DEBUG, "publishing services %s",
+                                   Arrays.toString(m_services));
+                m_registration = m_bc.registerService(m_services, m_srv.getService(), m_srv.getServiceProperties());
+            }
+            catch (Throwable t)
+            {
+                m_published.set(false);
+                if (t instanceof RuntimeException)
+                {
+                    throw (RuntimeException) t;
+                }
+                else
+                {
+                    throw new RuntimeException("Could not register services", t);
+                }
+            }
+        }
     }
 
     private class Unpublisher implements Runnable
@@ -155,7 +192,6 @@
                 {
                     Log.instance().log(LogService.LOG_DEBUG, "unpublishing services %s",
                                        Arrays.toString(m_services));
-
                     m_registration.unregister();
                     m_registration = null;
                 }
diff --git a/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/FactoryServiceTestWthPublisher.java b/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/FactoryServiceTestWthPublisher.java
index fd82fa4..dc9ca46 100644
--- a/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/FactoryServiceTestWthPublisher.java
+++ b/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/FactoryServiceTestWthPublisher.java
@@ -19,17 +19,19 @@
 package org.apache.felix.dm.test.bundle.annotation.publisher;
 
 import java.util.Dictionary;
+import java.util.HashMap;
 import java.util.Hashtable;
+import java.util.Map;
 import java.util.Set;
 
-import org.apache.felix.dm.annotation.api.Destroy;
+import org.apache.felix.dm.annotation.api.Property;
 import org.apache.felix.dm.annotation.api.Service;
 import org.apache.felix.dm.annotation.api.ServiceDependency;
 import org.apache.felix.dm.annotation.api.Start;
 import org.apache.felix.dm.test.bundle.annotation.sequencer.Sequencer;
 
 /**
- * This test validate that a basic "ProviderImpl" which is instantiated from a FactorySet can register/unregister its
+ * This test validates that a basic "ProviderImpl" which is instantiated from a FactorySet can register/unregister its
  * service using the Publisher annotation.
  */
 public class FactoryServiceTestWthPublisher
@@ -41,24 +43,30 @@
         Sequencer m_sequencer;
         
         @ServiceDependency(required=false, removed = "unbind")
-        void bind(Provider provider)
+        void bind(Map properties, Provider provider)
         {
             m_sequencer.step(1);
+            if ("bar".equals(properties.get("foo"))) 
+            {
+                m_sequencer.step(2);
+            }
+            if ("bar2".equals(properties.get("foo2"))) 
+            {
+                m_sequencer.step(3);
+            }
+            if ("bar3".equals(properties.get("foo3"))) 
+            {
+                m_sequencer.step(4);
+            }
         }
 
         void unbind(Provider provider)
         {
-            m_sequencer.step(2);
-        }
-        
-        @Destroy
-        void destroy() 
-        {
-            m_sequencer.step(3);
+            m_sequencer.step(5);
         }
     }
    
-    @Service(publisher="m_publisher", unpublisher="m_unpublisher", factorySet="MyFactory")
+    @Service(publisher="m_publisher", unpublisher="m_unpublisher", factorySet="MyFactory", properties={@Property(name="foo", value="bar")})
     public static class ProviderImpl implements Provider
     {
         Runnable m_publisher; // injected and used to register our service
@@ -68,12 +76,15 @@
         Sequencer m_sequencer;
         
         @Start
-        void start()
+        Map start()
         {
             // register service in 1 second
             schedule(m_publisher, 1000);
             // unregister the service in 2 seconds
             schedule(m_unpublisher, 2000);
+            // At this point, our service properties are the one specified in our @Service annotation + the one specified by our Factory.
+            // We also append an extra service property here:
+            return new HashMap() {{ put("foo3", "bar3"); }};
         }
 
         private void schedule(final Runnable task, final long n)
@@ -103,7 +114,7 @@
         @ServiceDependency(filter="(dm.factory.name=MyFactory)")
         void bind(Set<Dictionary> m_providerImplFactory)
         {
-            m_providerImplFactory.add(new Hashtable() {{ put("foo", "bar"); }});
+            m_providerImplFactory.add(new Hashtable() {{ put("foo2", "bar2"); }});
         }
     }
 }
diff --git a/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthEarlyPublisher.java b/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthEarlyPublisher.java
new file mode 100644
index 0000000..edae9e2
--- /dev/null
+++ b/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthEarlyPublisher.java
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.dm.test.bundle.annotation.publisher;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.felix.dm.annotation.api.Init;
+import org.apache.felix.dm.annotation.api.Property;
+import org.apache.felix.dm.annotation.api.Service;
+import org.apache.felix.dm.annotation.api.ServiceDependency;
+import org.apache.felix.dm.annotation.api.Start;
+import org.apache.felix.dm.test.bundle.annotation.sequencer.Sequencer;
+
+/**
+ * This test validates that when a provider publishes its service early (before the
+ * start callback method is invoked), then the runtime bundle must delay the service
+ * registration until the service is fully started.
+ */
+public class ServiceTestWthEarlyPublisher
+{
+    @Service
+    public static class Consumer
+    {
+        @ServiceDependency(filter="(test=testEarlyService)")
+        Sequencer m_sequencer;
+        
+        @ServiceDependency(required=false, removed = "unbind")
+        void bind(Provider provider)
+        {
+            m_sequencer.step(3);
+        }
+
+        void unbind(Provider provider)
+        {
+            m_sequencer.step(5);
+        }
+    }
+    
+    @Service(publisher="m_publisher", unpublisher="m_unpublisher")
+    public static class ProviderImpl implements Provider
+    {
+        Runnable m_publisher; // injected and used to register our service
+        Runnable m_unpublisher; // injected and used to unregister our service
+        
+        @ServiceDependency(filter="(test=testEarlyService)")
+        Sequencer m_sequencer;
+
+        @Init
+        void init()
+        {
+            // invoking the publisher before the start() method has been called 
+            // does not make sense, but this testcase just ensure that the
+            // runtime will defer the service registration until the start 
+            // method is invoked.
+            m_sequencer.step(1);
+            m_publisher.run(); 
+        }
+        
+        @Start
+        void start()
+        {
+            m_sequencer.step(2);
+            // unregister the service in 2 seconds
+            schedule(m_unpublisher, 2000, 4);
+        }
+
+        private void schedule(final Runnable task, final long n, final int step)
+        {
+            Thread t = new Thread() {
+                public void run()
+                {
+                    try
+                    {
+                        sleep(n);
+                    }
+                    catch (InterruptedException e)
+                    {
+                        // TODO Auto-generated catch block
+                        e.printStackTrace();
+                    }
+                    m_sequencer.step(step);
+                    task.run();
+                }
+            };
+            t.start();
+        }
+    }
+}
diff --git a/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthPublisher.java b/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthPublisher.java
index 7ed0731..965960e 100644
--- a/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthPublisher.java
+++ b/dependencymanager/test/src/main/java/org/apache/felix/dm/test/bundle/annotation/publisher/ServiceTestWthPublisher.java
@@ -18,14 +18,18 @@
  */
 package org.apache.felix.dm.test.bundle.annotation.publisher;
 
+import java.util.HashMap;
+import java.util.Map;
+
 import org.apache.felix.dm.annotation.api.Destroy;
+import org.apache.felix.dm.annotation.api.Property;
 import org.apache.felix.dm.annotation.api.Service;
 import org.apache.felix.dm.annotation.api.ServiceDependency;
 import org.apache.felix.dm.annotation.api.Start;
 import org.apache.felix.dm.test.bundle.annotation.sequencer.Sequencer;
 
 /**
- * This test validate that a basic "ProviderImpl" services can register/unregister its
+ * This test validates that a basic "ProviderImpl" services can register/unregister its
  * service using the Publisher annotation.
  */
 public class ServiceTestWthPublisher
@@ -37,24 +41,26 @@
         Sequencer m_sequencer;
         
         @ServiceDependency(required=false, removed = "unbind")
-        void bind(Provider provider)
+        void bind(Map properties, Provider provider)
         {
             m_sequencer.step(1);
+            if ("bar".equals(properties.get("foo")))
+            {
+                m_sequencer.step(2);
+            }
+            if ("bar2".equals(properties.get("foo2")))
+            {
+                m_sequencer.step(3);
+            }
         }
 
         void unbind(Provider provider)
         {
-            m_sequencer.step(2);
-        }
-        
-        @Destroy
-        void destroy() 
-        {
-            m_sequencer.step(3);
+            m_sequencer.step(4);
         }
     }
     
-    @Service(publisher="m_publisher", unpublisher="m_unpublisher")
+    @Service(publisher="m_publisher", unpublisher="m_unpublisher", properties={@Property(name="foo", value="bar")})
     public static class ProviderImpl implements Provider
     {
         Runnable m_publisher; // injected and used to register our service
@@ -64,12 +70,16 @@
         Sequencer m_sequencer;
 
         @Start
-        void start()
+        Map start()
         {
             // register service in 1 second
             schedule(m_publisher, 1000);
             // unregister the service in 2 seconds
             schedule(m_unpublisher, 2000);
+            
+            // Add some extra service properties ... they will be appended to the one we have defined
+            // in the @Service annotation.
+            return new HashMap() {{ put("foo2", "bar2"); }};
         }
 
         private void schedule(final Runnable task, final long n)
diff --git a/dependencymanager/test/src/test/java/org/apache/felix/dm/test/annotation/PublisherAnnotationTest.java b/dependencymanager/test/src/test/java/org/apache/felix/dm/test/annotation/PublisherAnnotationTest.java
index f1d5c7c..39f59b3 100644
--- a/dependencymanager/test/src/test/java/org/apache/felix/dm/test/annotation/PublisherAnnotationTest.java
+++ b/dependencymanager/test/src/test/java/org/apache/felix/dm/test/annotation/PublisherAnnotationTest.java
@@ -72,12 +72,7 @@
         // Provide the Sequencer service to the "Component" service.
         m.add(m.createService().setImplementation(this).setInterface(Sequencer.class.getName(), 
                                                                      new Hashtable() {{ put("test", "testService"); }}));
-        // Check if the Provider has seen the Provider.
-        m_ensure.waitForStep(2, 10000);
-        // Stop the bundle
-        stopBundle("PublisherAnnotationsTest", context);
-        // And check if the Consumer has been destroyed.
-        m_ensure.waitForStep(3, 10000);
+        m_ensure.waitForStep(4, 10000);
     }
     
     /**
@@ -90,11 +85,21 @@
         // Provide the Sequencer service to the "Component" service.
         m.add(m.createService().setImplementation(this).setInterface(Sequencer.class.getName(), 
                                                                      new Hashtable() {{ put("test", "testFactoryService"); }}));
-        // Check if the Provider has seen the Provider.
-        m_ensure.waitForStep(2, 10000);
-        // Stop the bundle
-        stopBundle("PublisherAnnotationsTest", context);
-        // And check if the Consumer has been destroyed.
-        m_ensure.waitForStep(3, 10000);
+        m_ensure.waitForStep(5, 10000);
+    }
+    
+    /**
+     * A Provider that registers its service, using the Publisher annotation, but very early: before
+     * the start callback is invoked ! In this case, we must verify that the runtime will delay the
+     * service registration until the service is fully started.
+     */
+    @Test
+    public void testServiceWithEarlyPublisher(BundleContext context)
+    {
+        DependencyManager m = new DependencyManager(context);
+        // Provide the Sequencer service to the "Component" service.
+        m.add(m.createService().setImplementation(this).setInterface(Sequencer.class.getName(), 
+                                                                     new Hashtable() {{ put("test", "testEarlyService"); }}));
+        m_ensure.waitForStep(5, 10000);
     }
 }