Updated Branches: refs/heads/trunk 458ddd8f9 -> c5fe858e9
GIRAPH-675: Mutable edge iterator gets corrupted by calling vertex.getNumEdges() during iteration (apresta) Project: http://git-wip-us.apache.org/repos/asf/giraph/repo Commit: http://git-wip-us.apache.org/repos/asf/giraph/commit/c5fe858e Tree: http://git-wip-us.apache.org/repos/asf/giraph/tree/c5fe858e Diff: http://git-wip-us.apache.org/repos/asf/giraph/diff/c5fe858e Branch: refs/heads/trunk Commit: c5fe858e9cf7ccd5746aa1168414b42b79c4adc2 Parents: 458ddd8 Author: Alessandro Presta <[email protected]> Authored: Fri May 31 14:57:11 2013 -0700 Committer: Alessandro Presta <[email protected]> Committed: Fri May 31 15:34:47 2013 -0700 ---------------------------------------------------------------------- CHANGELOG | 3 ++ .../apache/giraph/edge/MutableEdgesIterable.java | 1 + .../apache/giraph/edge/MutableEdgesWrapper.java | 13 ++++++++- .../main/java/org/apache/giraph/graph/Vertex.java | 5 +++- .../apache/giraph/graph/TestVertexAndEdges.java | 21 ++++++++++++++- 5 files changed, 40 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/giraph/blob/c5fe858e/CHANGELOG ---------------------------------------------------------------------- diff --git a/CHANGELOG b/CHANGELOG index 66fdd69..99d6958 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,9 @@ Giraph Change Log Release 1.1.0 - unreleased + GIRAPH-675: Mutable edge iterator gets corrupted by calling + vertex.getNumEdges() during iteration (apresta) + GIRAPH-512: JavaDoc warnings (emre.aladag via nitay) GIRAPH-620: Website Documentation: How to use Hive I/O with Giraph (nitay) http://git-wip-us.apache.org/repos/asf/giraph/blob/c5fe858e/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesIterable.java ---------------------------------------------------------------------- diff --git a/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesIterable.java b/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesIterable.java index ca32a9f..ea55e29 100644 --- a/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesIterable.java +++ b/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesIterable.java @@ -96,6 +96,7 @@ public class MutableEdgesIterable<I extends WritableComparable, // Set the current edge to null, so that it's not added to the // new edges. mutableEdgesWrapper.setCurrentEdge(null); + mutableEdgesWrapper.decrementEdges(); } }; } http://git-wip-us.apache.org/repos/asf/giraph/blob/c5fe858e/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java ---------------------------------------------------------------------- diff --git a/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java b/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java index 529234d..4e2a2a2 100644 --- a/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java +++ b/giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java @@ -47,6 +47,8 @@ public class MutableEdgesWrapper<I extends WritableComparable, private final Iterator<Edge<I, E>> oldEdgesIterator; /** Last edge that was returned during iteration. */ private MutableEdge<I, E> currentEdge; + /** Number of edges. */ + private int numEdges; /** * Private constructor: instantiation happens through the {@code wrap()} @@ -59,6 +61,7 @@ public class MutableEdgesWrapper<I extends WritableComparable, OutEdges<I, E> newEdges) { oldEdgesIterator = oldEdges.iterator(); this.newEdges = newEdges; + numEdges = oldEdges.size(); } /** @@ -131,6 +134,14 @@ public class MutableEdgesWrapper<I extends WritableComparable, currentEdge = edge; } + /** + * Decrement the number of edges (to account for a deletion from the mutable + * iterator). + */ + public void decrementEdges() { + --numEdges; + } + @Override public void initialize(Iterable<Edge<I, E>> edges) { throw new IllegalStateException("initialize: MutableEdgesWrapper should " + @@ -161,7 +172,7 @@ public class MutableEdgesWrapper<I extends WritableComparable, @Override public int size() { - return unwrap().size(); + return numEdges; } @Override http://git-wip-us.apache.org/repos/asf/giraph/blob/c5fe858e/giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java ---------------------------------------------------------------------- diff --git a/giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java b/giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java index 82fbe0c..1241ae7 100644 --- a/giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java +++ b/giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java @@ -129,6 +129,8 @@ public class Vertex<I extends WritableComparable, * Note: edge objects returned by this iterable may be invalidated as soon * as the next element is requested. Thus, keeping a reference to an edge * almost always leads to undesired behavior. + * Accessing the edges with other methods (e.g., addEdge()) during iteration + * leads to undefined behavior. * * @return the out edges (sort order determined by subclass implementation). */ @@ -140,7 +142,8 @@ public class Vertex<I extends WritableComparable, * Get an iterable of out-edges that can be modified in-place. * This can mean changing the current edge value or removing the current edge * (by using the iterator version). - * Note: if + * Note: accessing the edges with other methods (e.g., addEdge()) during + * iteration leads to undefined behavior. * * @return An iterable of mutable out-edges */ http://git-wip-us.apache.org/repos/asf/giraph/blob/c5fe858e/giraph-core/src/test/java/org/apache/giraph/graph/TestVertexAndEdges.java ---------------------------------------------------------------------- diff --git a/giraph-core/src/test/java/org/apache/giraph/graph/TestVertexAndEdges.java b/giraph-core/src/test/java/org/apache/giraph/graph/TestVertexAndEdges.java index b6e17fd..d0a6c46 100644 --- a/giraph-core/src/test/java/org/apache/giraph/graph/TestVertexAndEdges.java +++ b/giraph-core/src/test/java/org/apache/giraph/graph/TestVertexAndEdges.java @@ -308,7 +308,6 @@ public class TestVertexAndEdges { int i = 2; for (MutableEdge<LongWritable, DoubleWritable> edge : vertex.getMutableEdges()) { - System.out.println(edge.toString()); if (i-- == 0) { break; } @@ -323,6 +322,26 @@ public class TestVertexAndEdges { } } assertEquals(5, vertex.getNumEdges()); + + // Calling size() during iteration shouldn't modify the data structure. + int iterations = 0; + for (MutableEdge<LongWritable, DoubleWritable> edge : vertex.getMutableEdges()) { + edge.setValue(new DoubleWritable(3)); + assertEquals(5, vertex.getNumEdges()); + ++iterations; + } + assertEquals(5, vertex.getNumEdges()); + assertEquals(5, iterations); + + // If we remove an edge after calling next(), size() should return the + // correct number of edges. + it = vertex.getMutableEdges().iterator(); + it.next(); + it.remove(); + assertEquals(4, vertex.getNumEdges()); + it.next(); + it.remove(); + assertEquals(3, vertex.getNumEdges()); } /**
