Hello I've added quite a long comment under https://issues.apache.org/jira/browse/KARAF-6074 showing my investigation.
So this is actually NOT a race condition - feature configs with dash in name will ALWAYS create two configurations from the factory PID. And yes - reverting https://github.com/apache/karaf/commit/1221b0158d2494523cf94cc3f223bd552a2467c7 helps with duplicate PID, but actually gives you REAL race condition (happening intermittently) described at https://issues.apache.org/jira/browse/KARAF-7389. What actually is needed is: - if (pid.contains("~")) { - cid.name = pid.substring(n + 1); - } + cid.name = pid.substring(n + 1); which ensures that both Karaf and felix.fileinstall operate on the same PID (and not only factory PID). let me prepare a PR. regards Grzegorz Grzybek śr., 31 maj 2023 o 17:47 Grzegorz Grzybek <[email protected]> napisał(a): > I personally didn't check with factory pids (dash in the name). > I remember I spotted the problem when dealing with FeatureDeployer > (deploy/ folder) for Fuse - > https://issues.apache.org/jira/browse/KARAF-6074 (but not necessarily > related to this config problem). > > Also from quick search of my history, > https://issues.apache.org/jira/browse/KARAF-7389 was needed to prevent > reading half-written file. > > More - tomorrow (CEST) ;) > > regards > Grzegorz Grzybek > > śr., 31 maj 2023 o 17:25 Jean-Baptiste Onofré <[email protected]> > napisał(a): > >> Good catch. >> >> I remember we investigated some race condition issues with Greg. >> >> Let me check if the name matches. >> >> Regards >> JB >> >> On Wed, May 31, 2023 at 4:45 PM Jesse White <[email protected]> wrote: >> > >> > Good find Ben, and thanks Grzegorz. >> > >> > Reverting that commit does solve the problem, so it must be related. >> > >> > I also found that this change does the trick: >> > >> > diff --git >> a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> > index 40bb666a34..fce2c1221c 100644 >> > --- >> a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> > +++ >> b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> > @@ -68,9 +68,7 @@ public class FeatureConfigInstaller { >> > if (n > 0) { >> > cid.isFactoryPid = true; >> > cid.factoryPid = pid.substring(0, n); >> > - if (pid.contains("~")) { >> > - cid.name = pid.substring(n + 1); >> > - } >> > + cid.name = pid.substring(n + 1); >> > } >> > return cid; >> > } >> > >> > I guess there's some mismatch about the "name" of the config, so it >> gets treated as two distinct objects. >> > >> > Best, >> > Jesse >> > ________________________________ >> > From: Grzegorz Grzybek <[email protected]> >> > Sent: Wednesday, May 31, 2023 10:33 AM >> > To: [email protected] <[email protected]> >> > Subject: Re: same configuration registered with multiple PIDs in >> ManagedServiceFactory >> > >> > >> > EXTERNAL EMAIL DON'T BE QUICK TO CLICK >> > >> > If you believe this email is suspicious, report via ‘Phish Alert >> Report’ button >> > >> > ________________________________ >> > Hello >> > >> > I remember adding some safety logic related to feature-embedded >> configuration. Because you're using dash ("-") in the PID, you're actually >> adding a factory PID. And there may be some hidden race condition here. >> > >> > Let me check this problem tomorrow. >> > >> > regards >> > Grzegorz Grzybek >> > >> > śr., 31 maj 2023 o 16:29 [email protected] <[email protected]> >> napisał(a): >> > >> > Looking at a diff between 4.3.6 and 4.3.7, the only thing that seems >> relevant is this change, maybe the issue with multiple threads writing the >> config at the same time needs to be solved in a different way? >> > >> > >> > >> > My guess is this fixed the race condition by making it atomic, but it >> doesn’t stop it from happening twice in quick succession. >> > >> > commit 1221b0158d2494523cf94cc3f223bd552a2467c7 >> > >> > Author: Grzegorz Grzybek [email protected] >> > >> > Date: Wed Feb 9 11:53:33 2022 +0100 >> > >> > >> > >> > [KARAF-7389] Prevent two threads (feature installer, CM Event >> Dispatcher through fileinstall) writing the same config file >> > >> > >> > >> > (cherry picked from commit f3260d5ab641cdbf1bbd594875c07d974ed470a0) >> > >> > >> > >> > diff --git >> a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> > >> > index ba46eb3a2d..40bb666a34 100644 >> > >> > --- >> a/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> > >> > +++ >> b/features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java >> > >> > @@ -139,2 +139,3 @@ public class FeatureConfigInstaller { >> > >> > properties.put(CONFIG_KEY, cid.pid); >> > >> > + cfg.update(cfgProps); >> > >> > if (storage != null && configCfgStore) { >> > >> > @@ -142,3 +143,2 @@ public class FeatureConfigInstaller { >> > >> > } >> > >> > - cfg.update(cfgProps); >> > >> > try { >> > >> > @@ -327,7 +327,9 @@ public class FeatureConfigInstaller { >> > >> > if (!cfgFile.exists()) { >> > >> > + File tmpCfgFile = >> File.createTempFile(cfgFile.getName(), ".tmp", cfgFile.getParentFile()); >> > >> > if (jsonFormat) { >> > >> > - Configurations.buildWriter().build(new >> FileWriter(cfgFile)).writeConfiguration(convertToDict(props)); >> > >> > + Configurations.buildWriter().build(new >> FileWriter(tmpCfgFile)).writeConfiguration(convertToDict(props)); >> > >> > } else { >> > >> > - props.save(cfgFile); >> > >> > + props.save(tmpCfgFile); >> > >> > } >> > >> > + tmpCfgFile.renameTo(cfgFile); >> > >> > } else { >> > >> > >> > >> > From: Jesse White <[email protected]> >> > Date: Tuesday, May 30, 2023 at 7:11 PM >> > To: [email protected] <[email protected]> >> > Subject: same configuration registered with multiple PIDs in >> ManagedServiceFactory >> > >> > Hey folks, >> > >> > >> > >> > I've encountered some odd behavior with Karaf 4.4.3, and I'd like to >> confirm if this is a bug or if there are some settings I can tune to alter >> the behavior. >> > >> > >> > >> > Here's how to reproduce: >> > >> > Start Karaf 4.4.3 w/ JDK 11 >> > Install the managed factory example >> > >> > feature:repo-add >> mvn:org.apache.karaf.examples/karaf-config-example-features/4.4.3/xml >> > >> > feature:install karaf-config-example-managed-factory >> > >> > Copy the attached 'config.xml' file to the 'deploy/' directory >> > Wait about 5 seconds, and notice the following output to the console >> > >> > New configuration with pid >> org.apache.karaf.example.config.04388423-2d46-4308-8214-3dcd1e0b8fd0 >> > >> > key1 = value1 >> > >> > key2 = value2 >> > >> > org.apache.karaf.features.configKey = >> org.apache.karaf.example.config-abc >> > >> > service.factoryPid = org.apache.karaf.example.config >> > >> > service.pid = >> org.apache.karaf.example.config.04388423-2d46-4308-8214-3dcd1e0b8fd0 >> > >> > 18:42:02.131 INFO [features-3-thread-1] Done. >> > >> > 18:42:10.999 INFO >> [fileinstall-/Users/jesse/labs/karaf/apache-karaf-4.4.3/etc] Creating >> configuration {org.apache.karaf.example.config~abc} from >> /Users/jesse/labs/karaf/apache-karaf-4.4.3/etc/org.apache.karaf.example.config-abc.cfg >> > >> > New configuration with pid org.apache.karaf.example.config~abc >> > >> > felix.fileinstall.filename = >> file:/Users/jesse/labs/karaf/apache-karaf-4.4.3/etc/org.apache.karaf.example.config-abc.cfg >> > >> > key1 = value1 >> > >> > key2 = value2 >> > >> > org.apache.karaf.features.configKey = >> org.apache.karaf.example.config-abc >> > >> > service.factoryPid = org.apache.karaf.example.config >> > >> > service.pid = org.apache.karaf.example.config~abc >> > >> > >> > >> > >> > >> > Note that the single config results in multiple callbacks under two >> separate pids. This seems isolated to cases where the deploy/ folder is >> used to write the config, and doesn't happen when the configuration is >> manually placed in etc/. >> > >> > >> > >> > Following the same instructions with Karaf 4.3.6 has the expected >> behavior. Karaf 4.3.7 and later experience this issue though. >> > >> > >> > >> > Any ideas? >> > >> > >> > >> > Thanks, >> > >> > Jesse >> > >> > >> > >> > >> > >> > >> > >> > CONFIDENTIALITY NOTICE >> > This e-mail message and any attachments are only for the use of the >> intended recipient and may contain information that is privileged, >> confidential or exempt from disclosure under applicable law. If you are not >> the intended recipient, any disclosure, distribution or other use of this >> e-mail message or attachments is prohibited. If you have received this >> e-mail message in error, please delete and notify the sender immediately. >> Thank you. >> >
