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