From c9e0e27e4385b5a61d212a4540387bce3b07d952 Mon Sep 17 00:00:00 2001 From: ntisseyre Date: Thu, 10 Aug 2023 10:18:23 -0500 Subject: [PATCH] Returning a copied collection to avoid corrupted iterator by multiple threads Signed-off-by: ntisseyre --- .../graphdb/transaction/StandardJanusGraphTx.java | 2 +- .../addedrelations/AddedRelationsContainer.java | 4 ++-- .../addedrelations/ConcurrentAddedRelations.java | 14 +++++++++++--- .../addedrelations/EmptyAddedRelations.java | 2 +- .../addedrelations/SimpleAddedRelations.java | 2 +- .../addedrelations/EmptyAddedRelationsTest.java | 4 ++-- 6 files changed, 18 insertions(+), 10 deletions(-) diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java index 939ce23962..361c7af983 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java @@ -1568,7 +1568,7 @@ public synchronized void commit() { } try { if (hasModifications()) { - graph.commit(addedRelations.getAll(), deletedRelations.values(), this); + graph.commit(addedRelations.getAllUnsafe(), deletedRelations.values(), this); } else { txHandle.commit(); } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/AddedRelationsContainer.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/AddedRelationsContainer.java index c0ece09253..52c74de3c4 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/AddedRelationsContainer.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/AddedRelationsContainer.java @@ -39,7 +39,7 @@ public interface AddedRelationsContainer { * of the transaction after there are no additional changes. Otherwise the behavior is non deterministic. * @return */ - Collection getAll(); + Collection getAllUnsafe(); /** * Clears the container which releases allocated memory. @@ -69,7 +69,7 @@ public boolean isEmpty() { } @Override - public Collection getAll() { + public Collection getAllUnsafe() { return Collections.emptyList(); } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/ConcurrentAddedRelations.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/ConcurrentAddedRelations.java index 78a8ef7544..32fb69827c 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/ConcurrentAddedRelations.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/ConcurrentAddedRelations.java @@ -15,8 +15,10 @@ package org.janusgraph.graphdb.transaction.addedrelations; import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; import org.janusgraph.graphdb.internal.InternalRelation; +import java.util.ArrayList; import java.util.Collection; /** @@ -37,11 +39,17 @@ public synchronized boolean remove(final InternalRelation relation) { @Override public synchronized Iterable getView(final Predicate filter) { - return super.getView(filter); + return copyView(super.getView(filter)); } @Override - public synchronized Collection getAll() { - return super.getAll(); + public synchronized Collection getAllUnsafe() { + return super.getAllUnsafe(); + } + + private Iterable copyView(Iterable currentView) { + ArrayList view = new ArrayList<>(); + Iterables.addAll(view, currentView); + return view; } } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelations.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelations.java index c139065120..0069daa67a 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelations.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelations.java @@ -52,7 +52,7 @@ public boolean isEmpty() { } @Override - public Collection getAll() { + public Collection getAllUnsafe() { return Collections.emptyList(); } diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/SimpleAddedRelations.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/SimpleAddedRelations.java index fcc534c9b5..afc2bbeb14 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/SimpleAddedRelations.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/addedrelations/SimpleAddedRelations.java @@ -62,7 +62,7 @@ public boolean isEmpty() { } @Override - public Collection getAll() { + public Collection getAllUnsafe() { return Collections.unmodifiableCollection(new AbstractCollection() { @Override @Nonnull diff --git a/janusgraph-test/src/test/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelationsTest.java b/janusgraph-test/src/test/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelationsTest.java index c3529962cc..01bafb70f4 100644 --- a/janusgraph-test/src/test/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelationsTest.java +++ b/janusgraph-test/src/test/java/org/janusgraph/graphdb/transaction/addedrelations/EmptyAddedRelationsTest.java @@ -41,8 +41,8 @@ public void isEmpty() { } @Test - public void getAllReturnsEmpty() { - assertTrue(EmptyAddedRelations.getInstance().getAll().isEmpty()); + public void getAllUnsafeReturnsEmpty() { + assertTrue(EmptyAddedRelations.getInstance().getAllUnsafe().isEmpty()); } @Test