[ 
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)

Reply via email to