jsedding commented on code in PR #15:
URL:
https://github.com/apache/sling-org-apache-sling-i18n/pull/15#discussion_r1713325067
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -22,25 +22,13 @@
import static org.apache.sling.i18n.impl.JcrResourceBundle.PROP_LANGUAGE;
import static org.apache.sling.i18n.impl.JcrResourceBundle.PROP_PATH;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Date;
-import java.util.Dictionary;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Hashtable;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
-import java.util.MissingResourceException;
-import java.util.ResourceBundle;
-import java.util.Set;
+import java.util.*;
Review Comment:
Please revert the "*"-import.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -264,7 +247,7 @@ private void onChange(final ChangeStatus status, final
ResourceChange change)
for (final String root : languageRootPaths) {
if (change.getPath().startsWith(root)) {
// figure out which JcrResourceBundles from the cached
ones is affected
- for (JcrResourceBundle bundle :
resourceBundleCache.values()) {
+ for (JcrResourceBundle bundle :
resourceBundleRegistry.getRBs()) {
Review Comment:
I would stick with `getResourceBundles()`. It is longer, but it also reads
better IMHO.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -377,15 +360,12 @@ public void run() {
void reloadBundle(final Key key) {
log.info("Reloading resource bundle for {}", key);
if (!this.preloadBundles) {
- // remove bundle from cache
- resourceBundleCache.remove(key);
- // unregister bundle
- unregisterResourceBundle(key);
+ resourceBundleRegistry.unregisterRB(key);
Review Comment:
Same as above, I'd prefer `unregisterResourceBundle(key)`.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -431,13 +412,14 @@ protected void activate(final BundleContext context,
final Config config) throws
@Deactivate
protected void deactivate() {
+
if (this.locatorPathsTracker != null) {
this.locatorPathsTracker.close();
this.locatorPathsTracker = null;
}
+ this.resourceBundleRegistry.close();
Review Comment:
Maybe it's good to add the null check. Not sure if "deactivate" is called
after a failed "activate". That's the only scenario I see where
`this.resourceBundleRegistry` could be `null`, however.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -791,4 +733,102 @@ public String toString() {
return "Key(" + baseName + ", " + locale + ")";
}
}
+
+ /**
+ * Registry of the loaded <code>resource bundles</code> and the associated
<code>service registrations</code>
+ * The <code>ResourceBundleRegistry</code> takes care of the
registration/deregistration of the resource bundles as OSGi services.
+ * It stores the references to the registered resource bundles and to the
associated service registrations.
+ */
+ private static class ResourceBundleRegistry {
+ private final Logger log = LoggerFactory.getLogger(getClass());
+ private final BundleContext bundleContext;
+ final AtomicBoolean isClosed = new AtomicBoolean(false);
+ private final Map<Key, Entry> bundleServiceRegistrations = new
ConcurrentHashMap<>();
Review Comment:
I would rename the field to `registrations`. But it's fine either way 🙂
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -791,4 +733,102 @@ public String toString() {
return "Key(" + baseName + ", " + locale + ")";
}
}
+
+ /**
+ * Registry of the loaded <code>resource bundles</code> and the associated
<code>service registrations</code>
+ * The <code>ResourceBundleRegistry</code> takes care of the
registration/deregistration of the resource bundles as OSGi services.
+ * It stores the references to the registered resource bundles and to the
associated service registrations.
+ */
+ private static class ResourceBundleRegistry {
+ private final Logger log = LoggerFactory.getLogger(getClass());
+ private final BundleContext bundleContext;
+ final AtomicBoolean isClosed = new AtomicBoolean(false);
+ private final Map<Key, Entry> bundleServiceRegistrations = new
ConcurrentHashMap<>();
+
+ private static class Entry {
+ final JcrResourceBundle resourceBundle;
+ final ServiceRegistration<ResourceBundle> serviceRegistration;
+ Entry(JcrResourceBundle resourceBundle,
ServiceRegistration<ResourceBundle> serviceRegistration) {
+ this.resourceBundle = resourceBundle;
+ this.serviceRegistration = serviceRegistration;
+ }
+ }
+
+ ResourceBundleRegistry(BundleContext bundleContext) {
+ this.bundleContext = bundleContext;
+ }
+
+ JcrResourceBundle getRB(Key key) {
+ Entry entry = bundleServiceRegistrations.get(key);
+ return entry != null ? entry.resourceBundle : null;
+ }
+
+ Collection<JcrResourceBundle> getRBs() {
+ return bundleServiceRegistrations.values().stream().map(e ->
e.resourceBundle).collect(Collectors.toList());
+ }
+
+ void updateRB(Key key, JcrResourceBundle resourceBundle) {
Review Comment:
I would go with `registerResourceBundle()` for this one.
##########
src/test/java/org/apache/sling/i18n/impl/ConcurrentJcrResourceBundleLoadingTest.java:
##########
@@ -18,12 +18,13 @@
*/
package org.apache.sling.i18n.impl;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.fail;
+import static java.lang.Thread.sleep;
+import static org.junit.Assert.*;
Review Comment:
Avoid star imports, please.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -484,12 +466,13 @@ private ResourceBundle
getResourceBundleInternal(ResourceResolver optionalResolv
}
resourceBundle =
createResourceBundle(optionalResolver, key.baseName, key.locale);
- // put the newly created ResourceBundle to the cache.
If it replaces an existing entry unregister the existing
- // service registration first before re-registering
the new ResourceBundle.
- if (resourceBundleCache.put(key, resourceBundle) !=
null) {
- unregisterResourceBundle(key);
- }
- registerResourceBundle(key, resourceBundle);
+ resourceBundleRegistry.updateRB(key, resourceBundle);
+
+ final Set<String> languageRoots =
resourceBundle.getLanguageRootPaths();
+ this.languageRootPaths.addAll(languageRoots);
+
+ log.debug("Key {} - added service registration and
language roots {}", key, languageRoots);
+ log.info("Currently loaded dictionaries across all
locales: {}", languageRootPaths);
Review Comment:
Should these messages stay, be removed or both be on debug level?
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -791,4 +733,102 @@ public String toString() {
return "Key(" + baseName + ", " + locale + ")";
}
}
+
+ /**
+ * Registry of the loaded <code>resource bundles</code> and the associated
<code>service registrations</code>
+ * The <code>ResourceBundleRegistry</code> takes care of the
registration/deregistration of the resource bundles as OSGi services.
+ * It stores the references to the registered resource bundles and to the
associated service registrations.
+ */
+ private static class ResourceBundleRegistry {
+ private final Logger log = LoggerFactory.getLogger(getClass());
+ private final BundleContext bundleContext;
+ final AtomicBoolean isClosed = new AtomicBoolean(false);
+ private final Map<Key, Entry> bundleServiceRegistrations = new
ConcurrentHashMap<>();
+
+ private static class Entry {
+ final JcrResourceBundle resourceBundle;
+ final ServiceRegistration<ResourceBundle> serviceRegistration;
+ Entry(JcrResourceBundle resourceBundle,
ServiceRegistration<ResourceBundle> serviceRegistration) {
+ this.resourceBundle = resourceBundle;
+ this.serviceRegistration = serviceRegistration;
+ }
+ }
+
+ ResourceBundleRegistry(BundleContext bundleContext) {
+ this.bundleContext = bundleContext;
+ }
+
+ JcrResourceBundle getRB(Key key) {
+ Entry entry = bundleServiceRegistrations.get(key);
+ return entry != null ? entry.resourceBundle : null;
+ }
+
+ Collection<JcrResourceBundle> getRBs() {
+ return bundleServiceRegistrations.values().stream().map(e ->
e.resourceBundle).collect(Collectors.toList());
+ }
+
+ void updateRB(Key key, JcrResourceBundle resourceBundle) {
+ if (isClosed.get()) {
+ return;
+ }
+ ServiceRegistration<ResourceBundle> serviceReg =
bundleContext.registerService(ResourceBundle.class, resourceBundle,
serviceProps(key));
+ Entry oldEntry = bundleServiceRegistrations.put(key, new
Entry(resourceBundle, serviceReg));
+ if (oldEntry != null) {
+ oldEntry.serviceRegistration.unregister();
+ }
+ log.debug("[ResourceBundleRegistry.updateRB] Registry updated - Nr
of entries: {} - Keys: {}", bundleServiceRegistrations.size(),
bundleServiceRegistrations.keySet());
+ }
+
+ private static Dictionary<String, Object> serviceProps(Key key) {
+ Dictionary<String, Object> serviceProps = new Hashtable<>();
+ if (key.baseName != null) {
+ serviceProps.put("baseName", key.baseName);
+ }
+ serviceProps.put("locale", key.locale.toString());
+ return serviceProps;
+ }
+
+ void unregisterRB(Key key) {
+ if (isClosed.get()) {
+ return;
+ }
+ Entry oldEntry = bundleServiceRegistrations.remove(key);
+ if (oldEntry!= null) {
+ oldEntry.serviceRegistration.unregister();
+ } else {
+ log.warn("[ResourceBundleRegistry.unregisterRB] Could not find
resource bundle service for {}", key);
+ }
+ }
+
+ void clear() {
+ if (isClosed.get()) {
+ return;
+ }
+ clearInternal();
+ }
+
+ private void clearInternal() {
+ log.debug("[ResourceBundleRegistry.clearInternal] Before - Nr of
Keys: {} - Keys: {}", bundleServiceRegistrations.size(),
bundleServiceRegistrations.keySet());
+ List<ServiceRegistration<ResourceBundle>> regs = new ArrayList<>();
+ Iterator<java.util.Map.Entry<Key, Entry>> iterator =
bundleServiceRegistrations.entrySet().iterator();
+ while(iterator.hasNext()) {
+ regs.add(iterator.next().getValue().serviceRegistration);
+ iterator.remove();
+ }
Review Comment:
An alternative approach could be
- make the field `bundleServiceRegistrations` an
`AtomicReference<ConcurrentHashMap<Key, Entry>>`
- swap the map out with `AtomicReference#getAndSet()`
- take all the time you want to traverse the "old" map and unregister its
services
That might further narrow down the possibility of any races between one
thread removing the map entries and another one reading or adding/updating
entries. Not sure if that would prevent any meaningful error scenarios.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -609,22 +560,13 @@ private ResourceBundle getRootResourceBundle() {
return rootResourceBundle;
}
- private void clearCache() {
- resourceBundleCache.clear();
+ void clearCache() {
languageRootPaths.clear();
-
- final List<ServiceRegistration<ResourceBundle>> regs;
- synchronized (this) {
- regs = new ArrayList<>(bundleServiceRegistrations.values());
- bundleServiceRegistrations.clear();
- }
- for (final ServiceRegistration<ResourceBundle> serviceReg : regs) {
- serviceReg.unregister();
- }
+ resourceBundleRegistry.clear();
Review Comment:
Maybe `resourceBundleRegistry.unregisterAll()` would be a more descriptive
name?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
