diff --git a/src/main/java/org/opentripplanner/routing/core/RoutingContext.java b/src/main/java/org/opentripplanner/routing/core/RoutingContext.java index c75a119b608..4ed53c7a5ac 100644 --- a/src/main/java/org/opentripplanner/routing/core/RoutingContext.java +++ b/src/main/java/org/opentripplanner/routing/core/RoutingContext.java @@ -141,7 +141,7 @@ private Set overlappingStreetEdges(Vertex u, Vertex v) { for (Edge e : Iterables.concat(u.getIncoming(), u.getOutgoing())) { uIds.add(e.getId()); } - + // Intesection of edge IDs between u and v. uIds.retainAll(vIds); Set overlappingIds = uIds; @@ -229,7 +229,6 @@ private RoutingContext(RoutingRequest routingRequest, Graph graph, Vertex from, else this.streetSpeedSnapshot = null; - Edge fromBackEdge = null; Edge toBackEdge = null; if (findPlaces) { @@ -290,7 +289,7 @@ private RoutingContext(RoutingRequest routingRequest, Graph graph, Vertex from, makePartialEdgeAlong(pse, fromStreetVertex, toStreetVertex); } } - + if (opt.startingTransitStopId != null) { Stop stop = graph.index.stopForId.get(opt.startingTransitStopId); TransitStop tstop = graph.index.stopVertexForStop.get(stop); @@ -404,27 +403,7 @@ public boolean isWheelchairAccessible(Vertex v) { * for garbage collection. */ public void destroy() { - disposeTemporaryStart(fromVertex, null); - disposeTemporaryEnd(toVertex, null); - } - - private static void disposeTemporaryStart(Vertex v, Edge incoming) { - if (v instanceof TemporaryVertex) { - for (Edge edge : v.getOutgoing()) { - disposeTemporaryStart(edge.getToVertex(), edge); - } - } else if (incoming != null) { - v.removeIncoming(incoming); - } - } - - private static void disposeTemporaryEnd(Vertex v, Edge outgoing) { - if (v instanceof TemporaryVertex) { - for (Edge edge : v.getIncoming()) { - disposeTemporaryEnd(edge.getFromVertex(), edge); - } - } else if (outgoing != null) { - v.removeOutgoing(outgoing); - } + TemporaryVertex.dispose(fromVertex); + TemporaryVertex.dispose(toVertex); } -} +} \ No newline at end of file diff --git a/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertex.java b/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertex.java index e0891f18c18..438937c1b64 100644 --- a/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertex.java +++ b/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertex.java @@ -1,8 +1,26 @@ package org.opentripplanner.routing.vertextype; +import org.opentripplanner.routing.graph.Vertex; + /** - * Marker interface for temporary vertices + * Marker interface for temporary vertices. + *

+ * Remember to use the {@link #dispose(Vertex)} to delete the temporary vertex + * from the main graph after use. */ public interface TemporaryVertex { boolean isEndVertex(); + + /** + * This method traverse the subgraph of temporary vertices, and cuts that subgraph off from the + * main graph at each point it encounters a non-temporary vertexes. OTP then holds no + * references to the temporary subgraph and it is garbage collected. + *

+ * Note! If the {@code vertex} is NOT a TemporaryVertex the method returns. No action taken. + * + * @param vertex Vertex part of the temporary part of the graph. + */ + static void dispose(Vertex vertex) { + TemporaryVertexDispose.dispose(vertex); + } } diff --git a/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertexDispose.java b/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertexDispose.java new file mode 100644 index 00000000000..9fa8976a352 --- /dev/null +++ b/src/main/java/org/opentripplanner/routing/vertextype/TemporaryVertexDispose.java @@ -0,0 +1,117 @@ +package org.opentripplanner.routing.vertextype; + +import org.opentripplanner.routing.graph.Edge; +import org.opentripplanner.routing.graph.Vertex; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +/** + * This is a utility class used to remove a temporary subgraph from the manin graph. + * It traverse the subgraph of temporary vertices, and cuts that subgraph off from the + * main graph at each point it encounters a non-temporary vertexes. + *

+ * OTP then holds no references to the temporary subgraph and it is garbage collected. + *

+ * The static {@link #dispose(Vertex)} utility method is the only way to access the logic, + * hence preventing this class from reuse. This make the class thread safe, and simplify + * the implementation. + */ +class TemporaryVertexDispose { + /** + * A list of all Vertexes not jet processed. + */ + private List todo = new ArrayList<>(); + + /** + * Processed vertexes. To prevent looping and processing the same vertex twice we + * keep all processed vertexes in the 'done' set. + */ + private Set done = new HashSet<>(); + + /** Intentionally private constructor to prevent instantiation outside of the class. */ + private TemporaryVertexDispose(Vertex tempVertex) { + todo.add(tempVertex); + } + + /** + * Create an instance and dispose temporary subgraph. + * @param tempVertex any temporary vertex part of the temporary subgrap. + */ + static void dispose(Vertex tempVertex) { + if(tempVertex instanceof TemporaryVertex) { + new TemporaryVertexDispose(tempVertex).dispose(); + } + } + + + /* private methods */ + + private void dispose() { + // Add all connected vertexes to the TODO_list and disconnect all + // main graph vertexes. We use a loop and not recursion to avoid + // stack overflow in the case of deep temporary graphs. + while (!todo.isEmpty()) { + Vertex current = next(); + if(isNotAlreadyProcessed(current)) { + for (Edge edge : current.getOutgoing()) { + disposeVertex(edge.getToVertex(), edge, true); + } + for (Edge edge : current.getIncoming()) { + disposeVertex(edge.getFromVertex(), edge, false); + } + done.add(current); + } + } + } + + /** + * Add the temporary vertex to processing queue OR disconnect edge from vertex if + * vertex is part of the main graph. + * + * @param v the vertex to dispose + * @param connectedEdge the connected temporary edge + * @param incoming true if the edge is an incoming edge, false if it is an outgoing edge + */ + private void disposeVertex(Vertex v, Edge connectedEdge, boolean incoming) { + if(v instanceof TemporaryVertex) { + addVertexToProcessTodoList(v); + } + else { + removeEdgeFromMainGraphVertex(v, connectedEdge, incoming); + } + } + + /** + * We have reached a NONE temporary Vertex and need to remove the temporary `connectedEdge` + * from the Vertex part of the main graph. + * + * @param v the vertex part of the main graph + * @param connectedEdge the connected temporary edge to be removed + * @param incoming true if the edge is an incoming edge, false if it is an outgoing edge + */ + private void removeEdgeFromMainGraphVertex(Vertex v, Edge connectedEdge, boolean incoming) { + if(incoming) { + v.removeIncoming(connectedEdge); + } + else { + v.removeOutgoing(connectedEdge); + } + } + + private void addVertexToProcessTodoList(Vertex v) { + if(isNotAlreadyProcessed(v)) { + todo.add(v); + } + } + + private boolean isNotAlreadyProcessed(Vertex v) { + return !done.contains(v); + } + + private Vertex next() { + return todo.remove(todo.size()-1); + } +} diff --git a/src/test/java/org/opentripplanner/routing/core/RoutingContext_Destroy_Test.java b/src/test/java/org/opentripplanner/routing/core/RoutingContext_Destroy_Test.java index a6b5df8cf31..8e43ef3ede5 100644 --- a/src/test/java/org/opentripplanner/routing/core/RoutingContext_Destroy_Test.java +++ b/src/test/java/org/opentripplanner/routing/core/RoutingContext_Destroy_Test.java @@ -29,11 +29,6 @@ import static org.junit.Assert.assertTrue; public class RoutingContext_Destroy_Test { - - private static final boolean ARRIVE_BY_TRUE = true; - - private static final boolean ARRIVE_BY_FALSE = false; - private final GeometryFactory gf = GeometryUtils.getGeometryFactory(); private RoutingRequest request; private RoutingContext subject; @@ -65,17 +60,10 @@ public class RoutingContext_Destroy_Test { } @Test public void temporary_changes_is_removed_from_graph_on_context_destroy() { - // try arriveBy set to both true and false - temporary_changes_is_removed_from_graph_on_context_destroy(ARRIVE_BY_TRUE); - temporary_changes_is_removed_from_graph_on_context_destroy(ARRIVE_BY_FALSE); - } - - private void temporary_changes_is_removed_from_graph_on_context_destroy(boolean arriveBy) { // Given - A request request = new RoutingRequest(); request.from = from; request.to = to; - request.arriveBy = arriveBy; // When - the context is created subject = new RoutingContext(request, g); @@ -151,9 +139,8 @@ private static > T findAllReachableVertexes(Vertex return list; } - private void assertVertexEdgeIsNotReferencingTemporaryElements(Vertex source, Edge e, - Vertex v) { - String sourceName = source.getName(); + private void assertVertexEdgeIsNotReferencingTemporaryElements(Vertex src, Edge e, Vertex v) { + String sourceName = src.getName(); assertFalse(sourceName + " -> " + e.getName(), e instanceof TemporaryEdge); assertFalse(sourceName + " -> " + v.getName(), v instanceof TemporaryVertex); } diff --git a/src/test/java/org/opentripplanner/routing/vertextype/TemporaryVertexDisposeTest.java b/src/test/java/org/opentripplanner/routing/vertextype/TemporaryVertexDisposeTest.java new file mode 100644 index 00000000000..396129bf65d --- /dev/null +++ b/src/test/java/org/opentripplanner/routing/vertextype/TemporaryVertexDisposeTest.java @@ -0,0 +1,227 @@ +package org.opentripplanner.routing.vertextype; + +import org.junit.Test; +import org.opentripplanner.routing.edgetype.FreeEdge; +import org.opentripplanner.routing.graph.Vertex; + +import static org.junit.Assert.assertEquals; + +public class TemporaryVertexDisposeTest { + + private static final double ANY_LOC = 1; + + // Given a very simple graph: A -> B + private final Vertex a = new V("A"); + private final Vertex b = new V("B"); + { + edge(a, b); + assertOriginalGraphIsIntact(); + } + + @Test public void disposeNormalCase() { + // Given a temporary vertex 'origin' and 'destination' connected to graph + Vertex origin = new TempVertex("origin"); + Vertex destination = new TempVertex("dest"); + edge(origin, a); + edge(b, destination); + + // Then before we dispose temporary vertexes + assertEquals("[origin->A]", a.getIncoming().toString()); + assertEquals("[B->dest]", b.getOutgoing().toString()); + + // When + TemporaryVertex.dispose(origin); + TemporaryVertex.dispose(destination); + + // Then + assertOriginalGraphIsIntact(); + } + + @Test public void disposeShouldNotDeleteOtherIncomingEdges() { + // Given a temporary vertex 'origin' connected to B - has other incoming edges + Vertex origin = new TempVertex("origin"); + Vertex otherTemp = new TempVertex("OT"); + edge(origin, b); + edge(otherTemp, b); + + // Then before we dispose temporary vertexes + assertEquals("[A->B, origin->B, OT->B]", b.getIncoming().toString()); + // When + TemporaryVertex.dispose(origin); + + // Then B is back to normal + assertEquals("[A->B, OT->B]", b.getIncoming().toString()); + } + + @Test public void disposeShouldNotDeleteOtherOutgoingEdges() { + // Given a temporary vertex 'destination' connected from A - with one other outgoing edge + Vertex destination = new TempVertex("destination"); + Vertex otherTemp = new TempVertex("OT"); + edge(a, destination); + edge(a, otherTemp); + + // Then before we dispose temporary vertexes + assertEquals("[A->B, A->destination, A->OT]", a.getOutgoing().toString()); + + // When + TemporaryVertex.dispose(destination); + + // A is back to normal + assertEquals("[A->B, A->OT]", a.getOutgoing().toString()); + } + + + @Test public void disposeShouldHandleLoopsInTemporaryPath() { + Vertex x = new TempVertex("x"); + Vertex y = new TempVertex("y"); + Vertex z = new TempVertex("z"); + + edge(x, y); + edge(y, z); + edge(z, x); + // Add some random links the the main graph + edge(x, a); + edge(b, y); + edge(z, a); + + // When + TemporaryVertex.dispose(x); + + // Then do return without stack overflow and: + assertOriginalGraphIsIntact(); + } + + /** + * Verify a complex temporary path is disposed. The temporary graph is connected to the + * main graph in both directions (in/out) from many places (temp. vertexes). + *

+ * The temporary part of the graph do NOT contain any loops. + */ + @Test public void disposeTemporaryVertexesWithComplexPaths() { + + TempVertex x = new TempVertex("x"); + Vertex y = new TempVertex("y"); + Vertex z = new TempVertex("z"); + Vertex q = new TempVertex("q"); + + // A and B are connected both ways A->B and B->A (A->B is done in the class init) + edge(b, a); + + // All temporary vertexes are connected in a chain + edge(x, y); + edge(y, z); + edge(z, q); + + // And they are connected to the graph in many ways - no loops + edge(x, a); + edge(y, b); + edge(z, a); + edge(q, a); + edge(q, b); + + // Then before we dispose temporary vertexes + assertEquals("[B->A, x->A, z->A, q->A]", a.getIncoming().toString()); + assertEquals("[A->B, y->B, q->B]", b.getIncoming().toString()); + + // When + TemporaryVertex.dispose(x); + + // Then + assertEquals("[B->A]", a.getIncoming().toString()); + assertEquals("[A->B]", b.getIncoming().toString()); + + + // Destination, connect to A and B outgoing - we can reuse the temp structure + edge(a, x); + edge(b, y); + edge(a, z); + edge(a, q); + edge(b, q); + + assertEquals("[A->B, A->x, A->z, A->q]", a.getOutgoing().toString()); + assertEquals("[B->A, B->y, B->q]", b.getOutgoing().toString()); + + // When + TemporaryVertex.dispose(x); + + // Then + assertEquals("[B->A]", a.getIncoming().toString()); + assertEquals("[A->B]", b.getIncoming().toString()); + } + + /** + * We should be able to delete an alternative path/loop created in the graph like: + *

+ * A -> x -> y -> B + *

+ * Where 'x' and 'y' are temporary vertexes + */ + @Test public void disposeTemporaryAlternativePath() { + Vertex x = new TempVertex("x"); + Vertex y = new TempVertex("y"); + + // Make a new loop from 'A' via 'x' and 'y' to 'B' + edge(a, x); + edge(x, y); + edge(y, b); + + // Then before we dispose temporary vertexes + assertEquals("[A->B, A->x]", a.getOutgoing().toString()); + assertEquals("[A->B, y->B]", b.getIncoming().toString()); + + // When + TemporaryVertex.dispose(x); + + // Then + assertOriginalGraphIsIntact(); + } + + /* private test helper classes */ + + private void assertOriginalGraphIsIntact() { + assertEquals("[]", a.getIncoming().toString()); + assertEquals("[A->B]", a.getOutgoing().toString()); + assertEquals("[A->B]", b.getIncoming().toString()); + assertEquals("[]", b.getOutgoing().toString()); + } + + + static class V extends Vertex { + private V(String label) { + super(null, label, ANY_LOC, ANY_LOC); + } + + @Override public String toString() { + return getLabel(); + } + } + + static class TempVertex extends Vertex implements TemporaryVertex { + private TempVertex(String label) { + super(null, label, ANY_LOC, ANY_LOC); + } + + @Override public boolean isEndVertex() { + throw new IllegalStateException("The `isEndVertex` is not used by dispose logic."); + } + + @Override public String toString() { + return getLabel(); + } + } + + // Factory method to create an edge + private static void edge(Vertex a, Vertex b) { + new E(a, b); + } + + static class E extends FreeEdge { + private E(Vertex from, Vertex to) { + super(from, to); + } + + @Override public String toString() { + return getFromVertex().toString() + "->" + getToVertex(); + } + } +} \ No newline at end of file