Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Status.PRESENT after synchronous preload #950

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ public LoaderState<TransportNetwork> preloadData (AnalysisWorkerTask task) {
* data is prepared.
*/
public TransportNetwork synchronousPreload (AnalysisWorkerTask task) {
return buildValue(Key.forTask(task));
Key key = Key.forTask(task);
TransportNetwork scenarioNetwork = buildValue(key);
setComplete(key, scenarioNetwork);
return scenarioNetwork;
}

@Override
Expand Down Expand Up @@ -140,7 +143,7 @@ protected TransportNetwork buildValue(Key key) {
linkedPointSet.getEgressCostTable(progressListener);
}
}
// Finished building all needed inputs for analysis, return the completed network to the AsyncLoader code.
abyrd marked this conversation as resolved.
Show resolved Hide resolved
// Finished building all needed inputs for analysis, return the completed network
return scenarioNetwork;
}

Expand Down
15 changes: 10 additions & 5 deletions src/main/java/com/conveyal/r5/util/AsyncLoader.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
* "value is present in map".
*
* Potential problem: if we try to return immediately saying whether the needed data are available,
* there are some cases where preparing the reqeusted object might take only a few hundred milliseconds or less.
* there are some cases where preparing the requested object might take only a few hundred milliseconds or less.
* In that case then we don't want the caller to have to re-poll. In this case a Future.get() with timeout is good.
*
* Created by abyrd on 2018-09-14
Expand Down Expand Up @@ -123,9 +123,7 @@ public LoaderState<V> get (K key) {
setProgress(key, 0, "Starting...");
try {
V value = buildValue(key);
synchronized (map) {
map.put(key, new LoaderState(Status.PRESENT, null, 100, value));
}
setComplete(key, value);
} catch (Throwable t) {
// It's essential to trap Throwable rather than just Exception. Otherwise the executor
// threads can be killed by any Error that happens, stalling the executor.
Expand All @@ -139,7 +137,8 @@ public LoaderState<V> get (K key) {

/**
* Override this method in concrete subclasses to specify the logic to build/calculate/fetch a value.
* Implementations may call setProgress to report progress on long operations.
* Implementations may call setProgress to report progress on long operations; if they do so, any callers of this
* method are responsible for also calling setComplete() to ensure loaded objects are marked as PRESENT.
* Throw an exception to indicate an error has occurred and the building process cannot complete.
* It's not entirely clear this should return a value - might be better to call setValue within the overridden
* method, just as we call setProgress or setError.
Expand All @@ -155,6 +154,12 @@ public void setProgress(K key, int percentComplete, String message) {
}
}

public void setComplete(K key, V value) {
synchronized (map) {
map.put(key, new LoaderState(Status.PRESENT, "Loaded", 100, value));
}
}

/**
* Call this method inside the buildValue method to indicate that an unrecoverable error has happened.
* FIXME this will permanently associate an error with the key. No further attempt will ever be made to create the value.
Expand Down
Loading