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
