Attachment 271946 Details for Bug 522635 – Fix for master and 2.7

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

[patch] Fix for master and 2.7

0002-Bug-522635-ConcurrentModificationException-when-trig.patch (text/plain), 39.97 KB, created by Tomas Kraus

on 2017-12-18 10:45:15 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Tomas Kraus

Created: 2017-12-18 10:45:15 EST

Size: 39.97 KB

patch

obsolete

>From 0d06164409eeb7f3b55ee85343a58ee81e4ce998 Mon Sep 17 00:00:00 2001
>From: Tomas Kraus <tomas.kraus@oracle.com>
>Date: Thu, 14 Dec 2017 12:21:33 +0100
>Subject: [PATCH 2/2] Bug 522635 - ConcurrentModificationException when
> triggering lazy load from conforming query
>
>Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
>Reviewed-by: Lukas Jungmann <lukas.jungmann@oracle.com>
>---
> .../identitymaps/GetAllFromIdentityMapTest.java    |  86 ++++++++++++++++
> .../tests/identitymaps/IdentityMapTestSuite.java   |   4 +-
> ...gerValueHoldersSelfReferencingOneToOneTest.java |  67 ++++++-------
> .../AbstractIdentityMapEnumeration.java            | 108 +++++++++++++++++++++
> .../internal/identitymaps/FullIdentityMap.java     |  38 ++++++--
> .../internal/identitymaps/IdentityMap.java         |  18 +++-
> .../identitymaps/IdentityMapEnumeration.java       |  32 ++++--
> .../identitymaps/IdentityMapKeyEnumeration.java    |  80 +++++++--------
> .../internal/identitymaps/IdentityMapManager.java  |  32 +++---
> .../internal/identitymaps/NoIdentityMap.java       |  24 +++--
> .../sessions/interceptors/CacheInterceptor.java    |  35 +++++--
> 11 files changed, 388 insertions(+), 136 deletions(-)
> create mode 100644 foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/GetAllFromIdentityMapTest.java
> create mode 100644 foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/AbstractIdentityMapEnumeration.java
>
>diff --git a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/GetAllFromIdentityMapTest.java b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/GetAllFromIdentityMapTest.java
>new file mode 100644
>index 0000000..30fd736
>--- /dev/null
>+++ b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/GetAllFromIdentityMapTest.java
>@@ -0,0 +1,86 @@
>+/*******************************************************************************
>+ * Copyright (c) 2017 Oracle and/or its affiliates. All rights reserved.
>+ * This program and the accompanying materials are made available under the
>+ * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>+ * which accompanies this distribution.
>+ * The Eclipse Public License is available at http://www.eclipse.org/legal/epl-v10.html
>+ * and the Eclipse Distribution License is available at
>+ * http://www.eclipse.org/org/documents/edl-v10.php.
>+ *
>+ * Contributors:
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>+ ******************************************************************************/
>+package org.eclipse.persistence.testing.tests.identitymaps;
>+
>+import java.util.Vector;
>+
>+import org.eclipse.persistence.expressions.ExpressionBuilder;
>+import org.eclipse.persistence.queries.InMemoryQueryIndirectionPolicy;
>+import org.eclipse.persistence.queries.ReadAllQuery;
>+import org.eclipse.persistence.sessions.UnitOfWork;
>+import org.eclipse.persistence.testing.framework.TestCase;
>+import org.eclipse.persistence.testing.models.employee.domain.Employee;
>+
>+/**
>+ * Bug# 522635 - Verify that {@code ConcurrentModificationException} is not thrown from {@code IdentityMapManager}
>+ * when {@code getAllFromIdentityMap} method is being called twice.
>+ */
>+public class GetAllFromIdentityMapTest extends TestCase {
>+
>+    /** Current transaction. */
>+    private UnitOfWork uow = null;
>+
>+    /** Query instance. */
>+    private ReadAllQuery query;
>+
>+    /**
>+     * Create an instance of {@link ReadAllQuery} and initialize it.
>+     *
>+     * @param c entity class
>+     * @return new instance of initialized {@link ReadAllQuery}
>+     */
>+    private static ReadAllQuery newReadAllQuery(Class<?> c) {
>+        final ReadAllQuery query = new ReadAllQuery(c);
>+        query.conformResultsInUnitOfWork();
>+        query.setInMemoryQueryIndirectionPolicy(new InMemoryQueryIndirectionPolicy(InMemoryQueryIndirectionPolicy.SHOULD_TRIGGER_INDIRECTION));
>+        return query;
>+    }
>+
>+    /**
>+     * Test setup.
>+     * Open transaction, initialize query and initialize cache with 1st query execution.
>+     */
>+    public void setup() {
>+        getSession().getIdentityMapAccessor().initializeAllIdentityMaps();
>+        uow = getSession().acquireUnitOfWork();
>+        query = newReadAllQuery(Employee.class);
>+        ExpressionBuilder emp = new ExpressionBuilder();
>+        query.setSelectionCriteria(emp.get("manager").get("firstName").equal("Bob"));
>+        uow.executeQuery(query);
>+    }
>+
>+    /**
>+     * Test cleanup.
>+     * Release transaction and reset cache.
>+     */
>+    public void reset() {
>+        uow.release();
>+        uow = null;
>+        getSession().getIdentityMapAccessor().initializeAllIdentityMaps();
>+    }
>+
>+    /**
>+     * Test case.
>+     * Verify that {@code ConcurrentModificationException} is not thrown from {@code IdentityMapManager}
>+     * during 2nd query execution when objects are already in the cache.
>+     */
>+    public void test() throws Throwable {
>+        Vector<Employee> employees = (Vector<Employee>)uow.executeQuery(query);
>+        assertFalse(employees.isEmpty());
>+        for (Employee employee : employees) {
>+            assertEquals("Bob", employee.getManager().getFirstName());
>+        }
>+    }
>+
>+}
>diff --git a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/IdentityMapTestSuite.java b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/IdentityMapTestSuite.java
>index 1ae0bf6..575e27d 100644
>--- a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/IdentityMapTestSuite.java
>+++ b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/IdentityMapTestSuite.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -66,6 +66,8 @@ public void addTests() {
>         // Bug 5840635
>         addTest(new CleanupCacheKeyCorrectnessTest());
>         addTest(new TriggerValueHoldersSelfReferencingOneToOneTest());
>+        // Bug 522635
>+        addTest(new GetAllFromIdentityMapTest());
>     }
> 
>     private TestSuite getCacheIdentityMapSuite() {
>diff --git a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/TriggerValueHoldersSelfReferencingOneToOneTest.java b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/TriggerValueHoldersSelfReferencingOneToOneTest.java
>index 04a1b22..d995a9c 100644
>--- a/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/TriggerValueHoldersSelfReferencingOneToOneTest.java
>+++ b/foundation/eclipselink.core.test/src/org/eclipse/persistence/testing/tests/identitymaps/TriggerValueHoldersSelfReferencingOneToOneTest.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 2011, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 2010, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -8,48 +8,58 @@
>  * http://www.eclipse.org/org/documents/edl-v10.php.
>  *
>  * Contributors:
>- *     Oracle - initial API and implementation from Oracle TopLink
>- *     Mark Wolochuk - Bug 321041 ConcurrentModificationException on getFromIdentityMap() fix
>+ *     08/17/2010-2.2 Tom Ware
>+ *       - 321041 ConcurrentModificationException on getFromIdentityMap() fix
>  ******************************************************************************/
> package org.eclipse.persistence.testing.tests.identitymaps;
> 
>-import java.util.ConcurrentModificationException;
>-
> import org.eclipse.persistence.expressions.Expression;
> import org.eclipse.persistence.expressions.ExpressionBuilder;
> import org.eclipse.persistence.queries.ReadObjectQuery;
> import org.eclipse.persistence.sessions.UnitOfWork;
>-import org.eclipse.persistence.testing.framework.AutoVerifyTestCase;
>-import org.eclipse.persistence.testing.framework.TestErrorException;
>+import org.eclipse.persistence.testing.framework.TestCase;
> import org.eclipse.persistence.testing.models.employee.domain.Employee;
> 
> /**
>- * Test for bug fix 321041 - EclipseLink throws ConcurrentModificationException when triggering lazy load from conforming query
>- * @author tware
>- *
>+ * Bug# 321041 - EclipseLink throws ConcurrentModificationException when triggering lazy load from conforming query
>  */
>-public class TriggerValueHoldersSelfReferencingOneToOneTest extends AutoVerifyTestCase{
>+public class TriggerValueHoldersSelfReferencingOneToOneTest extends TestCase {
> 
>+    /** Current transaction. */
>     protected UnitOfWork uow = null;
>-    protected ConcurrentModificationException exception = null;
> 
>-    public void setup(){
>+    /**
>+     * Test setup.
>+     * Open transaction, initialize query and initialize cache with 1st query executions.
>+     */
>+    public void setup() {
>         getSession().getIdentityMapAccessor().initializeAllIdentityMaps();
>         uow = getSession().acquireUnitOfWork();
>-
>-
>-        // preload the UnitOfWork identity map with 2 Employees which have different managers
>+        // Preload the UnitOfWork identity map with 2 Employees which have different managers
>         ExpressionBuilder emp = new ExpressionBuilder();
>         Expression queryExp = emp.get("firstName").equal("Charles").and(emp.get("lastName").equal("Chanley"));
>         uow.readObject(Employee.class, queryExp);
>-
>         emp = new ExpressionBuilder();
>         queryExp = emp.get("firstName").equal("Marcus").and(emp.get("lastName").equal("Saunders"));
>         uow.readObject(Employee.class, queryExp);
>     }
> 
>-    public void test(){
>+    /**
>+     * Test cleanup.
>+     * Release transaction and reset cache.
>+     */
>+    public void reset() {
>+        uow.release();
>+        uow = null;
>+        getSession().getIdentityMapAccessor().initializeAllIdentityMaps();
>+    }
> 
>+    /**
>+     * Test case.
>+     * Verify that {@code ConcurrentModificationException} is not thrown from {@code IdentityMapManager}
>+     * during 2nd queries execution when objects are already in the cache.
>+     */
>+    public void test() {
>         // We query for both Employees here because it is impossible to tell which order
>         // keys will be returned from the identity map in
>         // This bug only occurs when the first key returned is non-conforming and
>@@ -59,26 +69,17 @@ public void test(){
>         ReadObjectQuery query = new ReadObjectQuery(Employee.class, queryExp);
>         query.conformResultsInUnitOfWork();
>         query.getInMemoryQueryIndirectionPolicy().triggerIndirection();
>-        uow.executeQuery(query);
>-
>+        Employee bob = (Employee)uow.executeQuery(query);
>+        assertNotNull(bob);
>+        assertEquals("Bob", bob.getManager().getFirstName());
>         emp = new ExpressionBuilder();
>         queryExp = emp.get("manager").get("firstName").equal("John");
>         query = new ReadObjectQuery(Employee.class, queryExp);
>         query.conformResultsInUnitOfWork();
>         query.getInMemoryQueryIndirectionPolicy().triggerIndirection();
>-        uow.executeQuery(query);
>-    }
>-
>-    public void verify(){
>-        if (exception != null){
>-            throw new TestErrorException("A ConcurrentModificationException was thrown when using a Self Referencing Relationship and " +
>-                     "a TriggerIndirection query policy.");
>-        }
>-    }
>-
>-    public void reset(){
>-        uow = null;
>-        getSession().getIdentityMapAccessor().initializeAllIdentityMaps();
>+        Employee john = (Employee)uow.executeQuery(query);
>+        assertNotNull(john);
>+        assertEquals("John", john.getManager().getFirstName());
>     }
> 
> }
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/AbstractIdentityMapEnumeration.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/AbstractIdentityMapEnumeration.java
>new file mode 100644
>index 0000000..64ebc47
>--- /dev/null
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/AbstractIdentityMapEnumeration.java
>@@ -0,0 +1,108 @@
>+/*******************************************************************************
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>+ * This program and the accompanying materials are made available under the
>+ * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>+ * which accompanies this distribution.
>+ * The Eclipse Public License is available at http://www.eclipse.org/legal/epl-v10.html
>+ * and the Eclipse Distribution License is available at
>+ * http://www.eclipse.org/org/documents/edl-v10.php.
>+ *
>+ * Contributors:
>+ *     Oracle - initial API and implementation from Oracle TopLink
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>+ ******************************************************************************/
>+package org.eclipse.persistence.internal.identitymaps;
>+
>+import java.util.Collection;
>+import java.util.Enumeration;
>+import java.util.Iterator;
>+import java.util.NoSuchElementException;
>+
>+/**
>+ * Abstract {@link Enumeration} interface implementation for {@link IdentityMap}
>+ * interface. Allows to iterate over {@link CacheKey} instances stored in the map.
>+ *
>+ * @param <T> type of iterated {@link CacheKey} content
>+ */
>+public abstract class AbstractIdentityMapEnumeration<T> implements Enumeration<T> {
>+
>+    /** {@link CacheKey} instances iterator. */
>+    protected final Iterator<CacheKey> cacheKeysIterator;
>+
>+    /** Next key to be returned. */
>+    protected CacheKey nextKey;
>+
>+    /** Value of {@code true} if readLocks should be checked or false otherwise. */
>+    protected boolean shouldCheckReadLocks;
>+
>+    /**
>+     * Creates an instance of {@link CacheKey} content enumeration.
>+     *
>+     * @param keys {@link Collection} of {@link CacheKey} instances to be iterated
>+     * @param shouldCheckReadLocks value of {@code true} if read lock on the {@link CacheKey}
>+     *        instances should be checked or {@code false} otherwise
>+     */
>+    public AbstractIdentityMapEnumeration(Collection<CacheKey> keys, boolean shouldCheckReadLocks) {
>+        this.shouldCheckReadLocks = shouldCheckReadLocks;
>+        this.cacheKeysIterator = keys.iterator();
>+    }
>+
>+    /**
>+     * Check whether this enumeration contains more elements.
>+     *
>+     * @return value of {@code true} if this enumeration object contains at least
>+     *         one more element to provide or {@code false} otherwise
>+     */
>+    @Override
>+    public boolean hasMoreElements() {
>+        this.nextKey = getNextCacheKey();
>+        return this.nextKey != null;
>+    }
>+
>+    /**
>+     * Get next element of {@link CacheKey} content enumeration if this enumeration
>+     * object has at least one more element to provide.
>+     * It it expected that this method will be implemented using {@link #getNextElement()}
>+     * in child classes.
>+     *
>+     * @return the next element of this enumeration
>+     * @exception NoSuchElementException if no more elements exist
>+     */
>+    @Override
>+    public abstract T nextElement();
>+
>+    /**
>+     * Get next {@link CacheKey} instance from iterator.
>+     *
>+     * @return next {@link CacheKey} instance or {@code null} if there is no more
>+     *         instance to provide
>+     */
>+    private CacheKey getNextCacheKey() {
>+        CacheKey key = null;
>+        while (cacheKeysIterator.hasNext() && (key == null)) {
>+            key = cacheKeysIterator.next();
>+        }
>+        return key;
>+    }
>+
>+    /**
>+     * Get next element of {@link CacheKey} instances enumeration if this enumeration
>+     * object has at least one more element to provide.
>+     *
>+     * @return the next element of this enumeration
>+     * @exception NoSuchElementException  if no more elements exist
>+     */
>+    protected CacheKey getNextElement() {
>+        if (this.nextKey == null) {
>+            throw new NoSuchElementException("AbstractIdentityMapEnumeration nextElement");
>+        }
>+        // The read lock check is for avoidance of half built objects being returned.
>+        // bug 275724: Added shouldCheckReadLocks to avoid the read lock check when invalidating.
>+        if (shouldCheckReadLocks) {
>+            this.nextKey.checkReadLock();
>+        }
>+        return this.nextKey;
>+    }
>+
>+}
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/FullIdentityMap.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/FullIdentityMap.java
>index abeaab8..9267a73 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/FullIdentityMap.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/FullIdentityMap.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -9,6 +9,8 @@
>  *
>  * Contributors:
>  *     Oracle - initial API and implementation from Oracle TopLink
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.internal.identitymaps;
> 
>@@ -84,11 +86,13 @@ public void collectLocks(HashMap threadList) {
>     }
> 
>     /**
>-     * Allow for the cache to be iterated on.
>+     * Allow for the cache {@link CacheKey#getObject()} elements to be iterated.
>+     *
>+     * @return {@link Enumeration} of {@link CacheKey#getObject()} instances.
>      */
>     @Override
>-    public Enumeration elements() {
>-        return new IdentityMapEnumeration(this);
>+    public Enumeration<Object> elements() {
>+        return new IdentityMapEnumeration(this.getCacheKeys().values());
>     }
> 
>     /**
>@@ -157,16 +161,32 @@ public int getSize(Class myClass, boolean recurse) {
>      * Read locks will be checked.
>      */
>     @Override
>-    public Enumeration keys() {
>+    public Enumeration<CacheKey> keys() {
>         return keys(true);
>     }
> 
>     /**
>-     * Allow for the cache keys to be iterated on.
>-     * @param checkReadLocks - true if readLocks should be checked, false otherwise.
>+     * Allow for the {@link CacheKey} elements to be iterated.
>+     * {@link CacheKey} {@link Collection} is reused so this iteration may not be thread safe.
>+     *
>+     * @param checkReadLocks value of {@code true} if read lock on the {@link CacheKey}
>+     *        instances should be checked or {@code false} otherwise
>+     * @return {@link Enumeration} of {@link CacheKey} instances.
>+     */
>+    public Enumeration<CacheKey> keys(boolean checkReadLocks) {
>+        return new IdentityMapKeyEnumeration(this.getCacheKeys().values(), checkReadLocks);
>+    }
>+
>+    /**
>+     * Allow for the {@link CacheKey} elements to be iterated.
>+     * Clone of {@link CacheKey} {@link Collection} is returned so this iteration should
>+     * be thread safe.
>+     *
>+     * @return {@link Enumeration} with clone of the {@link CacheKey} {@link Collection}
>      */
>-    public Enumeration keys(boolean checkReadLocks) {
>-        return new IdentityMapKeyEnumeration(this, checkReadLocks);
>+    @Override
>+    public Enumeration<CacheKey> cloneKeys() {
>+        return new IdentityMapKeyEnumeration(new ArrayList(this.getCacheKeys().values()), true);
>     }
> 
>     /**
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMap.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMap.java
>index 2335078..e0420bd 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMap.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMap.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -9,6 +9,8 @@
>  *
>  * Contributors:
>  *     Gordon Yorke - Part of the Cache Interceptor feature. (ER 219683)
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.internal.identitymaps;
> 
>@@ -98,7 +100,7 @@
>     /**
>      * Allow for the cache to be iterated on.
>      */
>-    public Enumeration elements();
>+    public Enumeration<Object> elements();
> 
>     /**
>      * Return the object cached in the identity map or null if it could not be found.
>@@ -181,13 +183,21 @@
>      * Allow for the CacheKeys to be iterated on.
>      * Read locks should be checked
>      */
>-    public Enumeration keys();
>+    public Enumeration<CacheKey> keys();
>+
>+    /**
>+     * Allow for the CacheKeys to be iterated on using copy of keys enumeration.
>+     * This is thread safe access to keys.
>+     *
>+     * @return clone of the CacheKeys enumeration
>+     */
>+    public Enumeration<CacheKey> cloneKeys();
> 
>     /**
>      * Allow for the CacheKeys to be iterated on.
>      * @param checkReadLocks - true if readLocks should be checked, false otherwise.
>      */
>-    public Enumeration keys(boolean checkReadLocks);
>+    public Enumeration<CacheKey> keys(boolean checkReadLocks);
> 
>     /**
>      * Notify the cache that a lazy relationship has been triggered in the object
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapEnumeration.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapEnumeration.java
>index bcd01ce..95f9a18 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapEnumeration.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapEnumeration.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -9,22 +9,38 @@
>  *
>  * Contributors:
>  *     Oracle - initial API and implementation from Oracle TopLink
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.internal.identitymaps;
> 
>+import java.util.Collection;
>+import java.util.NoSuchElementException;
>+
> /**
>- * Used to allow iterating over a map.
>+ * Allows to iterate over {@code Object} instances stored in {@link CacheKey} instances
>+ * in the {@link IdentityMap}.
>  */
>-public class IdentityMapEnumeration extends IdentityMapKeyEnumeration {
>-    public IdentityMapEnumeration(FullIdentityMap map) {
>-        super(map);
>+public class IdentityMapEnumeration extends AbstractIdentityMapEnumeration<Object>{
>+
>+    /**
>+     * Creates an instance of {@link CacheKey} instances enumeration.
>+     * Checking of read lock on the {@link CacheKey} instances is turned on.
>+     *
>+     * @param keys {@link Collection} of {@link CacheKey} instances to be iterated
>+     */
>+    public IdentityMapEnumeration(Collection<CacheKey> keys) {
>+        super(keys, true);
>     }
> 
>     /**
>-     * Return the nextElement. It must be set to null so that subsequent calls will
>-     * actual get subsequent objects through the getNextCachedObject() logic.
>+     * Get next {@link CacheKey#getObject()} element of {@link CacheKey} enumeration
>+     * if this enumeration object has at least one more element to provide.
>+     *
>+     * @return the next {@link CacheKey#getObject()} element of this enumeration
>+     * @exception  NoSuchElementException  if no more elements exist
>      */
>     public Object nextElement() {
>-        return ((CacheKey)super.nextElement()).getObject();
>+        return getNextElement().getObject();
>     }
> }
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapKeyEnumeration.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapKeyEnumeration.java
>index 58883f0..1ca6a4c 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapKeyEnumeration.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapKeyEnumeration.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -9,63 +9,49 @@
>  *
>  * Contributors:
>  *     Oracle - initial API and implementation from Oracle TopLink
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.internal.identitymaps;
> 
> import java.util.*;
> 
> /**
>- * Used to allow iterating over a maps cache keys.
>+ * Allows to iterate over {@link CacheKey} instances stored in the {@link IdentityMap}.
>  */
>-public class IdentityMapKeyEnumeration implements Enumeration {
>-
>-    protected FullIdentityMap map;
>-    protected Iterator cacheKeysIterator;
>-    protected CacheKey nextKey;
>-    protected boolean shouldCheckReadLocks;
>-
>-    public IdentityMapKeyEnumeration(FullIdentityMap map) {
>-        this(map, true);
>-    }
>-
>-    public IdentityMapKeyEnumeration(FullIdentityMap map, boolean shouldCheckReadLocks) {
>-        this.map = map;
>-        this.shouldCheckReadLocks = shouldCheckReadLocks;
>-        this.cacheKeysIterator = map.getCacheKeys().values().iterator();
>-    }
>-
>-    public boolean hasMoreElements() {
>-        this.nextKey = getNextCacheKey();
>-        return this.nextKey != null;
>-    }
>-
>-    public Object nextElement() {
>-        if (this.nextKey == null) {
>-            throw new NoSuchElementException("IdentityMapKeyEnumeration nextElement");
>-        }
>-
>-        // The read lock check is for avoidance of half built objects being returned.
>-        // bug 275724: Added shouldCheckReadLocks to avoid the read lock check when invalidating.
>-        if (shouldCheckReadLocks) {
>-            this.nextKey.checkReadLock();
>-        }
>-        return this.nextKey;
>-    }
>-
>-    protected CacheKey getNextCacheKey() {
>-        CacheKey key = null;
>-        while (this.cacheKeysIterator.hasNext() && (key == null)) {
>-            key = (CacheKey)this.cacheKeysIterator.next();
>-        }
>-        return key;
>+public class IdentityMapKeyEnumeration extends AbstractIdentityMapEnumeration<CacheKey> {
>+
>+    /**
>+     * Creates an instance of {@link CacheKey} instances enumeration.
>+     * Checking of read lock on the {@link CacheKey} instances is turned on.
>+     *
>+     * @param keys {@link Collection} of {@link CacheKey} instances to be iterated
>+     */
>+    public IdentityMapKeyEnumeration(Collection<CacheKey> keys) {
>+        super(keys, true);
>     }
> 
>-    public boolean getShouldCheckReadLocks() {
>-        return this.shouldCheckReadLocks;
>+    /**
>+     * Creates an instance of {@link CacheKey} instances enumeration.
>+     *
>+     * @param keys {@link Collection} of {@link CacheKey} instances to be iterated
>+     * @param shouldCheckReadLocks value of {@code true} if read lock on the {@link CacheKey}
>+     *        instances should be checked or {@code false} otherwise
>+     */
>+    public IdentityMapKeyEnumeration(Collection<CacheKey> keys, boolean shouldCheckReadLocks) {
>+        super(keys, shouldCheckReadLocks);
>     }
> 
>-    public void setShouldCheckReadLocks(boolean shouldCheckReadLocks) {
>-        this.shouldCheckReadLocks = shouldCheckReadLocks;
>+    /**
>+     * Get next element of {@link CacheKey} enumeration if this enumeration
>+     * object has at least one more element to provide.
>+     *
>+     * @return the next element of this enumeration
>+     * @exception  NoSuchElementException  if no more elements exist
>+     */
>+    @Override
>+    public CacheKey nextElement() {
>+        return getNextElement();
>     }
> 
> }
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapManager.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapManager.java
>index fbf82e3..9c74c96 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapManager.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/IdentityMapManager.java
>@@ -12,7 +12,9 @@
>  *     Oracle - initial API and implementation from Oracle TopLink
>  *     Mark Wolochuk - Bug 321041 ConcurrentModificationException on getFromIdentityMap() fix
>  *     11/07/2017 - Dalia Abo Sheasha
>- *       - 526957 : Split the logging and trace messages
>+ *       - 526957: Split the logging and trace messages
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.internal.identitymaps;
> 
>@@ -539,12 +541,17 @@ public Vector getAllFromIdentityMap(Expression selectionCriteria, Class theClass
>             }
>             objects = new Vector();
>             IdentityMap map = getIdentityMap(descriptor, false);
>+
>+            // Bug #522635 - if policy is set to trigger indirection, then iterate over a copy of the cache keys collection
>+            //               to avoid a ConcurrentModificationException
>+            final Enumeration cacheEnum = valueHolderPolicy == InMemoryQueryIndirectionPolicy.SHOULD_TRIGGER_INDIRECTION ? map.cloneKeys() : map.keys();
>+
>             // bug 327900 - If don't read subclasses is set on the descriptor heed it.
>             boolean readSubclassesOrNoInheritance = (!descriptor.hasInheritance() || descriptor.getInheritancePolicy().shouldReadSubclasses());
> 
>             // cache the current time to avoid calculating it every time through the loop
>             long currentTimeInMillis = System.currentTimeMillis();
>-            for (Enumeration cacheEnum = map.keys(); cacheEnum.hasMoreElements();) {
>+            while (cacheEnum.hasMoreElements()) {
>                 CacheKey key = (CacheKey)cacheEnum.nextElement();
>                 if ((key.getObject() == null) || (!shouldReturnInvalidatedObjects && descriptor.getCacheInvalidationPolicy().isInvalidated(key, currentTimeInMillis))) {
>                     continue;
>@@ -852,24 +859,9 @@ public Object getFromIdentityMap(Expression selectionCriteria, Class theClass, R
>             }
>             IdentityMap map = getIdentityMap(descriptor, false);
> 
>-            // Bug #321041 - if policy is set to trigger indirection, then make a copy of the cache keys collection
>-            // and iterate over that to avoid a ConcurrentModificationException.
>-            // This happens when the indirect attribute is of the same type (or has same mapped superclass) as
>-            // the parent object. EclipseLink inserts the object into the same collection it is iterating over,
>-            // which results in a ConcurrentModificationException.
>-            // There's a slight performance hit in copying the collection, but we are already taking a hit
>-            // by triggering indirection in the first place.
>-            boolean copyKeyCollection = valueHolderPolicy == InMemoryQueryIndirectionPolicy.SHOULD_TRIGGER_INDIRECTION;
>-            Vector cacheKeys = null;
>-            if (copyKeyCollection) {
>-                cacheKeys = new Vector(map.getSize());
>-                for (Enumeration cacheEnum = map.keys(); cacheEnum.hasMoreElements();) {
>-                    CacheKey key = (CacheKey)cacheEnum.nextElement();
>-                    cacheKeys.add(key);
>-                }
>-            }
>-
>-            Enumeration cacheEnum = copyKeyCollection ? cacheKeys.elements() : map.keys();
>+            // Bug #321041 - if policy is set to trigger indirection, then iterate over a copy of the cache keys collection
>+            //               to avoid a ConcurrentModificationException
>+            Enumeration cacheEnum = valueHolderPolicy == InMemoryQueryIndirectionPolicy.SHOULD_TRIGGER_INDIRECTION ? map.cloneKeys() : map.keys();
> 
>             // cache the current time to avoid calculating it every time through the loop
>             long currentTimeInMillis = System.currentTimeMillis();
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/NoIdentityMap.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/NoIdentityMap.java
>index e47f27b..e51c065 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/NoIdentityMap.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/identitymaps/NoIdentityMap.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -9,6 +9,8 @@
>  *
>  * Contributors:
>  *     Oracle - initial API and implementation from Oracle TopLink
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.internal.identitymaps;
> 
>@@ -45,8 +47,8 @@ public void collectLocks(HashMap threadList) {
>      * Return an empty enumerator.
>      */
>     @Override
>-    public Enumeration elements() {
>-        return new Vector(0).elements();
>+    public Enumeration<Object> elements() {
>+        return Collections.<Object>emptyEnumeration();
>     }
> 
>     /**
>@@ -101,15 +103,23 @@ public Object getWriteLockValue(Object primaryKey) {
>      * Return an empty enumerator.
>      */
>     @Override
>-    public Enumeration keys() {
>-        return new Vector(0).elements();
>+    public Enumeration<CacheKey> keys() {
>+        return Collections.<CacheKey>emptyEnumeration();
>     }
> 
>     /**
>      * Return an empty enumerator.
>      */
>-    public Enumeration keys(boolean checkReadLocks) {
>-        return keys();
>+    @Override
>+    public Enumeration<CacheKey> cloneKeys() {
>+        return Collections.<CacheKey>emptyEnumeration();
>+    }
>+
>+    /**
>+     * Return an empty enumerator.
>+     */
>+    public Enumeration<CacheKey> keys(boolean checkReadLocks) {
>+        return Collections.<CacheKey>emptyEnumeration();
>     }
> 
>     /**
>diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/sessions/interceptors/CacheInterceptor.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/sessions/interceptors/CacheInterceptor.java
>index 4677413..8d7b458 100644
>--- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/sessions/interceptors/CacheInterceptor.java
>+++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/sessions/interceptors/CacheInterceptor.java
>@@ -1,5 +1,5 @@
> /*******************************************************************************
>- * Copyright (c) 1998, 2015 Oracle and/or its affiliates. All rights reserved.
>+ * Copyright (c) 1998, 2017 Oracle and/or its affiliates. All rights reserved.
>  * This program and the accompanying materials are made available under the
>  * terms of the Eclipse Public License v1.0 and Eclipse Distribution License v. 1.0
>  * which accompanies this distribution.
>@@ -9,9 +9,12 @@
>  *
>  * Contributors:
>  *     Gordon Yorke - Initial Feature development
>+ *     12/14/2017-3.0 Tomas Kraus
>+ *       - 522635: ConcurrentModificationException when triggering lazy load from conforming query
>  ******************************************************************************/
> package org.eclipse.persistence.sessions.interceptors;
> 
>+import java.util.Collection;
> import java.util.Enumeration;
> import java.util.HashMap;
> import java.util.Map;
>@@ -152,7 +155,7 @@ public boolean containsKey(Object primaryKey) {
>      * Allow for the cache to be iterated on.  This method is only used during debugging when
>      * validateCache() has been called to print out the contents of the cache.
>      */
>-    public Enumeration elements() {
>+    public Enumeration<Object> elements() {
>         return this.targetIdentityMap.elements();
>     }
> 
>@@ -262,17 +265,35 @@ public Object getWriteLockValue(Object primaryKey) {
>     }
> 
>     /**
>-     * Allow for the CacheKeys to be iterated on.
>+     * Allow for the {@link CacheKey} elements to be iterated.
>+     * {@link CacheKey} {@link Collection} is reused so this iteration may not be thread safe.
>+     *
>+     * @return {@link Enumeration} of {@link CacheKey} instances.
>      */
>-    public Enumeration keys() {
>+    public Enumeration<CacheKey> keys() {
>         return this.targetIdentityMap.keys();
>     }
> 
>     /**
>-     * Allow for the CacheKeys to be iterated on. - value should be true
>-     * if readloacks are to be used, false otherwise.
>+     * Allow for the {@link CacheKey} elements to be iterated.
>+     * Clone of {@link CacheKey} {@link Collection} is returned so this iteration should
>+     * be thread safe.
>+     *
>+     * @return {@link Enumeration} with clone of the {@link CacheKey} {@link Collection}
>      */
>-    public Enumeration keys(boolean checkReadLocks) {
>+    public Enumeration<CacheKey> cloneKeys() {
>+        return this.targetIdentityMap.cloneKeys();
>+    }
>+
>+    /**
>+     * Allow for the {@link CacheKey} elements to be iterated.
>+     * {@link CacheKey} {@link Collection} is reused so this iteration may not be thread safe.
>+     *
>+     * @param checkReadLocks value of {@code true} if read lock on the {@link CacheKey}
>+     *        instances should be checked or {@code false} otherwise
>+     * @return {@link Enumeration} of {@link CacheKey} instances.
>+     */
>+    public Enumeration<CacheKey> keys(boolean checkReadLocks) {
>         return this.targetIdentityMap.keys(checkReadLocks);
>     }
> 
>-- 
>2.10.1 (Apple Git-78)
>

Actions: View | Diff

Attachments on bug 522635: 271316 | 271946