From a000dd782a74bb27484e2d76760e76e8398a32e8 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 21 Aug 2014 14:24:28 +0200 Subject: [PATCH] ReloadableResourceBundleMessageSource uses ConcurrentHashMaps and ReentrantLocks instead of synchronization Issue: SPR-10500 --- ...ReloadableResourceBundleMessageSource.java | 240 +++++++++++------- .../ResourceBundleMessageSourceTests.java | 85 +++++-- 2 files changed, 209 insertions(+), 116 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/context/support/ReloadableResourceBundleMessageSource.java b/spring-context/src/main/java/org/springframework/context/support/ReloadableResourceBundleMessageSource.java index c145549d79..c8d56691b7 100644 --- a/spring-context/src/main/java/org/springframework/context/support/ReloadableResourceBundleMessageSource.java +++ b/spring-context/src/main/java/org/springframework/context/support/ReloadableResourceBundleMessageSource.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,11 +21,13 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.text.MessageFormat; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.locks.ReentrantLock; import org.springframework.context.ResourceLoaderAware; import org.springframework.core.io.DefaultResourceLoader; @@ -92,8 +94,7 @@ import org.springframework.util.StringUtils; * @see ResourceBundleMessageSource * @see java.util.ResourceBundle */ -public class ReloadableResourceBundleMessageSource extends AbstractMessageSource - implements ResourceLoaderAware { +public class ReloadableResourceBundleMessageSource extends AbstractMessageSource implements ResourceLoaderAware { private static final String PROPERTIES_SUFFIX = ".properties"; @@ -110,19 +111,23 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource private long cacheMillis = -1; + private boolean concurrentRefresh = true; + private PropertiesPersister propertiesPersister = new DefaultPropertiesPersister(); private ResourceLoader resourceLoader = new DefaultResourceLoader(); /** Cache to hold filename lists per Locale */ - private final Map>> cachedFilenames = - new HashMap>>(); + private final ConcurrentMap>> cachedFilenames = + new ConcurrentHashMap>>(); /** Cache to hold already loaded properties per filename */ - private final Map cachedProperties = new HashMap(); + private final ConcurrentMap cachedProperties = + new ConcurrentHashMap(); /** Cache to hold merged loaded properties per locale */ - private final Map cachedMergedProperties = new HashMap(); + private final ConcurrentMap cachedMergedProperties = + new ConcurrentHashMap(); /** @@ -231,6 +236,20 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource this.cacheMillis = (cacheSeconds * 1000); } + /** + * Specify whether to allow for concurrent refresh behavior, i.e. one thread + * locked in a refresh attempt for a specific cached properties file whereas + * other threads keep returning the old properties for the time being, until + * the refresh attempt has completed. + *

Default is "true": This behavior is new as of Spring Framework 4.1, + * minimizing contention between threads. If you prefer the old behavior, + * i.e. to fully block on refresh, switch this flag to "false". + * @see #setCacheSeconds + */ + public void setConcurrentRefresh(boolean concurrentRefresh) { + this.concurrentRefresh = concurrentRefresh; + } + /** * Set the PropertiesPersister to use for parsing properties files. *

The default is a DefaultPropertiesPersister. @@ -322,26 +341,27 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource * cached forever. */ protected PropertiesHolder getMergedProperties(Locale locale) { - synchronized (this.cachedMergedProperties) { - PropertiesHolder mergedHolder = this.cachedMergedProperties.get(locale); - if (mergedHolder != null) { - return mergedHolder; - } - Properties mergedProps = new Properties(); - mergedHolder = new PropertiesHolder(mergedProps, -1); - for (int i = this.basenames.length - 1; i >= 0; i--) { - List filenames = calculateAllFilenames(this.basenames[i], locale); - for (int j = filenames.size() - 1; j >= 0; j--) { - String filename = filenames.get(j); - PropertiesHolder propHolder = getProperties(filename); - if (propHolder.getProperties() != null) { - mergedProps.putAll(propHolder.getProperties()); - } - } - } - this.cachedMergedProperties.put(locale, mergedHolder); + PropertiesHolder mergedHolder = this.cachedMergedProperties.get(locale); + if (mergedHolder != null) { return mergedHolder; } + Properties mergedProps = new Properties(); + mergedHolder = new PropertiesHolder(mergedProps, -1); + for (int i = this.basenames.length - 1; i >= 0; i--) { + List filenames = calculateAllFilenames(this.basenames[i], locale); + for (int j = filenames.size() - 1; j >= 0; j--) { + String filename = filenames.get(j); + PropertiesHolder propHolder = getProperties(filename); + if (propHolder.getProperties() != null) { + mergedProps.putAll(propHolder.getProperties()); + } + } + } + PropertiesHolder existing = this.cachedMergedProperties.putIfAbsent(locale, mergedHolder); + if (existing != null) { + mergedHolder = existing; + } + return mergedHolder; } /** @@ -355,36 +375,34 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource * @see #calculateFilenamesForLocale */ protected List calculateAllFilenames(String basename, Locale locale) { - synchronized (this.cachedFilenames) { - Map> localeMap = this.cachedFilenames.get(basename); - if (localeMap != null) { - List filenames = localeMap.get(locale); - if (filenames != null) { - return filenames; - } + Map> localeMap = this.cachedFilenames.get(basename); + if (localeMap != null) { + List filenames = localeMap.get(locale); + if (filenames != null) { + return filenames; } - List filenames = new ArrayList(7); - filenames.addAll(calculateFilenamesForLocale(basename, locale)); - if (this.fallbackToSystemLocale && !locale.equals(Locale.getDefault())) { - List fallbackFilenames = calculateFilenamesForLocale(basename, Locale.getDefault()); - for (String fallbackFilename : fallbackFilenames) { - if (!filenames.contains(fallbackFilename)) { - // Entry for fallback locale that isn't already in filenames list. - filenames.add(fallbackFilename); - } - } - } - filenames.add(basename); - if (localeMap != null) { - localeMap.put(locale, filenames); - } - else { - localeMap = new HashMap>(); - localeMap.put(locale, filenames); - this.cachedFilenames.put(basename, localeMap); - } - return filenames; } + List filenames = new ArrayList(7); + filenames.addAll(calculateFilenamesForLocale(basename, locale)); + if (this.fallbackToSystemLocale && !locale.equals(Locale.getDefault())) { + List fallbackFilenames = calculateFilenamesForLocale(basename, Locale.getDefault()); + for (String fallbackFilename : fallbackFilenames) { + if (!filenames.contains(fallbackFilename)) { + // Entry for fallback locale that isn't already in filenames list. + filenames.add(fallbackFilename); + } + } + } + filenames.add(basename); + if (localeMap == null) { + localeMap = new ConcurrentHashMap>(); + Map> existing = this.cachedFilenames.putIfAbsent(basename, localeMap); + if (existing != null) { + localeMap = existing; + } + } + localeMap.put(locale, filenames); + return filenames; } /** @@ -432,16 +450,46 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource * @return the current PropertiesHolder for the bundle */ protected PropertiesHolder getProperties(String filename) { - synchronized (this.cachedProperties) { - PropertiesHolder propHolder = this.cachedProperties.get(filename); - if (propHolder != null && - (propHolder.getRefreshTimestamp() < 0 || - propHolder.getRefreshTimestamp() > System.currentTimeMillis() - this.cacheMillis)) { - // up to date + PropertiesHolder propHolder = this.cachedProperties.get(filename); + long originalTimestamp = -1; + + if (propHolder != null) { + originalTimestamp = propHolder.getRefreshTimestamp(); + if (originalTimestamp < 0 || originalTimestamp > System.currentTimeMillis() - this.cacheMillis) { + // Up to date return propHolder; } + } + else { + propHolder = new PropertiesHolder(); + PropertiesHolder existingHolder = this.cachedProperties.putIfAbsent(filename, propHolder); + if (existingHolder != null) { + propHolder = existingHolder; + } + } + + // At this point, we need to refresh... + if (this.concurrentRefresh && propHolder.getRefreshTimestamp() >= 0) { + // A populated but stale holder -> could keep using it. + if (!propHolder.refreshLock.tryLock()) { + // Getting refreshed by another thread already -> + // let's return the existing properties for the time being. + return propHolder; + } + } + else { + propHolder.refreshLock.lock(); + } + try { + PropertiesHolder existingHolder = this.cachedProperties.get(filename); + if (existingHolder != null && existingHolder.getRefreshTimestamp() > originalTimestamp) { + return existingHolder; + } return refreshProperties(filename, propHolder); } + finally { + propHolder.refreshLock.unlock(); + } } /** @@ -476,8 +524,7 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource catch (IOException ex) { // Probably a class path resource: cache it forever. if (logger.isDebugEnabled()) { - logger.debug( - resource + " could not be resolved in the file system - assuming that is hasn't changed", ex); + logger.debug(resource + " could not be resolved in the file system - assuming that it hasn't changed", ex); } fileTimestamp = -1; } @@ -561,12 +608,8 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource */ public void clearCache() { logger.debug("Clearing entire resource bundle cache"); - synchronized (this.cachedProperties) { - this.cachedProperties.clear(); - } - synchronized (this.cachedMergedProperties) { - this.cachedMergedProperties.clear(); - } + this.cachedProperties.clear(); + this.cachedMergedProperties.clear(); } /** @@ -595,30 +638,34 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource */ protected class PropertiesHolder { - private Properties properties; + private final Properties properties; - private long fileTimestamp = -1; + private final long fileTimestamp; - private long refreshTimestamp = -1; + private volatile long refreshTimestamp = -2; + + private final ReentrantLock refreshLock = new ReentrantLock(); /** Cache to hold already generated MessageFormats per message code */ - private final Map> cachedMessageFormats = - new HashMap>(); + private final ConcurrentMap> cachedMessageFormats = + new ConcurrentHashMap>(); + + public PropertiesHolder() { + this.properties = null; + this.fileTimestamp = -1; + } public PropertiesHolder(Properties properties, long fileTimestamp) { this.properties = properties; this.fileTimestamp = fileTimestamp; } - public PropertiesHolder() { - } - public Properties getProperties() { - return properties; + return this.properties; } public long getFileTimestamp() { - return fileTimestamp; + return this.fileTimestamp; } public void setRefreshTimestamp(long refreshTimestamp) { @@ -626,7 +673,7 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource } public long getRefreshTimestamp() { - return refreshTimestamp; + return this.refreshTimestamp; } public String getProperty(String code) { @@ -640,26 +687,27 @@ public class ReloadableResourceBundleMessageSource extends AbstractMessageSource if (this.properties == null) { return null; } - synchronized (this.cachedMessageFormats) { - Map localeMap = this.cachedMessageFormats.get(code); - if (localeMap != null) { - MessageFormat result = localeMap.get(locale); - if (result != null) { - return result; - } - } - String msg = this.properties.getProperty(code); - if (msg != null) { - if (localeMap == null) { - localeMap = new HashMap(); - this.cachedMessageFormats.put(code, localeMap); - } - MessageFormat result = createMessageFormat(msg, locale); - localeMap.put(locale, result); + Map localeMap = this.cachedMessageFormats.get(code); + if (localeMap != null) { + MessageFormat result = localeMap.get(locale); + if (result != null) { return result; } - return null; } + String msg = this.properties.getProperty(code); + if (msg != null) { + if (localeMap == null) { + localeMap = new ConcurrentHashMap(); + Map existing = this.cachedMessageFormats.putIfAbsent(code, localeMap); + if (existing != null) { + localeMap = existing; + } + } + MessageFormat result = createMessageFormat(msg, locale); + localeMap.put(locale, result); + return result; + } + return null; } } diff --git a/spring-context/src/test/java/org/springframework/context/support/ResourceBundleMessageSourceTests.java b/spring-context/src/test/java/org/springframework/context/support/ResourceBundleMessageSourceTests.java index 3a265516cd..5b864c3e83 100644 --- a/spring-context/src/test/java/org/springframework/context/support/ResourceBundleMessageSourceTests.java +++ b/spring-context/src/test/java/org/springframework/context/support/ResourceBundleMessageSourceTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,62 +21,68 @@ import java.util.Locale; import java.util.Properties; import java.util.ResourceBundle; -import junit.framework.TestCase; +import org.junit.After; +import org.junit.Test; import org.springframework.beans.MutablePropertyValues; import org.springframework.context.MessageSourceResolvable; import org.springframework.context.NoSuchMessageException; import org.springframework.context.i18n.LocaleContextHolder; -import org.springframework.core.JdkVersion; + +import static org.junit.Assert.*; /** * @author Juergen Hoeller * @since 03.02.2004 */ -public class ResourceBundleMessageSourceTests extends TestCase { +public class ResourceBundleMessageSourceTests { + @Test public void testMessageAccessWithDefaultMessageSource() { doTestMessageAccess(false, true, false, false, false); } + @Test public void testMessageAccessWithDefaultMessageSourceAndMessageFormat() { doTestMessageAccess(false, true, false, false, true); } + @Test public void testMessageAccessWithDefaultMessageSourceAndFallbackToGerman() { doTestMessageAccess(false, true, true, true, false); } + @Test public void testMessageAccessWithDefaultMessageSourceAndFallbackTurnedOff() { - if (JdkVersion.getMajorJavaVersion() < JdkVersion.JAVA_16) { - return; - } doTestMessageAccess(false, false, false, false, false); } + @Test public void testMessageAccessWithDefaultMessageSourceAndFallbackTurnedOffAndFallbackToGerman() { - if (JdkVersion.getMajorJavaVersion() < JdkVersion.JAVA_16) { - return; - } doTestMessageAccess(false, false, true, true, false); } + @Test public void testMessageAccessWithReloadableMessageSource() { doTestMessageAccess(true, true, false, false, false); } + @Test public void testMessageAccessWithReloadableMessageSourceAndMessageFormat() { doTestMessageAccess(true, true, false, false, true); } + @Test public void testMessageAccessWithReloadableMessageSourceAndFallbackToGerman() { doTestMessageAccess(true, true, true, true, false); } + @Test public void testMessageAccessWithReloadableMessageSourceAndFallbackTurnedOff() { doTestMessageAccess(true, false, false, false, false); } + @Test public void testMessageAccessWithReloadableMessageSourceAndFallbackTurnedOffAndFallbackToGerman() { doTestMessageAccess(true, false, true, true, false); } @@ -94,7 +100,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { MutablePropertyValues pvs = new MutablePropertyValues(); String basepath = "org/springframework/context/support/"; - String[] basenames = null; + String[] basenames; if (reloadable) { basenames = new String[] { "classpath:" + basepath + "messages", @@ -129,7 +135,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nochricht2", ac.getMessage("code2", null, new Locale("DE", "at"))); assertEquals("noochricht2", ac.getMessage("code2", null, new Locale("DE", "at", "oo"))); - if (reloadable && JdkVersion.getMajorJavaVersion() >= JdkVersion.JAVA_15) { + if (reloadable) { assertEquals("nachricht2xml", ac.getMessage("code2", null, Locale.GERMANY)); } @@ -196,6 +202,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { } } + @Test public void testDefaultApplicationContextMessageSource() { GenericApplicationContext ac = new GenericApplicationContext(); ac.refresh(); @@ -203,6 +210,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("default value", ac.getMessage("code1", new Object[] {"value"}, "default {0}", Locale.ENGLISH)); } + @Test public void testResourceBundleMessageSourceStandalone() { ResourceBundleMessageSource ms = new ResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -210,6 +218,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test public void testResourceBundleMessageSourceWithWhitespaceInBasename() { ResourceBundleMessageSource ms = new ResourceBundleMessageSource(); ms.setBasename(" org/springframework/context/support/messages "); @@ -217,6 +226,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test public void testResourceBundleMessageSourceWithDefaultCharset() { ResourceBundleMessageSource ms = new ResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -225,10 +235,8 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test public void testResourceBundleMessageSourceWithInappropriateDefaultCharset() { - if (JdkVersion.getMajorJavaVersion() < JdkVersion.JAVA_16) { - return; - } ResourceBundleMessageSource ms = new ResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); ms.setDefaultEncoding("argh"); @@ -242,6 +250,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { } } + @Test public void testReloadableResourceBundleMessageSourceStandalone() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -249,6 +258,36 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test + public void testReloadableResourceBundleMessageSourceWithCacheSeconds() throws InterruptedException { + ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); + ms.setBasename("org/springframework/context/support/messages"); + ms.setCacheSeconds(1); + // Initial cache attempt + assertEquals("message1", ms.getMessage("code1", null, Locale.ENGLISH)); + assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); + Thread.sleep(1100); + // Late enough for a re-cache attempt + assertEquals("message1", ms.getMessage("code1", null, Locale.ENGLISH)); + assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); + } + + @Test + public void testReloadableResourceBundleMessageSourceWithNonConcurrentRefresh() throws InterruptedException { + ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); + ms.setBasename("org/springframework/context/support/messages"); + ms.setCacheSeconds(1); + ms.setConcurrentRefresh(false); + // Initial cache attempt + assertEquals("message1", ms.getMessage("code1", null, Locale.ENGLISH)); + assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); + Thread.sleep(1100); + // Late enough for a re-cache attempt + assertEquals("message1", ms.getMessage("code1", null, Locale.ENGLISH)); + assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); + } + + @Test public void testReloadableResourceBundleMessageSourceWithCommonMessages() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); Properties commonMessages = new Properties(); @@ -261,6 +300,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("Do not do that", ms.getMessage("warning", new Object[] {"that"}, Locale.GERMAN)); } + @Test public void testReloadableResourceBundleMessageSourceWithWhitespaceInBasename() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); ms.setBasename(" org/springframework/context/support/messages "); @@ -268,6 +308,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test public void testReloadableResourceBundleMessageSourceWithDefaultCharset() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -276,6 +317,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("nachricht2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test public void testReloadableResourceBundleMessageSourceWithInappropriateDefaultCharset() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -293,6 +335,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { } } + @Test public void testReloadableResourceBundleMessageSourceWithInappropriateEnglishCharset() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -309,6 +352,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { } } + @Test public void testReloadableResourceBundleMessageSourceWithInappropriateGermanCharset() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -320,6 +364,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals("message2", ms.getMessage("code2", null, Locale.GERMAN)); } + @Test public void testReloadableResourceBundleMessageSourceFileNameCalculation() { ReloadableResourceBundleMessageSource ms = new ReloadableResourceBundleMessageSource(); @@ -352,6 +397,7 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertEquals(0, filenames.size()); } + @Test public void testMessageSourceResourceBundle() { ResourceBundleMessageSource ms = new ResourceBundleMessageSource(); ms.setBasename("org/springframework/context/support/messages"); @@ -363,11 +409,10 @@ public class ResourceBundleMessageSourceTests extends TestCase { assertTrue(rbg.containsKey("code2")); } - @Override - protected void tearDown() throws Exception { - if (JdkVersion.getMajorJavaVersion() >= JdkVersion.JAVA_16) { - ResourceBundle.clearCache(); - } + + @After + public void tearDown() { + ResourceBundle.clearCache(); } }