Skip to content

Commit

Permalink
PathService implementations that used to take an SPTService object in…
Browse files Browse the repository at this point in the history
… their constructor now take a SPTServiceFactory instead, which is used to instantiate a new PathService every time getPath() is called. As instances of SPTService could contain algorithm state, PathService using the same instance across simultaneous calls to getPath wasn't threadsafe
  • Loading branch information
bmander committed Nov 13, 2014
1 parent 237adba commit 84c9e0a
Show file tree
Hide file tree
Showing 12 changed files with 97 additions and 48 deletions.
Expand Up @@ -22,6 +22,7 @@ the License, or (at your option) any later version.
import org.opentripplanner.common.geometry.IsolineBuilder.ZMetric;
import org.opentripplanner.common.geometry.ZSampleGrid;
import org.opentripplanner.routing.core.RoutingRequest;
import org.opentripplanner.routing.impl.SPTServiceFactory;
import org.opentripplanner.routing.services.GraphService;
import org.opentripplanner.routing.services.SPTService;
import org.slf4j.Logger;
Expand All @@ -38,12 +39,12 @@ public class IsoChroneSPTRendererAccSampling implements IsoChroneSPTRenderer {
.getLogger(IsoChroneSPTRendererAccSampling.class);

private GraphService graphService;
private SPTService sptService;
private SPTServiceFactory sptServiceFactory;
private SampleGridRenderer sampleGridRenderer;

public IsoChroneSPTRendererAccSampling(GraphService graphService, SPTService sptService, SampleGridRenderer sampleGridRenderer) {
public IsoChroneSPTRendererAccSampling(GraphService graphService, SPTServiceFactory sptServiceFactory, SampleGridRenderer sampleGridRenderer) {
this.graphService = graphService;
this.sptService = sptService;
this.sptServiceFactory = sptServiceFactory;
this.sampleGridRenderer = sampleGridRenderer;
}

Expand Down
Expand Up @@ -16,6 +16,7 @@ the License, or (at your option) any later version.
import javax.annotation.PostConstruct;

import org.opentripplanner.routing.core.RoutingRequest;
import org.opentripplanner.routing.impl.SPTServiceFactory;
import org.opentripplanner.routing.services.GraphService;
import org.opentripplanner.routing.services.SPTService;
import org.opentripplanner.routing.spt.ShortestPathTree;
Expand All @@ -30,12 +31,12 @@ public class SPTCache extends CacheLoader<RoutingRequest, ShortestPathTree> {

private static final Logger LOG = LoggerFactory.getLogger(SPTCache.class);

private SPTService sptService;
private SPTServiceFactory sptServiceFactory;

private GraphService graphService;

public SPTCache(SPTService sptService, GraphService graphService) {
this.sptService = sptService;
public SPTCache(SPTServiceFactory sptServiceFactory, GraphService graphService) {
this.sptServiceFactory = sptServiceFactory;
this.graphService = graphService;
this.sptCache = CacheBuilder.newBuilder()
.concurrencyLevel(concurrency)
Expand All @@ -53,7 +54,7 @@ public ShortestPathTree load(RoutingRequest req) throws Exception {
LOG.debug("spt cache miss : {}", req);
req.setRoutingContext(graphService.getGraph());
long t0 = System.currentTimeMillis();
ShortestPathTree spt = sptService.getShortestPathTree(req);
ShortestPathTree spt = sptServiceFactory.instantiate().getShortestPathTree(req);
long t1 = System.currentTimeMillis();
LOG.debug("calculated spt in {}msec", (int) (t1 - t0));
req.cleanup();
Expand Down
Expand Up @@ -27,6 +27,7 @@ the License, or (at your option) any later version.
import org.opentripplanner.routing.core.State;
import org.opentripplanner.routing.edgetype.StreetEdge;
import org.opentripplanner.routing.graph.Edge;
import org.opentripplanner.routing.impl.SPTServiceFactory;
import org.opentripplanner.routing.services.GraphService;
import org.opentripplanner.routing.services.SPTService;
import org.opentripplanner.routing.spt.SPTWalker;
Expand Down Expand Up @@ -54,11 +55,11 @@ public class SampleGridRenderer {
private static final Logger LOG = LoggerFactory.getLogger(SampleGridRenderer.class);

private GraphService graphService;
private SPTService sptService;
private SPTServiceFactory sptServiceFactory;

public SampleGridRenderer(GraphService graphService, SPTService sptService) {
public SampleGridRenderer(GraphService graphService, SPTServiceFactory sptServiceFactory) {
this.graphService = graphService;
this.sptService = sptService;
this.sptServiceFactory = sptServiceFactory;
}

/**
Expand All @@ -78,7 +79,7 @@ public ZSampleGrid<WTWD> getSampleGrid(SampleGridRequest spgRequest, RoutingRequ
+ (sptRequest.arriveBy ? -spgRequest.maxTimeSec - tOvershot : spgRequest.maxTimeSec + tOvershot));
sptRequest.batch = (true);
sptRequest.setRoutingContext(graphService.getGraph(sptRequest.routerId));
final ShortestPathTree spt = sptService.getShortestPathTree(sptRequest);
final ShortestPathTree spt = sptServiceFactory.instantiate().getShortestPathTree(sptRequest);

// 3. Create a sample grid based on the SPT.
long t1 = System.currentTimeMillis();
Expand Down
@@ -0,0 +1,24 @@
package org.opentripplanner.routing.impl;

import org.opentripplanner.routing.algorithm.GenericAStar;
import org.opentripplanner.routing.services.SPTService;
import org.opentripplanner.visualizer.VisualTraverseVisitor;

public class GenericAStarFactory implements SPTServiceFactory{

private VisualTraverseVisitor traverseVisitor=null;

@Override
public SPTService instantiate() {
GenericAStar ret = new GenericAStar();
if(traverseVisitor!=null){
ret.setTraverseVisitor(traverseVisitor);
}
return ret;
}

public void setTraverseVisitor(VisualTraverseVisitor visitor) {
this.traverseVisitor = visitor;
}

}
Expand Up @@ -68,11 +68,11 @@ public class LongDistancePathService implements PathService {
private static final double CLAMP_MAX_WALK = 15000;

private GraphService graphService;
private SPTService sptService;
private SPTServiceFactory sptServiceFactory;

public LongDistancePathService(GraphService graphService, SPTService sptService) {
public LongDistancePathService(GraphService graphService, SPTServiceFactory sptServiceFactory) {
this.graphService = graphService;
this.sptService = sptService;
this.sptServiceFactory = sptServiceFactory;
}

public double timeout = 0; // seconds
Expand All @@ -86,6 +86,8 @@ public List<GraphPath> getPaths(RoutingRequest options) {
LOG.error("PathService was passed a null routing request.");
return null;
}

SPTService sptService = this.sptServiceFactory.instantiate();

if (options.rctx == null) {
options.setRoutingContext(graphService.getGraph(options.routerId));
Expand Down
Expand Up @@ -35,15 +35,15 @@ public class ParetoPathService implements PathService {
private static final Logger LOG = LoggerFactory.getLogger(ParetoPathService.class);

private GraphService graphService;
private SPTService sptService;
private SPTServiceFactory sptServiceFactory;

private SPTVisitor sptVisitor = null;

private double timeout = 0; // seconds

public ParetoPathService(GraphService gs, GenericAStar spts) {
public ParetoPathService(GraphService gs, SPTServiceFactory spts) {
this.graphService = gs;
this.sptService = spts;
this.sptServiceFactory = spts;
}

/** Give up on searching for itineraries after this many seconds have elapsed. */
Expand All @@ -57,6 +57,8 @@ public void setSPTVisitor(SPTVisitor sptVisitor){

@Override
public List<GraphPath> getPaths(RoutingRequest options) {

SPTService sptService = this.sptServiceFactory.instantiate();

ArrayList<GraphPath> paths = new ArrayList<GraphPath>();

Expand Down Expand Up @@ -114,12 +116,12 @@ public void setGraphService(GraphService graphService) {
this.graphService = graphService;
}

public SPTService getSptService() {
return sptService;
public SPTServiceFactory getSptServiceFactory() {
return sptServiceFactory;
}

public void setSptService(SPTService sptService) {
this.sptService = sptService;
public void setSptServiceFactory(SPTServiceFactory sptServiceFactory) {
this.sptServiceFactory = sptServiceFactory;
}

}
Expand Up @@ -46,13 +46,15 @@ public class RetryingPathServiceImpl implements PathService {
private static final double MAX_WALK_MULTIPLE = 16;

private GraphService graphService;

private SPTServiceFactory sptServiceFactory;

public RetryingPathServiceImpl(GraphService graphService, SPTService sptService) {
public RetryingPathServiceImpl(GraphService graphService, SPTServiceFactory sptServiceFactory) {
this.graphService = graphService;
this.sptService = sptService;
this.sptServiceFactory = sptServiceFactory;
}

private SPTService sptService;


private double firstPathTimeout = 0; // seconds
private double multiPathTimeout = 0; // seconds
Expand Down Expand Up @@ -108,6 +110,9 @@ public List<GraphPath> getPaths(RoutingRequest options) {
double initialMaxWalk = maxWalk;
long maxTime = options.arriveBy ? 0 : Long.MAX_VALUE;
RoutingRequest currOptions;

SPTService sptService = this.sptServiceFactory.instantiate();

while (paths.size() < options.numItineraries) {
currOptions = optionQueue.poll();
if (currOptions == null) {
Expand Down Expand Up @@ -214,12 +219,12 @@ public void setGraphService(GraphService graphService) {
this.graphService = graphService;
}

public SPTService getSptService() {
return sptService;
public SPTServiceFactory getSptServiceFactory() {
return sptServiceFactory;
}

public void setSptService(SPTService sptService) {
this.sptService = sptService;
public void setSptServiceFactory(SPTServiceFactory sptServiceFactory) {
this.sptServiceFactory = sptServiceFactory;
}

@Override
Expand Down
@@ -0,0 +1,9 @@
package org.opentripplanner.routing.impl;

import org.opentripplanner.routing.services.SPTService;

public interface SPTServiceFactory {

SPTService instantiate();

This comment has been minimized.

Copy link
@abyrd

abyrd Nov 17, 2014

Member

I'm wary of one-liner source files, and of adding more factory interfaces. It seems much simpler and more readable if the pathservice just looks at the server config or request and instantiates the proper type of SPT or search algorithm as needed, rather than having to specify different components in the proper configuration at startup time. I doubt we're ever going to have more SPT types or search algorithms that can fit in a short conditional.

I think we should just get rid of most of these pluggable service interfaces in OTP, as they are essentially holdovers from Spring. But that's another ticket. This solves the problem for now.


}
Expand Up @@ -28,16 +28,16 @@ the License, or (at your option) any later version.
public class TrivialPathServiceImpl implements PathService {

GraphService graphService;
SPTService sptService;
SPTServiceFactory sptServiceFactory;

public TrivialPathServiceImpl(GraphService graphService, SPTService sptService) {
public TrivialPathServiceImpl(GraphService graphService, SPTServiceFactory sptServiceFactory) {
this.graphService = graphService;
this.sptService = sptService;
this.sptServiceFactory = sptServiceFactory;
}

@Override
public List<GraphPath> getPaths(RoutingRequest options) {
ShortestPathTree spt = sptService.getShortestPathTree(options);
ShortestPathTree spt = sptServiceFactory.instantiate().getShortestPathTree(options);
if (spt == null) {
return Collections.emptyList();
}
Expand Down
16 changes: 9 additions & 7 deletions src/main/java/org/opentripplanner/standalone/OTPServer.java
Expand Up @@ -19,8 +19,10 @@
import org.opentripplanner.inspector.TileRendererManager;
import org.opentripplanner.routing.algorithm.GenericAStar;
import org.opentripplanner.routing.core.RoutingRequest;
import org.opentripplanner.routing.impl.GenericAStarFactory;
import org.opentripplanner.routing.impl.LongDistancePathService;
import org.opentripplanner.routing.impl.RetryingPathServiceImpl;
import org.opentripplanner.routing.impl.SPTServiceFactory;
import org.opentripplanner.routing.services.GraphService;
import org.opentripplanner.routing.services.PathService;
import org.opentripplanner.routing.services.SPTService;
Expand All @@ -47,7 +49,7 @@ public class OTPServer {
public PathService pathService;
public RoutingRequest routingRequest; // the prototype routing request which establishes default parameter values
public PlanGenerator planGenerator;
public SPTService sptService;
public SPTServiceFactory sptServiceFactory;

// Optional Analyst Modules
public Renderer renderer;
Expand All @@ -74,15 +76,15 @@ public OTPServer (CommandLineParameters params, GraphService gs) {
// Core OTP modules
graphService = gs;
routingRequest = new RoutingRequest();
sptService = new GenericAStar();
sptServiceFactory = new GenericAStarFactory();

This comment has been minimized.

Copy link
@abyrd

abyrd Nov 17, 2014

Member

Ah, the sptServiceFactory is hard-wired to produce GenericAStar instances. Couldn't we just instantiate GenericAStar instances explicitly everywhere instantiate() is called? It seems like many of those call sites were written with the expectation that the sptService would always be a GenericAStar.


// Choose a PathService to wrap the SPTService, depending on expected maximum path lengths
if (params.longDistance) {
LongDistancePathService pathService = new LongDistancePathService(graphService, sptService);
LongDistancePathService pathService = new LongDistancePathService(graphService, sptServiceFactory);
pathService.timeout = 10;
this.pathService = pathService;
} else {
RetryingPathServiceImpl pathService = new RetryingPathServiceImpl(graphService, sptService);
RetryingPathServiceImpl pathService = new RetryingPathServiceImpl(graphService, sptServiceFactory);
pathService.setFirstPathTimeout(10.0);
pathService.setMultiPathTimeout(1.0);
this.pathService = pathService;
Expand All @@ -96,10 +98,10 @@ public OTPServer (CommandLineParameters params, GraphService gs) {
// Optional Analyst Modules.
if (params.analyst) {
tileCache = new TileCache(graphService);
sptCache = new SPTCache(sptService, graphService);
sptCache = new SPTCache(sptServiceFactory, graphService);
renderer = new Renderer(tileCache, sptCache);
sampleGridRenderer = new SampleGridRenderer(graphService, sptService);
isoChroneSPTRenderer = new IsoChroneSPTRendererAccSampling(graphService, sptService, sampleGridRenderer);
sampleGridRenderer = new SampleGridRenderer(graphService, sptServiceFactory);
isoChroneSPTRenderer = new IsoChroneSPTRendererAccSampling(graphService, sptServiceFactory, sampleGridRenderer);
surfaceCache = new SurfaceCache(30);
pointSetCache = new DiskBackedPointSetCache(100, new File(params.pointSetDirectory));
}
Expand Down
Expand Up @@ -84,6 +84,7 @@ the License, or (at your option) any later version.
import org.opentripplanner.routing.graph.Edge;
import org.opentripplanner.routing.graph.Graph;
import org.opentripplanner.routing.graph.Vertex;
import org.opentripplanner.routing.impl.GenericAStarFactory;
import org.opentripplanner.routing.impl.LongDistancePathService;
import org.opentripplanner.routing.impl.ParetoPathService;
import org.opentripplanner.routing.impl.SPTVisitor;
Expand Down Expand Up @@ -374,7 +375,7 @@ public String toString(){

private PathService pathservice;

private GenericAStar sptService = new GenericAStar();
private GenericAStarFactory sptServiceFactory = new GenericAStarFactory();

private DefaultListModel<String> metadataModel;

Expand Down Expand Up @@ -466,7 +467,7 @@ public GraphVisualizer(GraphService graphService) {

this.graphService = graphService;
this.graph = graphService.getGraph();
this.pathservice = new ParetoPathService(graphService, sptService);
this.pathservice = new ParetoPathService(graphService, sptServiceFactory);
setTitle("GraphVisualizer");

init();
Expand Down Expand Up @@ -591,7 +592,7 @@ private Container makeMainTab() {
// init center graphical panel
showGraph = new ShowGraph(this, getGraph());
pane.add(showGraph, BorderLayout.CENTER);
sptService.setTraverseVisitor(new VisualTraverseVisitor(showGraph));
sptServiceFactory.setTraverseVisitor(new VisualTraverseVisitor(showGraph));

// init left panel
leftPanel = new JPanel();
Expand Down Expand Up @@ -779,9 +780,9 @@ public void itemStateChanged(ItemEvent e) {

protected void setLongDistanceMode(boolean selected) {
if( selected ){
this.pathservice = new LongDistancePathService(graphService, sptService);
this.pathservice = new LongDistancePathService(graphService, sptServiceFactory);
} else {
this.pathservice = new ParetoPathService(graphService, sptService);
this.pathservice = new ParetoPathService(graphService, sptServiceFactory);
}

}
Expand Down Expand Up @@ -1478,9 +1479,9 @@ protected void route(String from, String to) {

// apply callback if the options call for it
if( dontUseGraphicalCallbackCheckBox.isSelected() ){
sptService.setTraverseVisitor(null);
sptServiceFactory.setTraverseVisitor(null);
} else {
sptService.setTraverseVisitor(new VisualTraverseVisitor(showGraph));
sptServiceFactory.setTraverseVisitor(new VisualTraverseVisitor(showGraph));

This comment has been minimized.

Copy link
@abyrd

abyrd Nov 17, 2014

Member

It seems odd to me that the factory has a traverse visitor rather than the SPT service itself. But maybe this is just the most expedient way to do it.

}

// set up a visitor to the path service so we can get the SPT as it's generated
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/org/opentripplanner/GtfsTest.java
Expand Up @@ -38,6 +38,7 @@ the License, or (at your option) any later version.
import org.opentripplanner.routing.graph.Graph;
import org.opentripplanner.routing.impl.AlertPatchServiceImpl;
import org.opentripplanner.routing.impl.DefaultStreetVertexIndexFactory;
import org.opentripplanner.routing.impl.GenericAStarFactory;
import org.opentripplanner.routing.impl.LongDistancePathService;
import org.opentripplanner.routing.impl.RetryingPathServiceImpl;
import org.opentripplanner.routing.services.PathService;
Expand All @@ -55,7 +56,7 @@ public abstract class GtfsTest extends TestCase {
AlertsUpdateHandler alertsUpdateHandler;
PlanGenerator planGenerator;
PathService pathService;
GenericAStar genericAStar;
GenericAStarFactory genericAStar;
TimetableSnapshotSource timetableSnapshotSource;
AlertPatchServiceImpl alertPatchServiceImpl;

Expand Down Expand Up @@ -101,7 +102,7 @@ protected void setUp() {
alertsUpdateHandler.update(feedMessage);
} catch (Exception exception) {}

genericAStar = new GenericAStar();
genericAStar = new GenericAStarFactory();
if (isLongDistance()) {
pathService = new LongDistancePathService(null, genericAStar);
} else {
Expand Down

0 comments on commit 84c9e0a

Please sign in to comment.