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
The following commit(s) were added to refs/heads/STABLE-4.2 by this push: new 1f7f2a672 CAY-2876 Memory leak in the ObjectStore 1f7f2a672 is described below commit 1f7f2a672e17efd28bd9fb1cf11db75ca41aa45f Author: Nikita Timofeev <stari...@users.noreply.github.com> AuthorDate: Wed Feb 26 16:54:52 2025 +0400 CAY-2876 Memory leak in the ObjectStore --- 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, 228 insertions(+), 82 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 536ed09fe..fcfdd19b7 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -22,6 +22,7 @@ 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 12cc2fede..e1c50d32e 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,6 +20,8 @@ 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; @@ -44,7 +46,11 @@ public class ObjectIdRelationshipHandlerTest { @Before public void setUp() throws Exception { - runtime = ServerRuntime.builder().addConfig("cayenne-lifecycle.xml").build(); + 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(); // 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 9b2b68de4..c62f95a3d 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,26 +65,24 @@ public class ObjectDiff extends NodeDiff { private Map<ArcOperation, ArcOperation> flatIds; private Map<ArcOperation, ArcOperation> phantomFks; - private Persistent object; + private final ObjectStorePersistentWrapper object; - ObjectDiff(final Persistent object) { + ObjectDiff(final ObjectStorePersistentWrapper object) { - super(object.getObjectId()); + super(object.dataObject().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.getObjectContext().getEntityResolver(); + EntityResolver entityResolver = object.dataObject().getObjectContext().getEntityResolver(); - this.entityName = object.getObjectId().getEntityName(); + this.entityName = object.dataObject().getObjectId().getEntityName(); this.classDescriptor = entityResolver.getClassDescriptor(entityName); - int state = object.getPersistenceState(); + int state = object.dataObject().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) { @@ -99,7 +97,7 @@ public class ObjectDiff extends NodeDiff { @Override public boolean visitAttribute(AttributeProperty property) { - snapshot.put(property.getName(), property.readProperty(object)); + snapshot.put(property.getName(), property.readProperty(object.dataObject())); return true; } @@ -114,8 +112,8 @@ public class ObjectDiff extends NodeDiff { // eagerly resolve optimistically locked relationships Object target = (lock && isUsedForLocking) - ? property.readProperty(object) - : property.readPropertyDirectly(object); + ? property.readProperty(object.dataObject()) + : property.readPropertyDirectly(object.dataObject()); if (target instanceof Persistent) { target = ((Persistent) target).getObjectId(); @@ -130,14 +128,14 @@ public class ObjectDiff extends NodeDiff { } Object getObject() { - return object; + return object.dataObject(); } ClassDescriptor getClassDescriptor() { // class descriptor is initiated in constructor, but is nullified on // serialization if (classDescriptor == null) { - EntityResolver entityResolver = object.getObjectContext().getEntityResolver(); + EntityResolver entityResolver = object.dataObject().getObjectContext().getEntityResolver(); this.classDescriptor = entityResolver.getClassDescriptor(entityName); } @@ -152,7 +150,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, propertyName); + Persistent target = (Persistent) ((Fault) value).resolveFault(object.dataObject(), propertyName); value = target != null ? target.getObjectId() : null; arcSnapshot.put(propertyName, value); @@ -167,7 +165,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, propertyName); + Persistent target = (Persistent) ((Fault) value).resolveFault(object.dataObject(), propertyName); value = target != null ? target.getObjectId() : null; currentArcSnapshot.put(propertyName, value); @@ -328,7 +326,7 @@ public class ObjectDiff extends NodeDiff { return false; } - int state = object.getPersistenceState(); + int state = object.dataObject().getPersistenceState(); if (state == PersistenceState.NEW || state == PersistenceState.DELETED) { return false; } @@ -342,7 +340,7 @@ public class ObjectDiff extends NodeDiff { public boolean visitAttribute(AttributeProperty property) { Object oldValue = snapshot.get(property.getName()); - Object newValue = property.readProperty(object); + Object newValue = property.readProperty(object.dataObject()); if (!property.equals(oldValue, newValue)) { modFound[0] = true; @@ -363,7 +361,7 @@ public class ObjectDiff extends NodeDiff { return true; } - Object newValue = property.readPropertyDirectly(object); + Object newValue = property.readPropertyDirectly(object.dataObject()); if (newValue instanceof Fault) { return true; } @@ -411,7 +409,7 @@ public class ObjectDiff extends NodeDiff { @Override public boolean visitAttribute(AttributeProperty property) { - Object newValue = property.readProperty(object); + Object newValue = property.readProperty(object.dataObject()); // 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 74b778c79..aa522f017 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,8 +53,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; /** * ObjectStore stores objects using their ObjectId as a key. It works as a dedicated @@ -66,6 +65,9 @@ import java.util.concurrent.ConcurrentHashMap; */ public class ObjectStore implements Serializable, SnapshotEventListener, GraphManager { + /** + * Actual content is ObjectId -> ObjectStorePersistentWrapper + */ protected Map<Object, Persistent> objectMap; protected Map<Object, ObjectDiff> changes; @@ -73,6 +75,7 @@ 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; @@ -138,7 +141,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa Collection<GraphDiff> getLifecycleEventInducedChanges() { return lifecycleEventInducedChanges != null ? lifecycleEventInducedChanges - : Collections.<GraphDiff>emptyList(); + : Collections.emptyList(); } void registerLifecycleEventInducedChange(GraphDiff diff) { @@ -166,10 +169,11 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa if (objectDiff == null) { - Persistent object = objectMap.get(nodeId); - if (object == null) { + ObjectStorePersistentWrapper persistentWrapper = (ObjectStorePersistentWrapper)objectMap.get(nodeId); + if (persistentWrapper == 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); @@ -201,7 +205,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa } } - objectDiff = new ObjectDiff(object); + objectDiff = new ObjectDiff(persistentWrapper); objectDiff.setDiffId(++currentDiffId); changes.put(nodeId, objectDiff); } @@ -213,13 +217,28 @@ 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() { - return objectMap.size(); + AtomicInteger counter = new AtomicInteger(); + objectMap.forEach((id, obj) -> { + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj; + if(wrapper.hasObject()){ + counter.incrementAndGet(); + } + }); + return counter.get(); } /** @@ -303,9 +322,6 @@ 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); @@ -391,7 +407,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa for (Object id : changes.keySet()) { - Persistent object = objectMap.get(id); + Persistent object = getUnwrapped(id); // assume that no new or deleted objects are present (as otherwise commit // wouldn't have been phantom). @@ -411,7 +427,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa // scan through changed objects, set persistence state to committed for (Object id : changes.keySet()) { - Persistent object = objectMap.get(id); + Persistent object = getUnwrapped(id); switch (object.getPersistenceState()) { case PersistenceState.DELETED: @@ -508,7 +524,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * Returns an iterator over the registered objects. */ public synchronized Iterator<Persistent> getObjectIterator() { - return objectMap.values().iterator(); + return new WrapperIterator(objectMap.values().iterator()); } /** @@ -530,8 +546,9 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa List<Persistent> filteredObjects = new ArrayList<>(); for (Persistent object : objectMap.values()) { - if (object.getPersistenceState() == state) { - filteredObjects.add(object); + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) object; + if (wrapper.hasObject() && object.getPersistenceState() == state) { + filteredObjects.add(wrapper.dataObject()); } } @@ -595,20 +612,24 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa if (object != null) { object.setObjectId((ObjectId) newId); - objectMap.put(newId, object); + 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; + }); 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); - } - } } /** @@ -619,7 +640,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 = objectMap.get(nodeId); + Persistent object = getUnwrapped(nodeId); if (object != null) { @@ -725,7 +746,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) objectMap.get(oid); + final DataObject object = (DataObject) getUnwrapped(oid); if (object == null || object.getPersistenceState() != PersistenceState.COMMITTED) { continue; @@ -779,7 +800,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) objectMap.get(nodeId); + DataObject object = (DataObject) getUnwrapped(nodeId); // no object, or HOLLOW object require no processing if (object != null) { @@ -858,12 +879,12 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa */ @Override public synchronized Object getNode(Object nodeId) { - return objectMap.get(nodeId); + return getUnwrapped(nodeId); } // non-synchronized version of getNode for private use final Object getNodeNoSync(Object nodeId) { - return objectMap.get(nodeId); + return getUnwrapped(nodeId); } /** @@ -874,7 +895,15 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa */ @Override public synchronized Collection<Object> registeredNodes() { - return new ArrayList<Object>(objectMap.values()); + List<Object> values = new ArrayList<>(objectMap.size()); + objectMap.forEach((id, persistent) + -> { + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) persistent; + if(wrapper.hasObject()) { + values.add(wrapper.dataObject()); + } + }); + return values; } /** @@ -882,7 +911,7 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa */ @Override public synchronized void registerNode(Object nodeId, Object nodeObject) { - objectMap.put(nodeId, (Persistent) nodeObject); + objectMap.put(nodeId, new ObjectStorePersistentWrapper((Persistent) nodeObject)); } /** @@ -993,47 +1022,32 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * @since 4.1 */ boolean hasFlattenedPath(ObjectId objectId, String path) { - if(trackedFlattenedPaths == null) { - return false; - } - return trackedFlattenedPaths - .getOrDefault(objectId, Collections.emptyMap()).containsKey(path); + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); + return wrapper.hasFlattenedPath(path); } /** * @since 4.2 */ public ObjectId getFlattenedId(ObjectId objectId, String path) { - if(trackedFlattenedPaths == null) { - return null; - } - - return trackedFlattenedPaths - .getOrDefault(objectId, Collections.emptyMap()).get(path); + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); + return wrapper.getFlattenedId(path); } /** * @since 4.2 */ public Collection<ObjectId> getFlattenedIds(ObjectId objectId) { - if(trackedFlattenedPaths == null) { - return Collections.emptyList(); - } - - return trackedFlattenedPaths - .getOrDefault(objectId, Collections.emptyMap()).values(); + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); + return wrapper.getFlattenedIds(); } /** * @since 4.2.1 */ public Map<String, ObjectId> getFlattenedPathIdMap(ObjectId objectId) { - if(trackedFlattenedPaths == null) { - return Collections.emptyMap(); - } - - return trackedFlattenedPaths - .getOrDefault(objectId, Collections.emptyMap()); + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId); + return wrapper.getFlattenedPathIdMap(); } /** @@ -1041,12 +1055,9 @@ public class ObjectStore implements Serializable, SnapshotEventListener, GraphMa * @since 4.1 */ public void markFlattenedPath(ObjectId objectId, String path, ObjectId id) { - if(trackedFlattenedPaths == null) { - trackedFlattenedPaths = new ConcurrentHashMap<>(); - } - trackedFlattenedPaths - .computeIfAbsent(objectId, o -> new ConcurrentHashMap<>()) - .put(path, id); + ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap + .computeIfAbsent(objectId, objId -> new ObjectStorePersistentWrapper(null)); + wrapper.markFlattenedPath(path, id); } // an ObjectIdQuery optimized for retrieval of multiple snapshots - it can be reset @@ -1090,4 +1101,29 @@ 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 new file mode 100644 index 000000000..8a7677966 --- /dev/null +++ b/cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStorePersistentWrapper.java @@ -0,0 +1,105 @@ +/***************************************************************** + * 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); + } +}