Updated Branches: refs/heads/wicket-1.5.x 29844aa3f -> 0fdae2e8e
WICKET-4646 atomicity violation bugs of using concurrent collections Project: http://git-wip-us.apache.org/repos/asf/wicket/repo Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/0fdae2e8 Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/0fdae2e8 Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/0fdae2e8 Branch: refs/heads/wicket-1.5.x Commit: 0fdae2e8eb3c8716409e4c69e22deea902c3f6fd Parents: 29844aa Author: Martin Tzvetanov Grigorov <[email protected]> Authored: Mon Jul 23 13:53:48 2012 +0300 Committer: Martin Tzvetanov Grigorov <[email protected]> Committed: Mon Jul 23 13:53:48 2012 +0300 ---------------------------------------------------------------------- .../apache/wicket/feedback/FeedbackMessages.java | 17 +++++--- .../request/resource/PackageResourceReference.java | 6 ++- .../apache/wicket/session/DefaultPageFactory.java | 33 ++++++++------- .../apache/wicket/util/lang/PropertyResolver.java | 11 +++-- .../wicket/versioning/InMemoryPageStore.java | 9 +++- .../annot/AnnotProxyFieldValueFactory.java | 14 +++++- .../converter/AbstractIntegerConverter.java | 9 +++- .../wicket/util/watch/ModificationWatcher.java | 7 +-- 8 files changed, 66 insertions(+), 40 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-core/src/main/java/org/apache/wicket/feedback/FeedbackMessages.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/feedback/FeedbackMessages.java b/wicket-core/src/main/java/org/apache/wicket/feedback/FeedbackMessages.java index 5385043..b33c967 100644 --- a/wicket-core/src/main/java/org/apache/wicket/feedback/FeedbackMessages.java +++ b/wicket-core/src/main/java/org/apache/wicket/feedback/FeedbackMessages.java @@ -68,11 +68,12 @@ public final class FeedbackMessages implements IClusterable, Iterable<FeedbackMe */ public final void add(FeedbackMessage message) { - if (log.isDebugEnabled()) + log.debug("Adding feedback message '{}'", message); + + synchronized (messages) { - log.debug("Adding feedback message " + message); + messages.add(message); } - messages.add(message); } /** @@ -197,9 +198,13 @@ public final class FeedbackMessages implements IClusterable, Iterable<FeedbackMe message.detach(); } - messages.removeAll(toDelete); - - return toDelete.size(); + synchronized(messages) + { + int sizeBefore = messages.size(); + messages.removeAll(toDelete); + int sizeAfter = messages.size(); + return sizeAfter - sizeBefore; + } } /** http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResourceReference.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResourceReference.java b/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResourceReference.java index 4d21c94..d422082 100644 --- a/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResourceReference.java +++ b/wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResourceReference.java @@ -153,7 +153,11 @@ public class PackageResourceReference extends ResourceReference if (value == null) { value = getUrlAttributes(locale, style, variation); - urlAttributesCacheMap.put(key, value); + UrlAttributes tmpValue = urlAttributesCacheMap.putIfAbsent(key, value); + if (tmpValue != null) + { + value = tmpValue; + } } return value; http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-core/src/main/java/org/apache/wicket/session/DefaultPageFactory.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/session/DefaultPageFactory.java b/wicket-core/src/main/java/org/apache/wicket/session/DefaultPageFactory.java index 572fc4c..224ea86 100644 --- a/wicket-core/src/main/java/org/apache/wicket/session/DefaultPageFactory.java +++ b/wicket-core/src/main/java/org/apache/wicket/session/DefaultPageFactory.java @@ -18,7 +18,6 @@ package org.apache.wicket.session; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; -import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -50,7 +49,7 @@ public final class DefaultPageFactory implements IPageFactory private static final Logger log = LoggerFactory.getLogger(DefaultPageFactory.class); /** Map of Constructors for Page subclasses */ - private final Map<Class<?>, Constructor<?>> constructorForClass = Generics.newConcurrentHashMap(); + private final ConcurrentMap<Class<?>, Constructor<?>> constructorForClass = Generics.newConcurrentHashMap(); /** * {@link #isBookmarkable(Class)} is expensive, we cache the result here @@ -113,7 +112,7 @@ public final class DefaultPageFactory implements IPageFactory * @return The page constructor, or null if no one-arg constructor can be found taking the given * argument type. */ - private final <C extends IRequestablePage> Constructor<?> constructor(final Class<C> pageClass, + private <C extends IRequestablePage> Constructor<?> constructor(final Class<C> pageClass, final Class<PageParameters> argumentType) { // Get constructor for page class from cache @@ -128,22 +127,20 @@ public final class DefaultPageFactory implements IPageFactory constructor = pageClass.getConstructor(new Class[] { argumentType }); // Store it in the cache - constructorForClass.put(pageClass, constructor); - - if (log.isDebugEnabled()) + Constructor<?> tmpConstructor = constructorForClass.putIfAbsent(pageClass, constructor); + if (tmpConstructor != null) { - log.debug("Found constructor for Page of type '{}' and argument of type '{}'.", - pageClass, argumentType); + constructor = tmpConstructor; } + + log.debug("Found constructor for Page of type '{}' and argument of type '{}'.", + pageClass, argumentType); } catch (NoSuchMethodException e) { - if (log.isDebugEnabled()) - { - log.debug( - "Page of type '{}' has not visible constructor with an argument of type '{}'.", - pageClass, argumentType); - } + log.debug( + "Page of type '{}' has not visible constructor with an argument of type '{}'.", + pageClass, argumentType); return null; } @@ -164,7 +161,7 @@ public final class DefaultPageFactory implements IPageFactory * Thrown if the Page cannot be instantiated using the given constructor and * argument. */ - private final Page newPage(final Constructor<?> constructor, final Object argument) + private Page newPage(final Constructor<?> constructor, final Object argument) { try { @@ -257,7 +254,11 @@ public final class DefaultPageFactory implements IPageFactory { bookmarkable = Boolean.FALSE; } - pageToBookmarkableCache.put(pageClass.getName(), bookmarkable); + Boolean tmpBookmarkable = pageToBookmarkableCache.putIfAbsent(pageClass.getName(), bookmarkable); + if (tmpBookmarkable != null) + { + bookmarkable = tmpBookmarkable; + } } return bookmarkable; http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-core/src/main/java/org/apache/wicket/util/lang/PropertyResolver.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/util/lang/PropertyResolver.java b/wicket-core/src/main/java/org/apache/wicket/util/lang/PropertyResolver.java index a6471f4..893e1d4c 100644 --- a/wicket-core/src/main/java/org/apache/wicket/util/lang/PropertyResolver.java +++ b/wicket-core/src/main/java/org/apache/wicket/util/lang/PropertyResolver.java @@ -28,7 +28,6 @@ import org.apache.wicket.Application; import org.apache.wicket.Session; import org.apache.wicket.WicketRuntimeException; import org.apache.wicket.util.convert.ConversionException; -import org.apache.wicket.util.lang.PropertyResolver.IClassCache; import org.apache.wicket.util.string.Strings; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -71,7 +70,7 @@ public final class PropertyResolver private final static int CREATE_NEW_VALUE = 1; private final static int RESOLVE_CLASS = 2; - private final static Map<Object, IClassCache> applicationToClassesToGetAndSetters = Generics.newConcurrentHashMap(2); + private final static ConcurrentHashMap<Object, IClassCache> applicationToClassesToGetAndSetters = Generics.newConcurrentHashMap(2); private static final String GET = "get"; private static final String IS = "is"; @@ -1416,7 +1415,7 @@ public final class PropertyResolver private static IClassCache getClassesToGetAndSetters() { - Object key = null; + Object key; if (Application.exists()) { key = Application.get(); @@ -1428,7 +1427,11 @@ public final class PropertyResolver IClassCache result = applicationToClassesToGetAndSetters.get(key); if (result == null) { - applicationToClassesToGetAndSetters.put(key, result = new DefaultClassCache()); + IClassCache tmpResult = applicationToClassesToGetAndSetters.putIfAbsent(key, result = new DefaultClassCache()); + if (tmpResult != null) + { + result = tmpResult; + } } return result; } http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-core/src/test/java/org/apache/wicket/versioning/InMemoryPageStore.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/test/java/org/apache/wicket/versioning/InMemoryPageStore.java b/wicket-core/src/test/java/org/apache/wicket/versioning/InMemoryPageStore.java index 0087666..c6a5642 100644 --- a/wicket-core/src/test/java/org/apache/wicket/versioning/InMemoryPageStore.java +++ b/wicket-core/src/test/java/org/apache/wicket/versioning/InMemoryPageStore.java @@ -16,7 +16,6 @@ */ package org.apache.wicket.versioning; -import java.util.HashMap; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -34,7 +33,7 @@ public class InMemoryPageStore implements IDataStore /** * A map of : sessionId => pageId => pageAsBytes */ - private final Map<String, Map<Integer, byte[]>> store; + private final ConcurrentHashMap<String, Map<Integer, byte[]>> store; /** * Construct. @@ -94,7 +93,11 @@ public class InMemoryPageStore implements IDataStore if (sessionPages == null) { sessionPages = new ConcurrentHashMap<Integer, byte[]>(); - store.put(sessionId, sessionPages); + Map<Integer, byte[]> tmpSessionPages = store.putIfAbsent(sessionId, sessionPages); + if (tmpSessionPages != null) + { + sessionPages = tmpSessionPages; + } } sessionPages.put(pageId, pageAsBytes); http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-spring/src/main/java/org/apache/wicket/spring/injection/annot/AnnotProxyFieldValueFactory.java ---------------------------------------------------------------------- diff --git a/wicket-spring/src/main/java/org/apache/wicket/spring/injection/annot/AnnotProxyFieldValueFactory.java b/wicket-spring/src/main/java/org/apache/wicket/spring/injection/annot/AnnotProxyFieldValueFactory.java index 9d37847..8abbfd6 100644 --- a/wicket-spring/src/main/java/org/apache/wicket/spring/injection/annot/AnnotProxyFieldValueFactory.java +++ b/wicket-spring/src/main/java/org/apache/wicket/spring/injection/annot/AnnotProxyFieldValueFactory.java @@ -126,7 +126,7 @@ public class AnnotProxyFieldValueFactory implements IFieldValueFactory return cachedValue; } - final Object target; + Object target; if (wrapInProxies) { target = LazyInitProxyFactory.createProxy(field.getType(), locator); @@ -139,7 +139,11 @@ public class AnnotProxyFieldValueFactory implements IFieldValueFactory // only put the proxy into the cache if the bean is a singleton if (locator.isSingletonBean()) { - cache.put(locator, target); + Object tmpTarget = cache.putIfAbsent(locator, target); + if (tmpTarget != null) + { + target = tmpTarget; + } } return target; } @@ -165,7 +169,11 @@ public class AnnotProxyFieldValueFactory implements IFieldValueFactory if (name != null) { - beanNameCache.put(field.getType(), name); + String tmpName = beanNameCache.putIfAbsent(field.getType(), name); + if (tmpName != null) + { + name = tmpName; + } } } } http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-util/src/main/java/org/apache/wicket/util/convert/converter/AbstractIntegerConverter.java ---------------------------------------------------------------------- diff --git a/wicket-util/src/main/java/org/apache/wicket/util/convert/converter/AbstractIntegerConverter.java b/wicket-util/src/main/java/org/apache/wicket/util/convert/converter/AbstractIntegerConverter.java index 6998fde..5b3d864 100644 --- a/wicket-util/src/main/java/org/apache/wicket/util/convert/converter/AbstractIntegerConverter.java +++ b/wicket-util/src/main/java/org/apache/wicket/util/convert/converter/AbstractIntegerConverter.java @@ -18,7 +18,6 @@ package org.apache.wicket.util.convert.converter; import java.text.NumberFormat; import java.util.Locale; -import java.util.Map; import java.util.concurrent.ConcurrentHashMap; /** @@ -32,7 +31,7 @@ public abstract class AbstractIntegerConverter<I extends Number> extends Abstrac private static final long serialVersionUID = 1L; /** The date format to use */ - private final Map<Locale, NumberFormat> numberFormats = new ConcurrentHashMap<Locale, NumberFormat>(); + private final ConcurrentHashMap<Locale, NumberFormat> numberFormats = new ConcurrentHashMap<Locale, NumberFormat>(); /** * @param locale @@ -48,7 +47,11 @@ public abstract class AbstractIntegerConverter<I extends Number> extends Abstrac numberFormat = NumberFormat.getIntegerInstance(locale); numberFormat.setParseIntegerOnly(true); numberFormat.setGroupingUsed(false); - numberFormats.put(locale, numberFormat); + NumberFormat tmpNumberFormat = numberFormats.putIfAbsent(locale, numberFormat); + if (tmpNumberFormat != null) + { + numberFormat = tmpNumberFormat; + } } return (NumberFormat)numberFormat.clone(); } http://git-wip-us.apache.org/repos/asf/wicket/blob/0fdae2e8/wicket-util/src/main/java/org/apache/wicket/util/watch/ModificationWatcher.java ---------------------------------------------------------------------- diff --git a/wicket-util/src/main/java/org/apache/wicket/util/watch/ModificationWatcher.java b/wicket-util/src/main/java/org/apache/wicket/util/watch/ModificationWatcher.java index 8abe543..8662eaa 100644 --- a/wicket-util/src/main/java/org/apache/wicket/util/watch/ModificationWatcher.java +++ b/wicket-util/src/main/java/org/apache/wicket/util/watch/ModificationWatcher.java @@ -16,7 +16,6 @@ */ package org.apache.wicket.util.watch; -import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -43,7 +42,7 @@ public class ModificationWatcher implements IModificationWatcher private static final Logger log = LoggerFactory.getLogger(ModificationWatcher.class); /** maps <code>IModifiable</code> objects to <code>Entry</code> objects */ - private final Map<IModifiable, Entry> modifiableToEntry = new ConcurrentHashMap<IModifiable, Entry>(); + private final ConcurrentHashMap<IModifiable, Entry> modifiableToEntry = new ConcurrentHashMap<IModifiable, Entry>(); /** the <code>Task</code> to run */ private Task task; @@ -104,12 +103,12 @@ public class ModificationWatcher implements IModificationWatcher newEntry.listeners.add(listener); // Put in map - modifiableToEntry.put(modifiable, newEntry); + modifiableToEntry.putIfAbsent(modifiable, newEntry); } else { // The IModifiable is not returning a valid lastModifiedTime - log.info("Cannot track modifications to resource " + modifiable); + log.info("Cannot track modifications to resource '{}'", modifiable); } return true;
