You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "daniancu (via GitHub)" <gi...@apache.org> on 2026/07/17 13:08:59 UTC
[PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
daniancu opened a new pull request, #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20
## Summary
`OakDiscoveryService.doUpdateProperties()` can block the Felix start-level thread indefinitely on Oak's `CommitQueue` during framework shutdown. When a `PropertyProvider` unbind/update callback fires (from a higher-start-level bundle that stops before `discovery.oak`), DS invokes the callback synchronously on the Felix start-level thread. That callback calls `ResourceResolver.commit()`, which parks on a `CountDownLatch` inside `CommitQueue` and never returns. The JVM only exits when forcibly killed by the surrounding process manager; during that window the instance remains visible in the discovery topology, delaying cluster convergence.
The fix introduces a `volatile boolean deactivating` flag that short-circuits `doUpdateProperties()` before any repository traffic begins, latched from two triggers:
1. **First line of `deactivate()`** — covers standalone component deactivation.
2. **Inline poll of the system bundle state** inside `isShuttingDown()` — covers the observed failure path where `deactivate()` is never reached. Per the OSGi spec the system bundle transitions to `STOPPING` before any other bundle begins stopping, so the poll sees it on the very first guarded callback.
The system bundle reference is cached at activation via `FrameworkUtil.getBundle(...)` so unit tests without an OSGi runtime degrade cleanly to the `deactivate()`-only path.
The fix does **not** interrupt an in-flight commit — it only prevents new commits from starting once shutdown has begun.
## Tests
- `testIsShuttingDownAfterDeactivate` — deactivate flips the flag; subsequent callbacks succeed without touching the repository.
- `testInlineSystemBundleStateFallback` — system bundle `STOPPING` state latches the flag without `deactivate()` being called (the primary failure path).
- `testInvalidatedSystemBundleTreatedAsShuttingDown` — `IllegalStateException` from `getState()` is treated as shutting down.
- `testCacheSystemBundleWithInvalidatedContextLatchesDeactivating` — `IllegalStateException` from `getBundleContext()` at activation latches `deactivating` immediately.
- `testActivatedServiceSkipsResourceResolverWhenSystemBundleStopping` — main acceptance criterion: `Mockito.verifyNoInteractions(rrfMock)` confirms no repository write is attempted once the system bundle is `STOPPING`, even with the service fully activated.
All 22 tests pass (`OakDiscoveryServiceTest` + `its.OakDiscoveryServiceTest`).
## Related
- SLING-5382, SLING-3389, SLING-10204
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3751397613
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Test-only hook to install a system-bundle reference without going
+ * through {@link #cacheSystemBundle()}.
+ */
+ void setSystemBundleForTesting(final Bundle bundle) {
+ this.systemBundle.set(bundle);
+ }
+
+ private void cacheSystemBundle() {
+ cacheSystemBundle(FrameworkUtil.getBundle(OakDiscoveryService.class));
+ }
+
+ /**
+ * Package-private overload taking the caller-bundle explicitly, so unit
+ * tests can exercise the {@link BundleContext} failure branches without
+ * an OSGi framework.
+ */
+ void cacheSystemBundle(final Bundle ourBundle) {
+ if (ourBundle == null) {
+ logger.debug("cacheSystemBundle: no OSGi framework detected via FrameworkUtil"
+ + " - shutdown detection degrades to deactivate() only");
+ return;
+ }
+ try {
+ final BundleContext ctx = ourBundle.getBundleContext();
Review Comment:
Appreciate if you could fix that in a dedicated PR and then rebase this branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5092883831
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate failed**
Failed conditions
 [B Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) (required ≥ A)
<!-- sqra:qg_revision=f1454b82442dd063e60770343a6e87013026fba6b1388872829fe4e0f2e49930 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5102428233
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [94.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5214578804
applied @eglistefan suggestions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5100895560
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate failed**
Failed conditions
 [B Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) (required ≥ A)
<!-- sqra:qg_revision=5fc1c85d22fefd52e01832c65414de36a1b7065101ebd0c89d42c7ce89167a1a -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5092284138
@joerghoh can you re-trigger the build pipeline, seems that one test is flaky
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5057881551
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate failed**
Failed conditions
 [79.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_coverage&view=list) (required ≥ 80%)
 [B Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) (required ≥ A)
<!-- sqra:qg_revision=4b800481a6dc0cbfa69d41152ec78776299254085811e3488023bcbdeaf1ccc8 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
##
💡 **Need a hand with PR review?** Try [Gitar by Sonar](https://gitar.ai/?utm_source=sonarqube-cloud&utm_medium=in-product&utm_campaign=gitar-crosssell-q3-2026&utm_content=pr-deco)!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5031793405
some general remarks regarding the inline comments:
* remove any reference to the actual SLING ticket, unless the code is absolutely counter-intuitive and requires the additional context of the SLING ticket.
* keep the comments brief. Do not over-explain potential root-causes or how a situation evolves, all of that belongs in the Jira ticket and/or the PR documentation.
* in general: Imagine that in 12 months you need to adjust this code/class again. Then you are NOT interested in verbose comments which just clutter the code, especially if the aspect you need to change/augment is a different one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5214204076
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5175845374
> There seems to be a larger refactoring included which splits up the code of {{doUpdateProperties()}} into multiple methods. Is this required in the context of this PR? If not, it should be a PR on its own.
After the small change this PR added to `doUpdateProperties`, Sonar flagged the method as `Brain method` and I fixed it here (last 3/5 commits). Now I think I could have done the refactoring in a separated PR, merge that one than rebase this one on it. Let me do that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5181256658
@joerghoh please check https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/21 - refactoring of doUpdateProperties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5166541903
There seems to be a larger refactoring included which splits up the code of {{doUpdateProperties()}} into multiple methods. Is this required in the context of this PR? If not, it should be a PR on its own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5070957409
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate failed**
Failed conditions
 [B Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) (required ≥ A)
<!-- sqra:qg_revision=f1454b82442dd063e60770343a6e87013026fba6b1388872829fe4e0f2e49930 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5059055497
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate failed**
Failed conditions
 [79.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_coverage&view=list) (required ≥ 80%)
 [B Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) (required ≥ A)
<!-- sqra:qg_revision=950394c21534478916c85415316eac68192eeebe76f2797f816f93ba0612f664 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
##
💡 **Need a hand with PR review?** Try [Gitar by Sonar](https://gitar.ai/?utm_source=sonarqube-cloud&utm_medium=in-product&utm_campaign=gitar-crosssell-q3-2026&utm_content=pr-deco)!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3734185572
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -105,6 +108,17 @@ public class OakDiscoveryService extends BaseDiscoveryService {
**/
private volatile boolean activated = false;
+ /**
+ * Latched to {@code true} once this component or the OSGi framework begins
+ * shutting down. See {@link #isShuttingDown()}.
+ */
+ private volatile boolean deactivating = false;
+
+ /**
+ * Cached system bundle (bundle 0), resolved once at activation.
+ */
+ private final AtomicReference<Bundle> systemBundle = new AtomicReference<>();
Review Comment:
I made that change only because Sonar insisted, otherwise the build failed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3703989639
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -515,53 +546,50 @@ private void doUpdateProperties() {
resourceResolver.refresh();
final ModifiableValueMap myInstanceMap = myInstance.adaptTo(ModifiableValueMap.class);
- final Set<String> keys = new HashSet<String>(myInstanceMap.keySet());
- for (final String key : keys) {
- if (newProps.containsKey(key)) {
- // perfect
- continue;
- } else if (key.indexOf(":") != -1) {
- // ignore
- continue;
- } else {
- // remove
- myInstanceMap.remove(key);
- }
- }
-
- boolean anyChanges = false;
- for (final Entry<String, String> entry : newProps.entrySet()) {
- Object existingValue = myInstanceMap.get(entry.getKey());
- if (entry.getValue().equals(existingValue)) {
- // SLING-3389: dont rewrite the properties if nothing changed!
- if (logger.isDebugEnabled()) {
- logger.debug("doUpdateProperties: unchanged: {}={}", entry.getKey(), entry.getValue());
- }
- continue;
- }
- if (logger.isDebugEnabled()) {
- logger.debug("doUpdateProperties: changed: {}={}", entry.getKey(), entry.getValue());
- }
- anyChanges = true;
- myInstanceMap.put(entry.getKey(), entry.getValue());
- }
-
- if (anyChanges) {
+ removeStaleKeys(myInstanceMap, newProps.keySet());
+ if (applyNewProperties(myInstanceMap, newProps)) {
resourceResolver.commit();
}
} catch (LoginException e) {
- logger.error("handleEvent: could not log in administratively: " + e, e);
+ logger.error("doUpdateProperties: could not log in administratively: " + e, e);
throw new RuntimeException("Could not log in to repository (" + e + ")", e);
} catch (PersistenceException e) {
- logger.error("handleEvent: got a PersistenceException: " + e, e);
- throw new RuntimeException( "Exception while talking to repository (" + e + ")", e);
+ logger.error("doUpdateProperties: got a PersistenceException: " + e, e);
+ throw new RuntimeException("Exception while talking to repository (" + e + ")", e);
} finally {
Review Comment:
NIT and while you are at it: can you use a try-with-resource approach here for more compact code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5178562746
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3720259946
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Test-only hook to install a system-bundle reference without going
+ * through {@link #cacheSystemBundle()}.
+ */
+ void setSystemBundleForTesting(final Bundle bundle) {
+ this.systemBundle.set(bundle);
+ }
+
+ private void cacheSystemBundle() {
+ cacheSystemBundle(FrameworkUtil.getBundle(OakDiscoveryService.class));
+ }
+
+ /**
+ * Package-private overload taking the caller-bundle explicitly, so unit
+ * tests can exercise the {@link BundleContext} failure branches without
+ * an OSGi framework.
+ */
+ void cacheSystemBundle(final Bundle ourBundle) {
+ if (ourBundle == null) {
+ logger.debug("cacheSystemBundle: no OSGi framework detected via FrameworkUtil"
+ + " - shutdown detection degrades to deactivate() only");
+ return;
+ }
+ try {
+ final BundleContext ctx = ourBundle.getBundleContext();
Review Comment:
More a nitpick than an actual issue: There is a version of the ``activate()`` method which passes in the ComponentContext; from that you can derive the BundleContext of this bundle directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3729180636
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Test-only hook to install a system-bundle reference without going
+ * through {@link #cacheSystemBundle()}.
+ */
+ void setSystemBundleForTesting(final Bundle bundle) {
+ this.systemBundle.set(bundle);
+ }
+
+ private void cacheSystemBundle() {
+ cacheSystemBundle(FrameworkUtil.getBundle(OakDiscoveryService.class));
+ }
+
+ /**
+ * Package-private overload taking the caller-bundle explicitly, so unit
+ * tests can exercise the {@link BundleContext} failure branches without
+ * an OSGi framework.
+ */
+ void cacheSystemBundle(final Bundle ourBundle) {
+ if (ourBundle == null) {
+ logger.debug("cacheSystemBundle: no OSGi framework detected via FrameworkUtil"
+ + " - shutdown detection degrades to deactivate() only");
+ return;
+ }
+ try {
+ final BundleContext ctx = ourBundle.getBundleContext();
Review Comment:
I made the changes but broke existing tests. To make it work we need a small change in `MockFactory` from `sling-org-apache-sling-discovery-base` module. But that module was bumped to 2.1.0 and `MockFactory` was removed as per https://github.com/apache/sling-org-apache-sling-discovery-base/commit/729a566d627f3f393b45b88826ee87fe6ef6da7c#diff-cdc19a365ec199f6ae169da2474404cb63b52061a1de7d5b2132ef2a5b46211e
Tests in `sling-org-apache-sling-discovery-base` were converted, those in this module, not. Sounds like a new PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3730608551
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
Review Comment:
`logger.info` sounds fine, but perhaps logging the exception wouldn't hurt?
```suggestion
logger.info("isShuttingDown: system bundle reference invalidated - "
+ "OakDiscoveryService will skip further property writes : {}", e.toString());
```
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Test-only hook to install a system-bundle reference without going
+ * through {@link #cacheSystemBundle()}.
+ */
+ void setSystemBundleForTesting(final Bundle bundle) {
+ this.systemBundle.set(bundle);
+ }
+
+ private void cacheSystemBundle() {
+ cacheSystemBundle(FrameworkUtil.getBundle(OakDiscoveryService.class));
+ }
+
+ /**
+ * Package-private overload taking the caller-bundle explicitly, so unit
+ * tests can exercise the {@link BundleContext} failure branches without
+ * an OSGi framework.
+ */
+ void cacheSystemBundle(final Bundle ourBundle) {
+ if (ourBundle == null) {
+ logger.debug("cacheSystemBundle: no OSGi framework detected via FrameworkUtil"
+ + " - shutdown detection degrades to deactivate() only");
Review Comment:
might be worth a `logger.info`?
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Test-only hook to install a system-bundle reference without going
+ * through {@link #cacheSystemBundle()}.
+ */
+ void setSystemBundleForTesting(final Bundle bundle) {
+ this.systemBundle.set(bundle);
+ }
+
+ private void cacheSystemBundle() {
+ cacheSystemBundle(FrameworkUtil.getBundle(OakDiscoveryService.class));
+ }
+
+ /**
+ * Package-private overload taking the caller-bundle explicitly, so unit
+ * tests can exercise the {@link BundleContext} failure branches without
+ * an OSGi framework.
+ */
+ void cacheSystemBundle(final Bundle ourBundle) {
+ if (ourBundle == null) {
+ logger.debug("cacheSystemBundle: no OSGi framework detected via FrameworkUtil"
+ + " - shutdown detection degrades to deactivate() only");
+ return;
+ }
+ try {
+ final BundleContext ctx = ourBundle.getBundleContext();
+ if (ctx == null) {
+ logger.debug("cacheSystemBundle: our BundleContext is null"
+ + " - shutdown detection degrades to deactivate() only");
Review Comment:
same here, maybe worth a `logger.info` instead?
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -319,6 +335,9 @@ protected void activate(final BundleContext bundleContext) {
*/
@Deactivate
protected void deactivate() {
+ // set before acquiring any lock so concurrent PropertyProvider
+ // callbacks short-circuit in doUpdateProperties()
+ deactivating = true;
Review Comment:
as there is no trace of the new behaviour (depending on `isShuttingDown()` of course, but `doUpdateProperties()` does only `logger.debug`) - what about adding a `logger.info` here saying something like "further updates will be suppressed" or similar? it could also be a good marker that makes it clear that this new version would be in use.
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -105,6 +108,17 @@ public class OakDiscoveryService extends BaseDiscoveryService {
**/
private volatile boolean activated = false;
+ /**
+ * Latched to {@code true} once this component or the OSGi framework begins
+ * shutting down. See {@link #isShuttingDown()}.
+ */
+ private volatile boolean deactivating = false;
+
+ /**
+ * Cached system bundle (bundle 0), resolved once at activation.
+ */
+ private final AtomicReference<Bundle> systemBundle = new AtomicReference<>();
Review Comment:
nit: any advantage of using `AtomicReference` here vs the previous `private volatile Bundle systemBundle` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#issuecomment-5214126654
## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-discovery-oak&pullRequest=20)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SLING-13259 - OakDiscoveryService: skip property update during framework shutdown [sling-org-apache-sling-discovery-oak]
Posted by "daniancu (via GitHub)" <gi...@apache.org>.
daniancu commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-discovery-oak/pull/20#discussion_r3803493614
##########
src/main/java/org/apache/sling/discovery/oak/OakDiscoveryService.java:
##########
@@ -581,6 +604,80 @@ public void updateProperties() {
}
}
+ /**
+ * Returns {@code true} when this component or the framework is shutting
+ * down. Latches {@link #deactivating} on first observation so subsequent
+ * calls are a single volatile read. Package-private for unit testing.
+ */
+ boolean isShuttingDown() {
+ if (deactivating) {
+ return true;
+ }
+ final Bundle sb = systemBundle.get();
+ if (sb == null) {
+ return false;
+ }
+ try {
+ final int state = sb.getState();
+ if (state == Bundle.STOPPING
+ || state == Bundle.RESOLVED
+ || state == Bundle.INSTALLED
+ || state == Bundle.UNINSTALLED) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle state {} observed - "
+ + "OakDiscoveryService will skip further property writes", state);
+ return true;
+ }
+ } catch (IllegalStateException e) {
+ deactivating = true;
+ logger.info("isShuttingDown: system bundle reference invalidated - "
+ + "OakDiscoveryService will skip further property writes");
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Test-only hook to install a system-bundle reference without going
+ * through {@link #cacheSystemBundle()}.
+ */
+ void setSystemBundleForTesting(final Bundle bundle) {
+ this.systemBundle.set(bundle);
+ }
+
+ private void cacheSystemBundle() {
+ cacheSystemBundle(FrameworkUtil.getBundle(OakDiscoveryService.class));
+ }
+
+ /**
+ * Package-private overload taking the caller-bundle explicitly, so unit
+ * tests can exercise the {@link BundleContext} failure branches without
+ * an OSGi framework.
+ */
+ void cacheSystemBundle(final Bundle ourBundle) {
+ if (ourBundle == null) {
+ logger.debug("cacheSystemBundle: no OSGi framework detected via FrameworkUtil"
+ + " - shutdown detection degrades to deactivate() only");
+ return;
+ }
+ try {
+ final BundleContext ctx = ourBundle.getBundleContext();
Review Comment:
> Appreciate if you could fix that in a dedicated PR and then rebase this branch.
Created [SLING-13310](https://issues.apache.org/jira/browse/SLING-13310)
Will start working on it asap
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org