From beffc2723bcc9b4219fc0b915f27542b0c159af9 Mon Sep 17 00:00:00 2001 From: Jussi Koskela Date: Wed, 2 Sep 2020 12:17:31 +0300 Subject: [PATCH] Fixed multiple issues causing dangling references to discarded queries gitlab #594 Change-Id: Ia32190681ff551edfe6b56151c4fcb03349454ba --- .../simantics/db/impl/query/CacheEntry.java | 1 + .../db/impl/query/CacheEntryBase.java | 26 ++++++- .../db/impl/query/ExternalReadEntry.java | 10 +-- .../db/impl/query/QueryCollectorImpl.java | 1 + .../db/impl/query/QueryIdentityHash.java | 6 +- .../db/impl/query/QueryIdentityHashSet.java | 20 ++++- .../db/impl/query/QueryListening.java | 2 +- .../server/request/DocumentRequest.java | 75 ++++++++++--------- .../document/server/request/NodesRequest.java | 66 ++++++++-------- 9 files changed, 134 insertions(+), 73 deletions(-) diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntry.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntry.java index 1cdcbde9d..21369d793 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntry.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntry.java @@ -40,6 +40,7 @@ public abstract class CacheEntry { abstract Query getQuery(); abstract CacheEntry pruneFirstParents(); + abstract void pruneParentSet(); abstract void removeParent(CacheEntry entry); abstract void addParent(CacheEntry entry); abstract boolean hasParents(); diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntryBase.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntryBase.java index e79f99dea..74e39ab95 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntryBase.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/CacheEntryBase.java @@ -277,7 +277,31 @@ public abstract class CacheEntryBase extends CacheEntry { } } - + + @Override + void pruneParentSet() { + // First parent is discarded => look for more parents + if(p2OrParents instanceof QueryIdentityHashSet) { + + QueryIdentityHashSet set = (QueryIdentityHashSet)p2OrParents; + set.removeDiscardedReally(); + if(set.isEmpty()) p2OrParents = null; + + } else if(p2OrParents instanceof CacheEntry) { + + CacheEntry entry = (CacheEntry)p2OrParents; + if(entry.isDiscarded()) { + // Second entry is also discarded => all empty + p2OrParents = null; + } + + } else { + + // Nothing left + + } + } + @Override final public void removeParent(CacheEntry entry) { diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/ExternalReadEntry.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/ExternalReadEntry.java index d04404339..4b2269d52 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/ExternalReadEntry.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/ExternalReadEntry.java @@ -28,7 +28,7 @@ final public class ExternalReadEntry extends CacheEntryBase final LinkedList items = new LinkedList(); protected ExternalRead id; - protected ReadGraphImpl graph; + protected QueryProcessor processor; protected boolean registered = false; @Override @@ -49,7 +49,7 @@ final public class ExternalReadEntry extends CacheEntryBase public void discard() { id.unregistered(); id = null; - graph = null; + processor = null; super.discard(); } @@ -65,7 +65,7 @@ final public class ExternalReadEntry extends CacheEntryBase public ExternalReadEntry(ExternalRead request, ReadGraphImpl graph) { assert request != null; this.id = request; - this.graph = graph; + this.processor = graph.processor; } @Override @@ -213,7 +213,7 @@ final public class ExternalReadEntry extends CacheEntryBase synchronized(items) { items.addLast(result); - graph.processor.updatePrimitive(id); + processor.updatePrimitive(id); // TODO: implement flags/logic in ExternalRead to state that all but the latest request result can be evaporated // In some cases where data is produced really fast this might be necessary but currently this queueing will do. } @@ -229,7 +229,7 @@ final public class ExternalReadEntry extends CacheEntryBase @Override public boolean isDisposed() { - return registered && (isDiscarded() || !graph.processor.isBound(this)); + return registered && (isDiscarded() || !processor.isBound(this)); } } diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryCollectorImpl.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryCollectorImpl.java index 0b6f6ef83..e6dc252c4 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryCollectorImpl.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryCollectorImpl.java @@ -148,6 +148,7 @@ class QueryCollectorImpl implements QueryProcessor.QueryCollector { } else { + entry.pruneParentSet(); support.setLevel(entry, parent.getLevel() + 1); } diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHash.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHash.java index c2b21444f..d7e9e4b53 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHash.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHash.java @@ -227,7 +227,11 @@ abstract public class QueryIdentityHash extends THash { // TODO Auto-generated method stub return null; } - + + @Override + void pruneParentSet() { + } + }; /** diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHashSet.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHashSet.java index 3a7eb6182..ac2602fcb 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHashSet.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryIdentityHashSet.java @@ -147,7 +147,25 @@ final public class QueryIdentityHashSet extends QueryIdentityHash implements Ite } } - + + final public void removeDiscardedReally() { + + tempDisableAutoCompaction(); + try { + + for(int i=0;i<_set.length;i++) { + CacheEntry entry = _set[i]; + if(entry != null && REMOVED != entry) { + if(entry.isDiscarded()) removeAt(i); + } + } + + } finally { + reenableAutoCompaction(false); + } + + } + /** * Creates an iterator over the values of the set. The iterator * supports element deletion. diff --git a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryListening.java b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryListening.java index e3bd43741..a26460a47 100644 --- a/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryListening.java +++ b/bundles/org.simantics.db.impl/src/org/simantics/db/impl/query/QueryListening.java @@ -293,7 +293,7 @@ public class QueryListening { if(base == null) return; consumer.accept(() -> { - ListenerEntry entry = addedEntries.get(base); + ListenerEntry entry = addedEntries.remove(base); if(entry != null) entry.setLastKnown(result); }); diff --git a/bundles/org.simantics.document.server/src/org/simantics/document/server/request/DocumentRequest.java b/bundles/org.simantics.document.server/src/org/simantics/document/server/request/DocumentRequest.java index 8acc2f0d1..5b76cd344 100644 --- a/bundles/org.simantics.document.server/src/org/simantics/document/server/request/DocumentRequest.java +++ b/bundles/org.simantics.document.server/src/org/simantics/document/server/request/DocumentRequest.java @@ -1,6 +1,7 @@ package org.simantics.document.server.request; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.HashSet; @@ -10,7 +11,7 @@ import java.util.Set; import org.simantics.db.AsyncReadGraph; import org.simantics.db.ReadGraph; import org.simantics.db.common.procedure.adapter.TransientCacheAsyncListener; -import org.simantics.db.common.request.AsyncReadRequest; +import org.simantics.db.common.request.UnaryAsyncRead; import org.simantics.db.exception.DatabaseException; import org.simantics.db.layer0.request.VariableRead; import org.simantics.db.layer0.variable.Variable; @@ -28,49 +29,55 @@ public class DocumentRequest extends VariableRead> { super(var); } - @Override - public List perform(ReadGraph graph) throws DatabaseException { - - long s = System.nanoTime(); - - Set nodes = graph.syncRequest(new NodesRequest(variable), TransientCacheAsyncListener.>instance()); - HashSet rs = new HashSet(); // result - if(nodes.isEmpty()) { - return Collections.emptyList(); - } + static class CollectNodesRequest extends UnaryAsyncRead, Collection> { - if(PROFILE) { - long dura = System.nanoTime()-s; - System.err.println("DocumentRequest1 " + System.identityHashCode(this) + " in " + 1e-6*dura + "ms. " + variable.getURI(graph)); + public CollectNodesRequest(Collection nodes) { + super(nodes); } - graph.syncRequest(new AsyncReadRequest() { + @Override + public void perform(AsyncReadGraph graph, AsyncProcedure> procedure) { + HashSet rs = new HashSet(); // result - @Override - public void run(AsyncReadGraph graph) throws DatabaseException { + for(Variable node : parameter) { + graph.asyncRequest(new NodeRequest(node), new AsyncProcedure () { - for(Variable node : nodes) { - graph.asyncRequest(new NodeRequest(node), new AsyncProcedure () { - - @Override - public void execute(AsyncReadGraph graph, JSONObject result) { - synchronized (rs) { - rs.add(result); - } + @Override + public void execute(AsyncReadGraph graph, JSONObject result) { + synchronized(rs) { + rs.add(result); } + } - @Override - public void exception(AsyncReadGraph graph, Throwable throwable) { - } + @Override + public void exception(AsyncReadGraph graph, Throwable throwable) { + } + + }); - }); - - } - } - - }); + procedure.execute(graph, rs); + + } + + } + + @Override + public List perform(ReadGraph graph) throws DatabaseException { + + long s = System.nanoTime(); + + Set nodes = graph.syncRequest(new NodesRequest(variable), TransientCacheAsyncListener.>instance()); + if(nodes.isEmpty()) { + return Collections.emptyList(); + } + + if(PROFILE) { + long dura = System.nanoTime()-s; + System.err.println("DocumentRequest1 " + System.identityHashCode(this) + " in " + 1e-6*dura + "ms. " + variable.getURI(graph)); + } + Collection rs = graph.syncRequest(new CollectNodesRequest(nodes)); if(PROFILE) { long dura = System.nanoTime()-s; diff --git a/bundles/org.simantics.document.server/src/org/simantics/document/server/request/NodesRequest.java b/bundles/org.simantics.document.server/src/org/simantics/document/server/request/NodesRequest.java index 99526368e..133264e29 100644 --- a/bundles/org.simantics.document.server/src/org/simantics/document/server/request/NodesRequest.java +++ b/bundles/org.simantics.document.server/src/org/simantics/document/server/request/NodesRequest.java @@ -2,11 +2,12 @@ package org.simantics.document.server.request; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.Set; import org.simantics.db.AsyncReadGraph; import org.simantics.db.ReadGraph; -import org.simantics.db.common.request.AsyncReadRequest; +import org.simantics.db.common.request.UnaryAsyncRead; import org.simantics.db.exception.DatabaseException; import org.simantics.db.layer0.request.VariableChildren; import org.simantics.db.layer0.request.VariableRead; @@ -16,52 +17,57 @@ import org.simantics.structural.stubs.StructuralResource2; import org.simantics.utils.threads.logger.ITask; import org.simantics.utils.threads.logger.ThreadLogger; -import gnu.trove.set.hash.THashSet; - public class NodesRequest extends VariableRead> { public NodesRequest(Variable var) { super(var); } - @Override - public Set perform(ReadGraph graph) throws DatabaseException { + static class CollectNodesRequest2 extends UnaryAsyncRead, Set> { - ITask task = DocumentRequest.PROFILE ? ThreadLogger.task(this) : null; + public CollectNodesRequest2(Collection nodes) { + super(nodes); + } - StructuralResource2.getInstance(graph); - if(variable == null) - return Collections.emptySet(); + @Override + public void perform(AsyncReadGraph graph, AsyncProcedure> procedure) { + HashSet rs = new HashSet(); // result - Set nodes = new THashSet(); + for(Variable node : parameter) { + graph.asyncRequest(new NodesRequest2(node), new AsyncProcedure> () { - Collection children = graph.syncRequest(new VariableChildren(variable)); + @Override + public void execute(AsyncReadGraph graph, Set result) { + synchronized(rs) { + rs.addAll(result); + } + } - graph.syncRequest(new AsyncReadRequest() { + @Override + public void exception(AsyncReadGraph graph, Throwable throwable) { + } - @Override - public void run(AsyncReadGraph graph) throws DatabaseException { + }); - for(Variable child : children) { - graph.asyncRequest(new NodesRequest2(child), new AsyncProcedure>() { + } + procedure.execute(graph, rs); - @Override - public void execute(AsyncReadGraph graph, Set result) { - synchronized(nodes) { - nodes.addAll(result); - } - } + } - @Override - public void exception(AsyncReadGraph graph, Throwable throwable) { - } - - }); - } + } - } + @Override + public Set perform(ReadGraph graph) throws DatabaseException { + + ITask task = DocumentRequest.PROFILE ? ThreadLogger.task(this) : null; + + StructuralResource2.getInstance(graph); + if(variable == null) + return Collections.emptySet(); + + Collection children = graph.syncRequest(new VariableChildren(variable)); - }); + Set nodes = graph.syncRequest(new CollectNodesRequest2(children)); if(DocumentRequest.PROFILE) task.finish(); -- 2.47.1