Adam, all,
I am resurrecting this part of the thread: is it ok if we fix the 10.04 with
the patch below (Adam's commit with rev. 948333 that adds the putIfAbsent
method and then the same fix I committed to 11.04, 12.04 and trunk)?
Thanks,
Jacopo
On May 12, 2012, at 9:17 AM, Jacopo Cappellato wrote:
> As regards the 10.04, what if we backport the putIfAbsent method there and
> then apply a similar fix? Here is the patch I would like to backport:
>
> Index: framework/base/src/org/ofbiz/base/util/cache/test/UtilCacheTests.java
> ===================================================================
> --- framework/base/src/org/ofbiz/base/util/cache/test/UtilCacheTests.java
> (revision 1337449)
> +++ framework/base/src/org/ofbiz/base/util/cache/test/UtilCacheTests.java
> (working copy)
> @@ -323,6 +323,19 @@
> basicTest(cache);
> }
>
> + public void testPutIfAbsent() throws Exception {
> + UtilCache<String, String> cache = createUtilCache(5, 5, 2000, false,
> false);
> + Listener<String, String> gotListener = createListener(cache);
> + Listener<String, String> wantedListener = new Listener<String,
> String>();
> + wantedListener.noteKeyAddition(cache, "two", "dos");
> + assertNull("putIfAbsent", cache.putIfAbsent("two", "dos"));
> + assertHasSingleKey(cache, "two", "dos");
> + assertEquals("putIfAbsent", "dos", cache.putIfAbsent("two",
> "double"));
> + assertHasSingleKey(cache, "two", "dos");
> + cache.removeListener(gotListener);
> + assertEquals("listener", wantedListener, gotListener);
> + }
> +
> public void testChangeMemSize() throws Exception {
> int size = 5;
> long ttl = 2000;
> Index: framework/base/src/org/ofbiz/base/util/cache/UtilCache.java
> ===================================================================
> --- framework/base/src/org/ofbiz/base/util/cache/UtilCache.java
> (revision 1337449)
> +++ framework/base/src/org/ofbiz/base/util/cache/UtilCache.java
> (working copy)
> @@ -289,6 +289,10 @@
> return putInternal(key, value, expireTimeNanos);
> }
>
> + public V putIfAbsent(K key, V value) {
> + return putIfAbsentInternal(key, value, expireTimeNanos);
> + }
> +
> CacheLine<V> createSoftRefCacheLine(final Object key, V value, long
> loadTimeNanos, long expireTimeNanos) {
> return tryRegister(loadTimeNanos, new SoftRefCacheLine<V>(value,
> loadTimeNanos, expireTimeNanos) {
> @Override
> @@ -366,6 +370,10 @@
> return putInternal(key, value,
> TimeUnit.NANOSECONDS.convert(expireTimeMillis, TimeUnit.MILLISECONDS));
> }
>
> + public V putIfAbsent(K key, V value, long expireTimeMillis) {
> + return putIfAbsentInternal(key, value,
> TimeUnit.NANOSECONDS.convert(expireTimeMillis, TimeUnit.MILLISECONDS));
> + }
> +
> V putInternal(K key, V value, long expireTimeNanos) {
> Object nulledKey = fromKey(key);
> CacheLine<V> oldCacheLine = memoryTable.put(nulledKey,
> createCacheLine(key, value, expireTimeNanos));
> @@ -388,6 +396,41 @@
> }
> }
>
> + V putIfAbsentInternal(K key, V value, long expireTimeNanos) {
> + Object nulledKey = fromKey(key);
> + V oldValue;
> + if (fileTable != null) {
> + try {
> + synchronized (this) {
> + oldValue = fileTable.get(nulledKey);
> + if (oldValue == null) {
> + memoryTable.put(nulledKey, createCacheLine(key,
> value, expireTimeNanos));
> + fileTable.put(nulledKey, value);
> + jdbmMgr.commit();
> + }
> + }
> + } catch (IOException e) {
> + Debug.logError(e, module);
> + oldValue = null;
> + }
> + } else {
> + CacheLine<V> newCacheLine = createCacheLine(key, value,
> expireTimeNanos);
> + CacheLine<V> oldCacheLine = memoryTable.putIfAbsent(nulledKey,
> newCacheLine);
> + if (oldCacheLine == null) {
> + oldValue = null;
> + } else {
> + oldValue = oldCacheLine.getValue();
> + cancel(newCacheLine);
> + }
> + }
> + if (oldValue == null) {
> + noteAddition(key, value);
> + return null;
> + } else {
> + return oldValue;
> + }
> + }
> +
> /** Gets an element from the cache according to the specified key.
> * @param key The key for the element, used to reference it in the
> hastables and LRU linked list
> * @return The value of the element specified by the key
> Index: framework/base/src/org/ofbiz/base/util/GroovyUtil.java
> ===================================================================
> --- framework/base/src/org/ofbiz/base/util/GroovyUtil.java (revision
> 1337449)
> +++ framework/base/src/org/ofbiz/base/util/GroovyUtil.java (working copy)
> @@ -102,26 +102,20 @@
>
> public static Class<?> getScriptClassFromLocation(String location) throws
> GeneralException {
> try {
> - Class<?> scriptClass = null;
> - synchronized (parsedScripts) {
> - scriptClass = parsedScripts.get(location);
> - }
> + Class<?> scriptClass = parsedScripts.get(location);
> if (scriptClass == null) {
> URL scriptUrl = FlexibleLocation.resolveLocation(location);
> if (scriptUrl == null) {
> throw new GeneralException("Script not found at location
> [" + location + "]");
> }
> scriptClass = parseClass(scriptUrl.openStream(), location);
> - synchronized (parsedScripts) {
> - Class<?> scriptClassCached = parsedScripts.get(location);
> - if (scriptClassCached == null) {
> - if (Debug.verboseOn()) {
> - Debug.logVerbose("Caching Groovy script at: " +
> location, module);
> - }
> - parsedScripts.put(location, scriptClass);
> - } else {
> - scriptClass = scriptClassCached;
> + Class<?> scriptClassCached =
> parsedScripts.putIfAbsent(location, scriptClass);
> + if (scriptClassCached == null) { // putIfAbsent returns null
> if the class is added
> + if (Debug.verboseOn()) {
> + Debug.logVerbose("Cached Groovy script at: " +
> location, module);
> }
> + } else {
> + scriptClass = scriptClassCached;
> }
> }
> return scriptClass;
> @@ -152,22 +146,16 @@
>
> public static Object runScriptFromClasspath(String script,
> Map<String,Object> context) throws GeneralException {
> try {
> - Class<?> scriptClass = null;
> - synchronized (parsedScripts) {
> - parsedScripts.get(script);
> - }
> + Class<?> scriptClass = parsedScripts.get(script);
> if (scriptClass == null) {
> scriptClass = loadClass(script);
> - synchronized (parsedScripts) {
> - Class<?> scriptClassCached = parsedScripts.get(script);
> - if (scriptClassCached == null) {
> - if (Debug.verboseOn()) {
> - Debug.logVerbose("Caching Groovy script: " +
> script, module);
> - }
> - parsedScripts.put(script, scriptClass);
> - } else {
> - scriptClass = scriptClassCached;
> + Class<?> cachedScriptClass =
> parsedScripts.putIfAbsent(script, scriptClass);
> + if (cachedScriptClass == null) { // putIfAbsent returns null
> if the class is added
> + if (Debug.verboseOn()) {
> + Debug.logVerbose("Cached Groovy script at: " +
> script, module);
> }
> + } else {
> + scriptClass = cachedScriptClass;
> }
> }
> return InvokerHelper.createScript(scriptClass,
> getBinding(context)).run();
