This is an automated email from the ASF dual-hosted git repository. ntimofeev pushed a commit to branch STABLE-4.2 in repository https://gitbox.apache.org/repos/asf/cayenne.git
commit 785e5d5148e9980c3649a6f517708310c48be7dd Author: Nikita Timofeev <stari...@gmail.com> AuthorDate: Mon Mar 10 10:44:17 2025 +0400 Revert "CAY-2876 Memory leak in the ObjectStore" This reverts commit 1f7f2a672e17efd28bd9fb1cf11db75ca41aa45f. --- RELEASE-NOTES.txt | 1 - .../ObjectIdRelationshipHandlerTest.java | 8 +- .../java/org/apache/cayenne/access/ObjectDiff.java | 42 +++--- .../org/apache/cayenne/access/ObjectStore.java | 154 ++++++++------------- .../access/ObjectStorePersistentWrapper.java | 105 -------------- 5 files changed, 82 insertions(+), 228 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index eef9c2a31..e17e94af4 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -22,7 +22,6 @@ Bug Fixes: CAY-2866 DefaultDataDomainFlushAction breaks on circular relationship update CAY-2868 Regression: DefaultDbRowOpSorter shouldn't sort update operations CAY-2871 QualifierTranslator breaks on a relationship with a compound FK -CAY-2876 Memory leak in the ObjectStore CAY-2879 Negative number for non parameterized ObjectSelect query not processed correctly ---------------------------------- diff --git a/cayenne-lifecycle/src/test/java/org/apache/cayenne/lifecycle/relationship/ObjectIdRelationshipHandlerTest.java b/cayenne-lifecycle/src/test/java/org/apache/cayenne/lifecycle/relationship/ObjectIdRelationshipHandlerTest.java index e1c50d32e..12cc2fede 100644 --- a/cayenne-lifecycle/src/test/java/org/apache/cayenne/lifecycle/relationship/ObjectIdRelationshipHandlerTest.java +++ b/cayenne-lifecycle/src/test/java/org/apache/cayenne/lifecycle/relationship/ObjectIdRelationshipHandlerTest.java @@ -20,8 +20,6 @@ package org.apache.cayenne.lifecycle.relationship; import org.apache.cayenne.Cayenne; import org.apache.cayenne.ObjectContext; -import org.apache.cayenne.configuration.Constants; -import org.apache.cayenne.configuration.server.ServerModule; import org.apache.cayenne.configuration.server.ServerRuntime; import org.apache.cayenne.lifecycle.db.E1; import org.apache.cayenne.lifecycle.db.UuidRoot1; @@ -46,11 +44,7 @@ public class ObjectIdRelationshipHandlerTest { @Before public void setUp() throws Exception { - runtime = ServerRuntime.builder() - // Using soft, as weak could lead to unloading data before test completes - .addModule(b -> ServerModule.contributeProperties(b) - .put(Constants.SERVER_OBJECT_RETAIN_STRATEGY_PROPERTY, "soft")) - .addConfig("cayenne-lifecycle.xml").build(); + runtime = ServerRuntime.builder().addConfig("cayenne-lifecycle.xml").build(); // a filter is required to invalidate root objects after commit ObjectIdRelationshipFilter filter = new ObjectIdRelationshipFilter(); diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectDiff.java b/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectDiff.java index c62f95a3d..9b2b68de4 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectDiff.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectDiff.java @@ -65,24 +65,26 @@ public class ObjectDiff extends NodeDiff { private Map<ArcOperation, ArcOperation> flatIds; private Map<ArcOperation, ArcOperation> phantomFks; - private final ObjectStorePersistentWrapper object; + private Persistent object; - ObjectDiff(final ObjectStorePersistentWrapper object) { + ObjectDiff(final Persistent object) { - super(object.dataObject().getObjectId()); + super(object.getObjectId()); - // retain the object, as ObjectStore may have weak references to registered objects, - // and we can't allow it to deallocate dirty objects. + // retain the object, as ObjectStore may have weak references to + // registered + // objects and we can't allow it to deallocate dirty objects. this.object = object; - EntityResolver entityResolver = object.dataObject().getObjectContext().getEntityResolver(); + EntityResolver entityResolver = object.getObjectContext().getEntityResolver(); - this.entityName = object.dataObject().getObjectId().getEntityName(); + this.entityName = object.getObjectId().getEntityName(); this.classDescriptor = entityResolver.getClassDescriptor(entityName); - int state = object.dataObject().getPersistenceState(); + int state = object.getPersistenceState(); - // take snapshot of simple properties and arcs used for optimistic locking.. + // take snapshot of simple properties and arcs used for optimistic + // locking.. if (state == PersistenceState.COMMITTED || state == PersistenceState.DELETED || state == PersistenceState.MODIFIED) { @@ -97,7 +99,7 @@ public class ObjectDiff extends NodeDiff { @Override public boolean visitAttribute(AttributeProperty property) { - snapshot.put(property.getName(), property.readProperty(object.dataObject())); + snapshot.put(property.getName(), property.readProperty(object)); return true; } @@ -112,8 +114,8 @@ public class ObjectDiff extends NodeDiff { // eagerly resolve optimistically locked relationships Object target = (lock && isUsedForLocking) - ? property.readProperty(object.dataObject()) - : property.readPropertyDirectly(object.dataObject()); + ? property.readProperty(object) + : property.readPropertyDirectly(object); if (target instanceof Persistent) { target = ((Persistent) target).getObjectId(); @@ -128,14 +130,14 @@ public class ObjectDiff extends NodeDiff { } Object getObject() { - return object.dataObject(); + return object; } ClassDescriptor getClassDescriptor() { // class descriptor is initiated in constructor, but is nullified on // serialization if (classDescriptor == null) { - EntityResolver entityResolver = object.dataObject().getObjectContext().getEntityResolver(); + EntityResolver entityResolver = object.getObjectContext().getEntityResolver(); this.classDescriptor = entityResolver.getClassDescriptor(entityName); } @@ -150,7 +152,7 @@ public class ObjectDiff extends NodeDiff { Object value = arcSnapshot != null ? arcSnapshot.get(propertyName) : null; if (value instanceof Fault) { - Persistent target = (Persistent) ((Fault) value).resolveFault(object.dataObject(), propertyName); + Persistent target = (Persistent) ((Fault) value).resolveFault(object, propertyName); value = target != null ? target.getObjectId() : null; arcSnapshot.put(propertyName, value); @@ -165,7 +167,7 @@ public class ObjectDiff extends NodeDiff { public ObjectId getCurrentArcSnapshotValue(String propertyName) { Object value = currentArcSnapshot != null ? currentArcSnapshot.get(propertyName) : null; if (value instanceof Fault) { - Persistent target = (Persistent) ((Fault) value).resolveFault(object.dataObject(), propertyName); + Persistent target = (Persistent) ((Fault) value).resolveFault(object, propertyName); value = target != null ? target.getObjectId() : null; currentArcSnapshot.put(propertyName, value); @@ -326,7 +328,7 @@ public class ObjectDiff extends NodeDiff { return false; } - int state = object.dataObject().getPersistenceState(); + int state = object.getPersistenceState(); if (state == PersistenceState.NEW || state == PersistenceState.DELETED) { return false; } @@ -340,7 +342,7 @@ public class ObjectDiff extends NodeDiff { public boolean visitAttribute(AttributeProperty property) { Object oldValue = snapshot.get(property.getName()); - Object newValue = property.readProperty(object.dataObject()); + Object newValue = property.readProperty(object); if (!property.equals(oldValue, newValue)) { modFound[0] = true; @@ -361,7 +363,7 @@ public class ObjectDiff extends NodeDiff { return true; } - Object newValue = property.readPropertyDirectly(object.dataObject()); + Object newValue = property.readPropertyDirectly(object); if (newValue instanceof Fault) { return true; } @@ -409,7 +411,7 @@ public class ObjectDiff extends NodeDiff { @Override public boolean visitAttribute(AttributeProperty property) { - Object newValue = property.readProperty(object.dataObject()); + Object newValue = property.readProperty(object); // no baseline to compare if (snapshot == null) { diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java b/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java index aa522f017..74b778c79 100644 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java @@ -53,7 +53,8 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.concurrent.atomic.AtomicInteger; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; /** * ObjectStore stores objects using their ObjectId as a key. It works as a dedicated @@ -65,9 +66,6 @@ import java.util.concurrent.atomic.AtomicInteger; */ public class ObjectStore implements Serializable, SnapshotEventListener, GraphManager { - /** - * Actual content is ObjectId -> ObjectStorePersistentWrapper - */ protected Map<Object, Persistent> objectMap; protected Map<Object, ObjectDiff> changes; @@ -75,7 +73,6 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * Map that tracks flattened paths for given object Id that is present in db. * Presence of path in this map is used to separate insert from update case of flattened records. * @since 4.1 - * @deprecated since 4.2.2 it is unused */ protected Map<Object, Map<String, ObjectId>> trackedFlattenedPaths; @@ -141,7 +138,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa Collection<GraphDiff> getLifecycleEventInducedChanges() { return lifecycleEventInducedChanges != null ? lifecycleEventInducedChanges - : Collections.emptyList(); + : Collections.<GraphDiff>emptyList(); } void registerLifecycleEventInducedChange(GraphDiff diff) { @@ -169,11 +166,10 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa if (objectDiff == null) { - ObjectStorePersistentWrapper persistentWrapper = (ObjectStorePersistentWrapper)objectMap.get(nodeId); - if (persistentWrapper == null) { + Persistent object = objectMap.get(nodeId); + if (object == null) { throw new CayenneRuntimeException("No object is registered in context with Id %s", nodeId); } - Persistent object = persistentWrapper.dataObject(); if (object.getPersistenceState() == PersistenceState.COMMITTED) { object.setPersistenceState(PersistenceState.MODIFIED); @@ -205,7 +201,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa } } - objectDiff = new ObjectDiff(persistentWrapper); + objectDiff = new ObjectDiff(object); objectDiff.setDiffId(++currentDiffId); changes.put(nodeId, objectDiff); } @@ -217,28 +213,13 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa return objectDiff; } - private Persistent getUnwrapped(Object nodeId) { - Persistent persistent = objectMap.get(nodeId); - if(persistent == null) { - return null; - } - return ((ObjectStorePersistentWrapper) persistent).dataObject(); - } - /** * Returns a number of objects currently registered with this ObjectStore. * * @since 1.2 */ public int registeredObjectsCount() { - AtomicInteger counter = new AtomicInteger(); - objectMap.forEach((id, obj) -> { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj; - if(wrapper.hasObject()){ - counter.incrementAndGet(); - } - }); - return counter.get(); + return objectMap.size(); } /** @@ -322,6 +303,9 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa // remove object but not snapshot objectMap.remove(id); changes.remove(id); + if(id != null && trackedFlattenedPaths != null) { + trackedFlattenedPaths.remove(id); + } ids.add(id); object.setObjectContext(null); @@ -407,7 +391,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa for (Object id : changes.keySet()) { - Persistent object = getUnwrapped(id); + Persistent object = objectMap.get(id); // assume that no new or deleted objects are present (as otherwise commit // wouldn't have been phantom). @@ -427,7 +411,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa // scan through changed objects, set persistence state to committed for (Object id : changes.keySet()) { - Persistent object = getUnwrapped(id); + Persistent object = objectMap.get(id); switch (object.getPersistenceState()) { case PersistenceState.DELETED: @@ -524,7 +508,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * Returns an iterator over the registered objects. */ public synchronized Iterator<Persistent> getObjectIterator() { - return new WrapperIterator(objectMap.values().iterator()); + return objectMap.values().iterator(); } /** @@ -546,9 +530,8 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa List<Persistent> filteredObjects = new ArrayList<>(); for (Persistent object : objectMap.values()) { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) object; - if (wrapper.hasObject() && object.getPersistenceState() == state) { - filteredObjects.add(wrapper.dataObject()); + if (object.getPersistenceState() == state) { + filteredObjects.add(object); } } @@ -612,24 +595,20 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa if (object != null) { object.setObjectId((ObjectId) newId); - objectMap.merge(newId, object, (oldValue, newValue) -> { - ObjectStorePersistentWrapper oldWrapper = (ObjectStorePersistentWrapper) oldValue; - ObjectStorePersistentWrapper newWrapper = (ObjectStorePersistentWrapper) newValue; - if(oldWrapper.trackedFlattenedPaths != null) { - if(newWrapper.trackedFlattenedPaths != null) { - newWrapper.trackedFlattenedPaths.putAll(oldWrapper.trackedFlattenedPaths); - } else { - newWrapper.trackedFlattenedPaths = oldWrapper.trackedFlattenedPaths; - } - } - return newWrapper; - }); + objectMap.put(newId, object); ObjectDiff change = changes.remove(nodeId); if (change != null) { changes.put(newId, change); } } + + if(trackedFlattenedPaths != null) { + Map<String, ObjectId> paths = trackedFlattenedPaths.remove(nodeId); + if(paths != null) { + trackedFlattenedPaths.put(newId, paths); + } + } } /** @@ -640,7 +619,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa void processDeletedID(ObjectId nodeId) { // access object map directly - the method should be called in a synchronized context... - Persistent object = getUnwrapped(nodeId); + Persistent object = objectMap.get(nodeId); if (object != null) { @@ -746,7 +725,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa void processIndirectlyModifiedIDs(Collection<ObjectId> indirectlyModifiedIDs) { for (ObjectId oid : indirectlyModifiedIDs) { // access object map directly - the method should be called in a synchronized context... - final DataObject object = (DataObject) getUnwrapped(oid); + final DataObject object = (DataObject) objectMap.get(oid); if (object == null || object.getPersistenceState() != PersistenceState.COMMITTED) { continue; @@ -800,7 +779,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa void processUpdatedSnapshot(ObjectId nodeId, DataRow diff) { // access object map directly - the method should be called in a synchronized context... - DataObject object = (DataObject) getUnwrapped(nodeId); + DataObject object = (DataObject) objectMap.get(nodeId); // no object, or HOLLOW object require no processing if (object != null) { @@ -879,12 +858,12 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa */ @Override public synchronized Object getNode(Object nodeId) { - return getUnwrapped(nodeId); + return objectMap.get(nodeId); } // non-synchronized version of getNode for private use final Object getNodeNoSync(Object nodeId) { - return getUnwrapped(nodeId); + return objectMap.get(nodeId); } /** @@ -895,15 +874,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa */ @Override public synchronized Collection<Object> registeredNodes() { - List<Object> values = new ArrayList<>(objectMap.size()); - objectMap.forEach((id, persistent) - -> { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) persistent; - if(wrapper.hasObject()) { - values.add(wrapper.dataObject()); - } - }); - return values; + return new ArrayList<Object>(objectMap.values()); } /** @@ -911,7 +882,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa */ @Override public synchronized void registerNode(Object nodeId, Object nodeObject) { - objectMap.put(nodeId, new ObjectStorePersistentWrapper((Persistent) nodeObject)); + objectMap.put(nodeId, (Persistent) nodeObject); } /** @@ -1022,32 +993,47 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * @since 4.1 */ boolean hasFlattenedPath(ObjectId objectId, String path) { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); - return wrapper.hasFlattenedPath(path); + if(trackedFlattenedPaths == null) { + return false; + } + return trackedFlattenedPaths + .getOrDefault(objectId, Collections.emptyMap()).containsKey(path); } /** * @since 4.2 */ public ObjectId getFlattenedId(ObjectId objectId, String path) { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); - return wrapper.getFlattenedId(path); + if(trackedFlattenedPaths == null) { + return null; + } + + return trackedFlattenedPaths + .getOrDefault(objectId, Collections.emptyMap()).get(path); } /** * @since 4.2 */ public Collection<ObjectId> getFlattenedIds(ObjectId objectId) { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); - return wrapper.getFlattenedIds(); + if(trackedFlattenedPaths == null) { + return Collections.emptyList(); + } + + return trackedFlattenedPaths + .getOrDefault(objectId, Collections.emptyMap()).values(); } /** * @since 4.2.1 */ public Map<String, ObjectId> getFlattenedPathIdMap(ObjectId objectId) { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); - return wrapper.getFlattenedPathIdMap(); + if(trackedFlattenedPaths == null) { + return Collections.emptyMap(); + } + + return trackedFlattenedPaths + .getOrDefault(objectId, Collections.emptyMap()); } /** @@ -1055,9 +1041,12 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * @since 4.1 */ public void markFlattenedPath(ObjectId objectId, String path, ObjectId id) { - ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap - .computeIfAbsent(objectId, objId -> new ObjectStorePersistentWrapper(null)); - wrapper.markFlattenedPath(path, id); + if(trackedFlattenedPaths == null) { + trackedFlattenedPaths = new ConcurrentHashMap<>(); + } + trackedFlattenedPaths + .computeIfAbsent(objectId, o -> new ConcurrentHashMap<>()) + .put(path, id); } // an ObjectIdQuery optimized for retrieval of multiple snapshots - it can be reset @@ -1101,29 +1090,4 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa throw new UnsupportedOperationException(); } } - - static class WrapperIterator implements Iterator<Persistent> { - - final Iterator<ObjectStorePersistentWrapper> iterator; - - @SuppressWarnings({"unchecked", "rawtypes"}) - WrapperIterator(Iterator<Persistent> iterator) { - this.iterator = (Iterator)iterator; - } - - @Override - public boolean hasNext() { - return iterator.hasNext(); - } - - @Override - public Persistent next() { - return iterator.next().dataObject(); - } - - @Override - public void remove() { - iterator.remove(); - } - } } diff --git a/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStorePersistentWrapper.java b/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStorePersistentWrapper.java deleted file mode 100644 index 8a7677966..000000000 --- a/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStorePersistentWrapper.java +++ /dev/null @@ -1,105 +0,0 @@ -/***************************************************************** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - ****************************************************************/ - -package org.apache.cayenne.access; - -import org.apache.cayenne.ObjectContext; -import org.apache.cayenne.ObjectId; -import org.apache.cayenne.Persistent; - -import java.util.Collection; -import java.util.Collections; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; - -/** - * Wrapper for a {@link Persistent} object used by the {@link ObjectStore} to keep additional info about persistent. - * <p> - * Right now the only additional information it keeps is flattened path linked to the object. - * @since 4.2.2 - */ -public class ObjectStorePersistentWrapper implements Persistent { - protected final Persistent dataObject; - protected Map<String, ObjectId> trackedFlattenedPaths; - - public ObjectStorePersistentWrapper(Persistent dataObject) { - this.dataObject = dataObject; - } - - public boolean hasObject() { - return dataObject != null; - } - - public Persistent dataObject() { - return dataObject; - } - - public void markFlattenedPath(String path, ObjectId objectId) { - if (trackedFlattenedPaths == null) { - trackedFlattenedPaths = new ConcurrentHashMap<>(); - } - trackedFlattenedPaths.put(path, objectId); - } - - public boolean hasFlattenedPath(String path) { - return trackedFlattenedPaths != null && trackedFlattenedPaths.containsKey(path); - } - - public ObjectId getFlattenedId(String path) { - return trackedFlattenedPaths == null ? null : trackedFlattenedPaths.get(path); - } - - public Collection<ObjectId> getFlattenedIds() { - return trackedFlattenedPaths == null ? Collections.emptyList() : trackedFlattenedPaths.values(); - } - - public Map<String, ObjectId> getFlattenedPathIdMap() { - return trackedFlattenedPaths == null ? Collections.emptyMap() : trackedFlattenedPaths; - } - - @Override - public ObjectId getObjectId() { - return dataObject.getObjectId(); - } - - @Override - public void setObjectId(ObjectId id) { - dataObject.setObjectId(id); - } - - @Override - public int getPersistenceState() { - return dataObject.getPersistenceState(); - } - - @Override - public void setPersistenceState(int state) { - dataObject.setPersistenceState(state); - } - - @Override - public ObjectContext getObjectContext() { - return dataObject.getObjectContext(); - } - - @Override - public void setObjectContext(ObjectContext objectContext) { - dataObject.setObjectContext(objectContext); - } -}