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
