Updated Branches: refs/heads/master 8fb770773 -> a4f964d47
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/a4f964d4 Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/a4f964d4 Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/a4f964d4 Branch: refs/heads/master Commit: a4f964d47a84e4efe404edbce58a8872c2d7ab65 Parents: 8fb7707 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:57:05 2012 +0300 ---------------------------------------------------------------------- .../wicket/core/util/lang/PropertyResolver.java | 10 ++++-- .../apache/wicket/feedback/FeedbackMessages.java | 17 ++++++--- .../request/resource/PackageResourceReference.java | 6 +++- .../apache/wicket/session/DefaultPageFactory.java | 29 ++++++++------- .../wicket/versioning/InMemoryPageStore.java | 9 +++-- .../annot/AnnotProxyFieldValueFactory.java | 14 ++++++-- .../converter/AbstractIntegerConverter.java | 9 +++-- .../wicket/util/watch/ModificationWatcher.java | 7 ++-- 8 files changed, 64 insertions(+), 37 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/wicket/blob/a4f964d4/wicket-core/src/main/java/org/apache/wicket/core/util/lang/PropertyResolver.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/core/util/lang/PropertyResolver.java b/wicket-core/src/main/java/org/apache/wicket/core/util/lang/PropertyResolver.java index 01efa84..6212a74 100644 --- a/wicket-core/src/main/java/org/apache/wicket/core/util/lang/PropertyResolver.java +++ b/wicket-core/src/main/java/org/apache/wicket/core/util/lang/PropertyResolver.java @@ -71,7 +71,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"; @@ -1441,7 +1441,7 @@ public final class PropertyResolver private static IClassCache getClassesToGetAndSetters() { - Object key = null; + Object key; if (Application.exists()) { key = Application.get(); @@ -1453,7 +1453,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/a4f964d4/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 fcbcf9c..55432b5 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 @@ -66,11 +66,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); } /** @@ -195,9 +196,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/a4f964d4/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 ea3f92e..ea2ddb6 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 @@ -259,7 +259,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/a4f964d4/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 0d4c7b8..cc8ac3a 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.ConcurrentMap; import org.apache.wicket.IPageFactory; @@ -48,7 +47,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 @@ -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; } @@ -258,7 +255,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/a4f964d4/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 d73e7fb..ca1399a 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. @@ -99,7 +98,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/a4f964d4/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 4d0a958..3d219da 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 @@ -127,7 +127,7 @@ public class AnnotProxyFieldValueFactory implements IFieldValueFactory return cachedValue; } - final Object target; + Object target; if (wrapInProxies) { target = LazyInitProxyFactory.createProxy(field.getType(), locator); @@ -140,7 +140,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; } @@ -176,7 +180,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/a4f964d4/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/a4f964d4/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 7af1621..8af963d 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; @@ -44,7 +43,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 = Generics.newConcurrentHashMap(); + private final ConcurrentHashMap<IModifiable, Entry> modifiableToEntry = Generics.newConcurrentHashMap(); /** the <code>Task</code> to run */ private Task task; @@ -102,12 +101,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;
