Re: [PR] SLING-12315: avoid new registrations during deactivation [sling-org-apache-sling-i18n]
jsedding commented on code in PR #14:
URL:
https://github.com/apache/sling-org-apache-sling-i18n/pull/14#discussion_r1601048338
##
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##
@@ -436,8 +439,10 @@ protected void deactivate() {
this.locatorPathsTracker = null;
}
-clearCache();
-this.bundleContext = null;
+synchronized(this) {
Review Comment:
Agreed that the thread-safety model of the class is confusing.
That said, I consider the `bundleContext` and `bundleServiceRegistrations`
fields to have a strong relation, because the registrations implicitly pertain
to the bundle context. The synchronized block in deactivate makes sure that no
thread-scheduling can lead to a registration being added to
`bundleServiceRegistrations` once `bundleContext` has been set to `null`. Such
registrations would never be unregistered, which is why I think it is important
to guard against this case.
In order to simplify the thread-safety model, it might be sensible to
encapsulate the concern of "bundle service registrations and deregistration"
into its own class. This class would receive the bundle context in the
constructor, removing the need for a `bundleContext` field in
`JcrResourceBundleProvider`. All registrations, unregistrations, book keeping
of registrations, and thread-safety concerns would be encapsulated. The object
would be created in the activate method and closed in the deactivate method.
WDYT? I believe that could be a start towards reducing complexity.
--
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-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12315: avoid new registrations during deactivation [sling-org-apache-sling-i18n]
rombert commented on code in PR #14:
URL:
https://github.com/apache/sling-org-apache-sling-i18n/pull/14#discussion_r1599862705
##
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##
@@ -436,8 +439,10 @@ protected void deactivate() {
this.locatorPathsTracker = null;
}
-clearCache();
-this.bundleContext = null;
+synchronized(this) {
Review Comment:
I think you're looking at interleaved access between `deactivate()` and
`registerResourceBundle`, right? That is the only scenario where
`bundleServiceRegistrations` will receive new entries.
It looks like it's theoretically possible for this scenario to happen,
although I see the `bundleContext != null` checks on every code path and I am
very skeptical that a runtime compiler will reorder staments in face of
multi-thread volatile reads and writes.
I find the overall thread safety model of this class quite confusing (
multiple volatile fields, concurrent maps with Semaphore keys, synchronized
blocks ( on `this` (!) , on a private collection ) and I think expanding the
scope of the synchronized block would make things more confusing.
If you think that it's worth protecting the `bundleContext` and
`bundleServiceRegistrations` together I would prefer to not make the
`bundleContext` synchronized and instead guard access to those two fields all
the time using the same lock. Of course, that can have performance implications
so care must be taken.
--
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-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12315: avoid new registrations during deactivation [sling-org-apache-sling-i18n]
sonarcloud[bot] commented on PR #14: URL: https://github.com/apache/sling-org-apache-sling-i18n/pull/14#issuecomment-2107393290 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-i18n=14) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-i18n=14=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-i18n=14=false=true)  [81.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-i18n=14) -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12315: avoid new registrations during deactivation [sling-org-apache-sling-i18n]
sonarcloud[bot] commented on PR #14: URL: https://github.com/apache/sling-org-apache-sling-i18n/pull/14#issuecomment-2106042064 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-i18n=14) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-i18n=14=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-i18n=14=false=true)  [81.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-i18n=14) -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12315: avoid new registrations during deactivation [sling-org-apache-sling-i18n]
sonarcloud[bot] commented on PR #14: URL: https://github.com/apache/sling-org-apache-sling-i18n/pull/14#issuecomment-2101220350 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-i18n=14) **Quality Gate passed** Issues  [1 New issue](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-i18n=14=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-i18n=14=false=true)  [81.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-i18n=14=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-i18n=14) -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
