[ https://issues.apache.org/jira/browse/FELIX-5435?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15735933#comment-15735933 ]
David Jencks commented on FELIX-5435: ------------------------------------- My initial impression from your analysis is that this is a configuration admin bug rather than a scr bug? Isn't the problem that ca is returning a change count of 1 rather than 2? > [DS] Service does not get loaded with updated properties that have been > modified on file system after felix framework restart > ----------------------------------------------------------------------------------------------------------------------------- > > Key: FELIX-5435 > URL: https://issues.apache.org/jira/browse/FELIX-5435 > Project: Felix > Issue Type: Bug > Components: Declarative Services (SCR) > Affects Versions: scr-2.0.4, scr-2.0.6 > Environment: Java 1.7; Using Felix framework version 5.4.0. > Reporter: Alin Brici > Priority: Blocker > > In felix.scr 2.0.6, seeing an issue where a configuration for a particular > Service is not loaded. > We have one @Component(Componenent A) that offers one service, lets call this > Service A. > ComponenetA attributes are as follows: {noformat}(name="ServiceA", policy= > ConfigurationPolicy.REQUIRE, metatype=true, immediate=true) {noformat} > ServiceA has a configuration that could look like this: > {noformat} > { > "name" : "ServiceA", > "description" : "Displays which version it has of ServiceB", > "versionRequired" : "1" > } > {noformat} > We have a second @Component(Component B) that offers another service, lets > call that Service B. > The Component attributes for Component B are as follows: > {noformat}(name:"ServiceB", policy=ConfigurationPolicy.OPTIONAL, > metatype=true, immediate=true).{noformat} > (I understand with immediate=true and ConfiguraitonPolicy.OPTIONAL, Service B > will be initialized without any configuration when it first gets started by > the framework, however if there is configuration for it on the file system, > Felix DS will reload Service B with what is on file system when it scans the > directory for ServiceB configuraiton). > Service B, has some properties in the configuraiton for it, that are used by > ServiceA if ServiceB has been loaded with configuration. For example say we > have a configuration for ServiceB that looks like this. > {noformat} > { > "name" : "ServiceB", > "description" : "is used by ServiceA" > "version" : "1" > } > {noformat} > Service A has a reference to Service B, as so: > {noformat} > @Reference(policy = ReferencePolicy.DYNAMIC) > volatile ServiceB serviceB = null; > For example if we have something like this for ComponentA/ServiceA, I will > keep the code short to illustrate the problem. > @Component(name = "ServiceA", > policy = ConfigurationPolicy.REQUIRE, > metatype = true, > description = "ComponentA with ServiceA", > immediate = true) > @Service(value = {ServiceAInterface.class}) > public class ServiceA implements ServiceAInterface { > private String version; > private String description; > private Json config = null; > // version requested by VersionB > private String versionRequested; > @Reference(policy = ReferencePolicy.DYNAMIC) > volatile ServiceB serviceB; > @Activate > protected void activate(ComponentContext context) { > this.config = getConfigFromComponentContext(context); > this.description = config.get("description"); > this.versionRequested = config.get("versionRequested"); > // get the version that ServiceA config requests > > serviceB.getVersionAsStringAsync(versionRequested).thenOnResult( > this.version = versionFromServiceB; > ); > } > // @Deactivate method would go here > @override > public String getVersion() { > if (version != null) { > return version; > } else { > return "No Version has been set yet!"; > } > } > } > {noformat} > For the scenario where this problem exists we need a configuration for both > ServiceA and ServiceB. The configuration is stored on the file system in some > directory that felix watches to load changes as they occur. /tmp/felixConfig/ > ServiceA.json ServiceB.json. > Start up the Felix OSGi environment with the configuration and what I see is > that it works as expected. ServiceA waits until it has its @Refernce to > ServiceB satisfied before it attempts to activate as expected. Then if I made > a call to ServiceA#getVersion("1"), and ServiceB is up and running with > configuration from the file that was found, ServiceA will be able to get the > version. > Here is where the problem is. I shutdown the OSGi Framework. Then I change > the configuration to both ServiceA and ServiceB, "versionRequired" : "2" and > "version" : "2", respectively. I then start the OSGi framework back up and > now ServiceA does not load correctly in the ComponentRegistry as > "versionRequired" : "2", but rather has the old version, "1". I see that > felix.scr picked up the changes and is attempting to notify all the > ConfigurationListener(org.osgi.service.cm.ConfigurationListener) > implementations. > The setup I am working with picks up a change and makes a call to > org.apache.felix.cm.impl.ConfigurationAdapter#update, > {noformat} > @Override > public void update( Dictionary<String, ?> properties ) throws IOException > { > delegatee.getConfigurationManager().log( LogService.LOG_DEBUG, > "update(properties={0})", new Object[] > { properties } ); > checkActive(); > checkDeleted(); > delegatee.update( properties ); > } > {noformat} > {noformat} > ** An Important NOTE ** > I set a breakpoint at `delegatee.update(properties)`. When I look here at the > `configurations` inside the > ConfigurationAdapter->ConfigurationAdmin->configurations, the configuration > for ServiceA is not here! The configuration for ServiceB is in that list of > configurations. IF I look at a previous version of felix.scr, 2.0.2, I see > ServiceA in that list of configurations..it no longer is there, in version > 2.0.4,2.0.6, and trunk and this will result in the problem because the > revision number will not be updated. > ** ** > {noformat} > These properties are the new properties for ServiceA, that has the the > versionRequested attribute in the ServiceA object updated to versionRequest = > 2, because I changed it on the file system when the system was down and on > startup of the framework it pick up those changes on the file system and is > attempting to propagate this configuration to all necessary services that > require it. > Digging deeper in the call to delegatee.update( properties), is implemented > in org.apache.felix.cm.impl#update(Dictionary<String, ?> properties) as so, > {noformat} > * @see org.osgi.service.cm.Configuration#update(java.util.Dictionary) > */ > public void update( Dictionary<String, ?> properties ) throws IOException > { > PersistenceManager localPersistenceManager = getPersistenceManager(); > if ( localPersistenceManager != null ) > { > CaseInsensitiveDictionary newProperties = new > CaseInsensitiveDictionary( properties ); > getConfigurationManager().log( LogService.LOG_DEBUG, "Updating > config {0} with {1}", new Object[] > { getPidString(), newProperties } ); > setAutoProperties( newProperties, true ); > // persist new configuration > localPersistenceManager.store( getPidString(), newProperties ); > // finally assign the configuration for use > configure( newProperties ); > // if this is a factory configuration, update the factory with > // do this only after configuring with current properties such > // that a concurrently registered ManagedServiceFactory service > // does not receive a new/unusable configuration > updateFactory(); > // update the service and fire an CM_UPDATED event > getConfigurationManager().updated( this, true ); > } > } > {noformat} > The intresting thing here is that the reference now to 'this' > ConfigurationImpl seen on the code above `getConfigurationManager().updated( > this, true );` > has the new revision, and properties of the ConfigurationImpl, but later on > when we dig deeper in the call to getConfigurationManager().updated( this, > true ); > we will see that we cannot find the ConfigurationImpl(with the UPDATED > revision and properties) in the list of Configurations of the > ConfirurationAdmin. > Following the getConfigurationManager().updated( this, true ); code brings us > to where the ConfigurationManager perpares the eventTread and the > UpdateThread as so: > org.apache.felix.cm.impl.ConfigurationManager > {noformat} > void updated( ConfigurationImpl config, boolean fireEvent ) > { > if ( fireEvent ) > { > fireConfigurationEvent( ConfigurationEvent.CM_UPDATED, > config.getPidString(), config.getFactoryPidString() ); > } > updateThread.schedule( new UpdateConfiguration( config ) ); // this > will occur with the correct revision... > log( LogService.LOG_DEBUG, "UpdateConfiguration({0}) scheduled", new > Object[] > { config.getPid() } ); > } > {noformat} > One very important thing to note here is that the `ConfigurationImpl config` > passed into this updated(config, fireEvent) method has the correct `revision` > and `properties`. > That ConfigurationImpl is not passed into the `fireConfigurationEvent`, but > we later look it up in the ConfigAdmin...which we will see that it is not > there, and this is what I believe to be the root of the problem. > Before we get to the updateThread.schedule(), lets go ahead into the > fireConfiguraitonEvent() which creates a new FireConfigurationEvent object > for both asyncSender and for > syncSender, I did not have any syncSender so that could be ignored. This new > object gets scheduled in the `eventThread`. This thread will then loop > through each ConfigurationListener > and passes the ConfiguraitonEvent that it had created letting them know about > the changed. > Using from the sample I wrote above, ComponentA and all its properties are > part of this ConfigurationEvent. The listener that is having problems > actually doing anything with the ConfigurationEvent is the > org.apache.felix.scr.impl.manager.RegionConfigurationSupport#configurationEvent(ConfigurationEvent > event) > when we look at the code inside the implementation of that we see this line > that is returning > a ConfigurationInfo that has a revision that is revision=1, and the > properties are the updatedProperties{version=2,etc.} instead of returning > revision=2,updatedProperties{version=2,etc.} > around like 259 > {noformat} > final ConfigurationInfo configInfo = getConfigurationInfo( pid, targetedPid, > // This IS where the config returns the wrong revision > componentHolder, bundleContext ); > {noformat} > If we dig deeper to understand why the ConfigurationInfo comes back with the > correct updated properties but without updating the revision to 2 we will > inside > org.apache.felix.scr.impl.manager.RegionConfigurationSupport#getConfigurationInfo(final > TargetedPID pid, TargetedPID targetedPid, ComponentHolder<?> > componentHolder, final BundleContext bundleContext) > {noformat} > try > {//final ServiceReference caRef = > bundleContext.getServiceReference(ComponentRegistry.CONFIGURATION_ADMIN); > final ConfigurationAdmin ca = getConfigAdmin( bundleContext ); > try // this looks interesting, could be getting the wrong config > admin? ? > { > Configuration[] configs = ca.listConfigurations( filter( > pid.getRawPid() ) ); <----- this ca.listConfigurations() is the problem, like > I mentioned earlier in the ** NOTE ** > if ( configs != null && configs.length > 0 ) > { > Configuration config = configs[0]; > return new ConfigurationInfo( config.getProperties(), > config.getBundleLocation(), > config.getChangeCount() ); > } > ... the rest of the implementation > {noformat} > The problem I am seeing is the the `ConfigurationAdmin ca` does not have the > configuration in its cachedConfigs > {noformat} > // the cache of Configuration instances mapped by their PID > // have this always set to prevent NPE on bundle shutdown > private final HashMap<String, ConfigurationImpl> configurations = new > HashMap<String, ConfigurationImpl>(); > {noformat} > when it makese the call to ca.listConfigurations.. > {noformat} > // ensure the service.pid and returned a cached > config if available > ConfigurationImpl cfg = getCachedConfiguration( pid ); <----- > this returns null > if ( cfg == null ) > { > cfg = new ConfigurationImpl( this, pmList[i], config ); > } > {noformat} > it returns no configuraiton from the cache, so then we return a new > ConfigurationImpl which configures a NEW implementation of > `ConfigurationImpl` from what we stored in the PersistanceManager above > at org.apache.felix.cm.impl#update(Dictionary<String, ?> properties), but the > revision, because it's a NEW object, is set to 1. and when this goes back up > the stack and gets to > org.apache.felix.scr.impl.manager.ConfigurableComponentHolder#configurationUpdated() > The logical statement inside, > {noformat} > if (oldChangeCount != null && changeCount <= oldChangeCount > && factoryPid.equals(oldTargetedPID)) { > return false; > } > {noformat} > is TRUE so the code immediately returns false and the Component(ComponentA) > is not reconfigured with the new properties. > This DOES NOT happen on version felix.scr 2.0.2, but tracing through that > code I see a lot of code that has been refactored. The > RegionConfigurationSupport has taken over what was the ConfigurationSupport. > The revision changeCount has been refactored, and the CA has usage has also > been refactored. > It is broken in both 2.0.4, and 2.0.6. > I did a manual bisect until I was able to narrow it down, here is a snapshot > of when the behavior was introduced: > {noformat} > ------------------------------------------------------------------------ > r1747831 | djencks | 2016-06-10 18:14:36 -0700 (Fri, 10 Jun 2016) | 1 line > <------------- TEST HERE [ BROKEN ] > FELIX-5270 Don't set bundle location on configurations > ------------------------------------------------------------------------ > {noformat} > This only started occuring after the first commit to > {noformat}FELIX-5270{noformat} > Here a larger snapshot of the actual bisect steps I took: > {noformat} > FELIX-5270 log when bundle locations are inconsistent > ------------------------------------------------------------------------ > r1749929 | djencks | 2016-06-23 08:57:30 -0700 (Thu, 23 Jun 2016) | 1 line > <------------- TEST HERE [ BROKEN ] AT THIS REVISION > FELIX-5248 missing license header > ------------------------------------------------------------------------ > r1749927 | cziegeler | 2016-06-23 08:48:00 -0700 (Thu, 23 Jun 2016) | 1 line > Revert rev 1749869 > ------------------------------------------------------------------------ > r1749869 | cziegeler | 2016-06-23 04:45:33 -0700 (Thu, 23 Jun 2016) | 1 line > add missing license header > ------------------------------------------------------------------------ > r1748287 | djencks | 2016-06-13 10:14:22 -0700 (Mon, 13 Jun 2016) | 1 line > FELIX-5248 test for complaint > ------------------------------------------------------------------------ > r1747831 | djencks | 2016-06-10 18:14:36 -0700 (Fri, 10 Jun 2016) | 1 line > <------------- TEST HERE [ BROKEN ] WINNER!!! > FELIX-5270 Don't set bundle location on configurations > > ------------------------------------------------------------------------ > r1747329 | djencks | 2016-06-07 17:14:12 -0700 (Tue, 07 Jun 2016) | 1 line > <------------- TEST HERE [ WORKS ] > FELIX-5276 track service event before changing service properties > ------------------------------------------------------------------------ > r1746618 | djencks | 2016-06-02 12:36:26 -0700 (Thu, 02 Jun 2016) | 1 line > <------------- TEST HERE [ WORKS ] > FELIX-4417 Improve logging of circular references. Fix some problems > introduced with rev 1744827 when activate changes service properties. > ------------------------------------------------------------------------ > r1746617 | djencks | 2016-06-02 12:36:22 -0700 (Thu, 02 Jun 2016) | 1 line > FELIX-5264 Introduce a single State enum and use an atomic to track it, and > use some optimistic locking on state changes. This fixes the specific issue > found and should provide much easier diagnosis of any remaining or new > problems. > ------------------------------------------------------------------------ > r1746471 | gnodet | 2016-06-01 07:36:32 -0700 (Wed, 01 Jun 2016) | 1 line > <-------- TEST HERE [ WORKS ] > [FELIX-5243] Make ComponentContextImpl#setImplementationAccessible public, > similar to setImplementationObject > ------------------------------------------------------------------------ > r1746470 | gnodet | 2016-06-01 07:20:03 -0700 (Wed, 01 Jun 2016) | 1 line > [FELIX-5243] Remove anonymous inner class, add a unit test to ensure package > consistency > {noformat} -- This message was sent by Atlassian JIRA (v6.3.4#6332)