Add test for min timeout for change count update

Also fix logging for when timeout is less than 1

Author: tjwatson

scr/src/main/java/org/apache/felix/scr/impl/ComponentRegistry.java

@@ -138,7 +138,7 @@ public class ComponentRegistry
     public ComponentRegistry(final ScrConfiguration scrConfiguration, final ScrLogger logger, final ScheduledExecutorService componentActor )
     {
         m_configuration = scrConfiguration;
-        m_updateChangeCountPropertyTask = new UpdateChangeCountProperty(m_configuration.serviceChangecountTimeout());
+        m_updateChangeCountPropertyTask = new UpdateChangeCountProperty(m_configuration.serviceChangecountTimeout(), logger, componentActor);
         m_logger = logger;
         m_componentActor = componentActor;
         m_componentHoldersByName = new HashMap<>();
@@ -708,38 +708,43 @@ public void unregisterRegionConfigurationSupport(
 
 	}
 
-    private final AtomicLong changeCount = new AtomicLong();
-
-    public Dictionary<String, Object> getServiceRegistrationProperties()
+	Dictionary<String, Object> getServiceRegistrationProperties()
     {
-        final Dictionary<String, Object> props = new Hashtable<>();
-        props.put(PROP_CHANGECOUNT, this.changeCount.get());
-
-        return props;
+        return m_updateChangeCountPropertyTask.getServiceRegistrationProperties();
     }
 
-    public void setRegistration(final ServiceRegistration<ServiceComponentRuntime> reg)
+	public void setRegistration(final ServiceRegistration<ServiceComponentRuntime> reg)
     {
         m_updateChangeCountPropertyTask.setRegistration(reg);
         m_updateChangeCountPropertyTask.schedule();
     }
 
-    class UpdateChangeCountProperty implements Runnable {
-        // TODO Is 100 ms an appropriate minimum?  
-        private static final long MIN_ALLOWED_DELAY = 100;
+    public void updateChangeCount()
+    {
+        m_updateChangeCountPropertyTask.updateChangeCount();
+    }
+
+    static class UpdateChangeCountProperty implements Runnable {
+        // TODO 1 seems really low?  
+        private static final long MIN_ALLOWED_DELAY = 1;
+        private final AtomicLong changeCount = new AtomicLong();
         private volatile ServiceRegistration<ServiceComponentRuntime> registration;
         private final long maxNumberOfNoChanges;
         private final long delay;
+        private final ScrLogger logger;
+        private final ScheduledExecutorService executor;
 
         // guarded by this
         private int noChangesCount = 0;
         // guarded by this
         private ScheduledFuture<?> scheduledFuture = null;
 
-        public UpdateChangeCountProperty(long delay)
+        UpdateChangeCountProperty(long delay, ScrLogger logger, ScheduledExecutorService executor)
         {
+            this.logger = logger;
+            this.executor = executor;
             if (delay < MIN_ALLOWED_DELAY) {
-                m_logger.log(Level.INFO,
+                logger.log(Level.INFO,
                         "The service change count timeout {0} is less than the allowable minimum {1}.  Using the allowable minimum instead.", null,
                         delay, MIN_ALLOWED_DELAY);
                 delay = MIN_ALLOWED_DELAY;
@@ -750,19 +755,31 @@ public UpdateChangeCountProperty(long delay)
             maxNumberOfNoChanges = Long.max(10000 / delay, 1);
         }
 
-        public void setRegistration(ServiceRegistration<ServiceComponentRuntime> reg)
+        void setRegistration(ServiceRegistration<ServiceComponentRuntime> reg)
         {
             this.registration = reg;
         }
 
-        public synchronized void schedule()
+        Dictionary<String, Object> getServiceRegistrationProperties()
+        {
+            final Dictionary<String, Object> props = new Hashtable<>();
+            props.put(PROP_CHANGECOUNT, this.changeCount.get());
+
+            return props;
+        }
+
+        public void updateChangeCount() {
+            this.changeCount.incrementAndGet();
+            schedule();
+        }
+        synchronized void schedule()
         {
             // reset noChangesCount to ensure task runs at least once more if it exists
             noChangesCount = 0;
             if (scheduledFuture != null) {
                 return;
             }
-            scheduledFuture = m_componentActor.scheduleWithFixedDelay(this , delay, delay, TimeUnit.MILLISECONDS);
+            scheduledFuture = executor.scheduleWithFixedDelay(this , delay, delay, TimeUnit.MILLISECONDS);
         }
 
         @Override
@@ -802,16 +819,10 @@ public void run()
                     }
                 }
             } catch (Exception e) {
-                m_logger.log(Level.WARN,
+                logger.log(Level.WARN,
                     "Service changecount update for {0} had a problem", e,
                     registration.getReference());
             }
         }
     }
-
-    public void updateChangeCount()
-    {
-        this.changeCount.incrementAndGet();
-        m_updateChangeCountPropertyTask.schedule();
-    }
 }

scr/src/test/java/org/apache/felix/scr/integration/GithubPR486Test.java

@@ -0,0 +1,84 @@
+/*
+ * 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.scr.integration;
+
+import static org.junit.Assert.assertEquals;
+import static org.ops4j.pax.exam.CoreOptions.systemProperty;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.ops4j.pax.exam.Configuration;
+import org.ops4j.pax.exam.Option;
+import org.ops4j.pax.exam.OptionUtils;
+import org.ops4j.pax.exam.junit.PaxExam;
+import org.osgi.framework.BundleException;
+import org.osgi.framework.Constants;
+import org.osgi.framework.InvalidSyntaxException;
+import org.osgi.framework.ServiceReference;
+import org.osgi.service.component.runtime.ServiceComponentRuntime;
+
+@RunWith(PaxExam.class)
+public class GithubPR486Test extends ComponentTestBase
+{
+
+    // test min timeout checking
+    private static final long DS_SERVICE_CHANGECOUNT_TIMEOUT = 0;
+
+    static
+    {
+        descriptorFile = "/integration_test_simple_components.xml";
+        // uncomment to enable debugging of this test class
+        //paxRunnerVmOption = DEBUG_VM_OPTION;
+    }
+
+    @Configuration
+    public static Option[] configuration()
+    {
+        return OptionUtils.combine(ComponentTestBase.configuration(),
+                systemProperty( "ds.service.changecount.timeout" ).value( Long.toString(DS_SERVICE_CHANGECOUNT_TIMEOUT) ));
+    }
+
+
+    private ServiceReference<ServiceComponentRuntime> scrRef;
+    @Before
+    public void addServiceListener() throws InvalidSyntaxException
+    {
+        scrRef = bundleContext.getServiceReference(ServiceComponentRuntime.class);
+    }
+
+    @Test
+    public void verify_changecount_updates_with_min_timeout() throws InterruptedException, BundleException
+    {
+        // Wait a second the changecount timeout`to account for the asynchronous service.changecount property update
+        Thread.sleep(1000);
+
+        // Check that the service.changecount update was recorded
+        assertEquals(13L, scrRef.getProperty(Constants.SERVICE_CHANGECOUNT));
+
+        // Trigger a change by stopping the bundle with components
+        bundle.stop();
+
+        // Wait a second the changecount timeout`to account for the asynchronous service.changecount property update
+        Thread.sleep(1000);
+
+        // Check that another service.changecount update was recorded
+        assertEquals(26L, scrRef.getProperty(Constants.SERVICE_CHANGECOUNT));
+    }
+}