From fe68b9404a3a46d03412987b538d0a0e2f444850 Mon Sep 17 00:00:00 2001 From: ansons Date: Sat, 26 Oct 2024 00:44:06 -0400 Subject: [PATCH 1/5] Set Status.PRESENT after synchronous preload Fixes #949 --- .../com/conveyal/r5/analyst/NetworkPreloader.java | 7 +++++-- .../java/com/conveyal/r5/util/AsyncLoader.java | 15 ++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java index c3ee89aef..1d3f904b8 100644 --- a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java +++ b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java @@ -97,7 +97,10 @@ public LoaderState 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 @@ -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. + // Finished building all needed inputs for analysis, return the completed network return scenarioNetwork; } diff --git a/src/main/java/com/conveyal/r5/util/AsyncLoader.java b/src/main/java/com/conveyal/r5/util/AsyncLoader.java index 4606803bb..9a49d85ec 100644 --- a/src/main/java/com/conveyal/r5/util/AsyncLoader.java +++ b/src/main/java/com/conveyal/r5/util/AsyncLoader.java @@ -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 @@ -123,9 +123,7 @@ public LoaderState 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. @@ -139,7 +137,8 @@ public LoaderState 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. @@ -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. From ea16ea5ccbc3f1dd2f41f7291cab6b951418801b Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 31 Oct 2024 22:00:30 +0800 Subject: [PATCH 2/5] single underlying get method in AsyncLoader The nonblocking version of get just runs getBlocking in a thread. NetworkPreloader has corresponding preload and preloadBlocking methods. --- .../conveyal/r5/analyst/NetworkPreloader.java | 9 +++---- .../r5/analyst/cluster/AnalysisWorker.java | 4 +-- .../com/conveyal/r5/util/AsyncLoader.java | 26 ++++++++++--------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java index 1d3f904b8..10a59d4fb 100644 --- a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java +++ b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java @@ -79,7 +79,7 @@ public NetworkPreloader(TransportNetworkCache transportNetworkCache) { this.transportNetworkCache = transportNetworkCache; } - public LoaderState preloadData (AnalysisWorkerTask task) { + public LoaderState preload (AnalysisWorkerTask task) { if (task.scenario != null) { transportNetworkCache.rememberScenario(task.scenario); } @@ -96,11 +96,8 @@ public LoaderState preloadData (AnalysisWorkerTask task) { * This is provided specifically for regional tasks, to ensure that they remain in preloading mode while all this * data is prepared. */ - public TransportNetwork synchronousPreload (AnalysisWorkerTask task) { - Key key = Key.forTask(task); - TransportNetwork scenarioNetwork = buildValue(key); - setComplete(key, scenarioNetwork); - return scenarioNetwork; + public TransportNetwork preloadBlocking (AnalysisWorkerTask task) { + return getBlocking(Key.forTask(task)); } @Override diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java b/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java index 2add61702..1e8b7290f 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/AnalysisWorker.java @@ -314,7 +314,7 @@ public static void sleepSeconds (int seconds) { protected byte[] handleAndSerializeOneSinglePointTask (TravelTimeSurfaceTask task) throws IOException { LOG.debug("Handling single-point task {}", task.toString()); // Get all the data needed to run one analysis task, or at least begin preparing it. - final AsyncLoader.LoaderState networkLoaderState = networkPreloader.preloadData(task); + final AsyncLoader.LoaderState networkLoaderState = networkPreloader.preload(task); // If loading is not complete, bail out of this function. // Ideally we'd stall briefly using something like Future.get(timeout) in case loading finishes quickly. @@ -462,7 +462,7 @@ protected void handleOneRegionalTask (RegionalTask task) throws Throwable { // Note we're completely bypassing the async loader here and relying on the older nested LoadingCaches. // If those are ever removed, the async loader will need a synchronous mode with per-path blocking (kind of // reinventing the wheel of LoadingCache) or we'll need to make preparation for regional tasks async. - TransportNetwork transportNetwork = networkPreloader.synchronousPreload(task); + TransportNetwork transportNetwork = networkPreloader.preloadBlocking(task); // If we are generating a static site, there must be a single metadata file for an entire batch of results. // Arbitrarily we create this metadata as part of the first task in the job. diff --git a/src/main/java/com/conveyal/r5/util/AsyncLoader.java b/src/main/java/com/conveyal/r5/util/AsyncLoader.java index 9a49d85ec..29c74ee18 100644 --- a/src/main/java/com/conveyal/r5/util/AsyncLoader.java +++ b/src/main/java/com/conveyal/r5/util/AsyncLoader.java @@ -97,6 +97,16 @@ public String toString() { } } + /** This has been factored out of the executor runnables so subclasses can force a blocking (non-async) load. */ + protected V getBlocking (K key) { + setProgress(key, 0, "Starting..."); + V value = buildValue(key); + synchronized (map) { + map.put(key, new LoaderState(Status.PRESENT, "Loaded", 100, value)); + } + return value; + } + /** * Attempt to fetch the value for the supplied key. * If the value is not yet present, and not yet being computed / fetched, enqueue a task to do so. @@ -109,7 +119,7 @@ public LoaderState get (K key) { state = map.get(key); if (state == null) { // Only enqueue a task to load the value for this key if another call hasn't already done it. - state = new LoaderState(Status.WAITING, "Enqueued task...", 0, null); + state = new LoaderState<>(Status.WAITING, "Enqueued task...", 0, null); map.put(key, state); enqueueLoadTask = true; } @@ -120,10 +130,8 @@ public LoaderState get (K key) { // Enqueue task outside the above block (synchronizing the fewest lines possible). if (enqueueLoadTask) { executor.execute(() -> { - setProgress(key, 0, "Starting..."); try { - V value = buildValue(key); - setComplete(key, value); + getBlocking(key); } 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. @@ -143,7 +151,7 @@ public LoaderState get (K key) { * 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. */ - protected abstract V buildValue(K key) throws Exception; + protected abstract V buildValue(K key); /** * Call this method inside the buildValue method to indicate progress. @@ -154,15 +162,9 @@ 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. + * This will permanently associate an error with the key. No further attempt will ever be made to create the value. */ protected void setError (K key, Throwable throwable) { synchronized (map) { From f26f116cdd9afa65b6c1c31827d441dc93ffa574 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 31 Oct 2024 22:08:08 +0800 Subject: [PATCH 3/5] inline setError method Method comments said it was called in the buildValue implementation methods but it was actually only ever called in AsyncLoader error handling code. --- .../java/com/conveyal/r5/util/AsyncLoader.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/conveyal/r5/util/AsyncLoader.java b/src/main/java/com/conveyal/r5/util/AsyncLoader.java index 29c74ee18..5165385d7 100644 --- a/src/main/java/com/conveyal/r5/util/AsyncLoader.java +++ b/src/main/java/com/conveyal/r5/util/AsyncLoader.java @@ -134,8 +134,11 @@ public LoaderState get (K key) { getBlocking(key); } 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. - setError(key, t); + // threads can be killed by any Error that happens, stalling the executor. The below permanently + // associates an error with the key. No further attempt will ever be made to create the value. + synchronized (map) { + map.put(key, new LoaderState(t)); + } LOG.error("Async load failed: " + ExceptionUtils.stackTraceString(t)); } }); @@ -162,13 +165,4 @@ public void setProgress(K key, int percentComplete, String message) { } } - /** - * Call this method inside the buildValue method to indicate that an unrecoverable error has happened. - * This will permanently associate an error with the key. No further attempt will ever be made to create the value. - */ - protected void setError (K key, Throwable throwable) { - synchronized (map) { - map.put(key, new LoaderState(throwable)); - } - } } From 2c1bc95166ad9db13f330b23ca2070d97e551247 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 31 Oct 2024 22:24:56 +0800 Subject: [PATCH 4/5] add comments explaining current exception behavior --- src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java | 3 ++- src/main/java/com/conveyal/r5/util/AsyncLoader.java | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java index 10a59d4fb..083ad2aa9 100644 --- a/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java +++ b/src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java @@ -94,7 +94,8 @@ public LoaderState preload (AnalysisWorkerTask task) { * similar tasks will make interleaved calls to setProgress (with superficial map synchronization). Other than * causing a value to briefly revert from PRESENT to BUILDING this doesn't seem deeply problematic. * This is provided specifically for regional tasks, to ensure that they remain in preloading mode while all this - * data is prepared. + * data is prepared. + * Any exceptions that occur while building the network will escape this method, leaving the status as BUILDING. */ public TransportNetwork preloadBlocking (AnalysisWorkerTask task) { return getBlocking(Key.forTask(task)); diff --git a/src/main/java/com/conveyal/r5/util/AsyncLoader.java b/src/main/java/com/conveyal/r5/util/AsyncLoader.java index 5165385d7..62fe4034b 100644 --- a/src/main/java/com/conveyal/r5/util/AsyncLoader.java +++ b/src/main/java/com/conveyal/r5/util/AsyncLoader.java @@ -97,7 +97,10 @@ public String toString() { } } - /** This has been factored out of the executor runnables so subclasses can force a blocking (non-async) load. */ + /** + * This has been factored out of the executor runnables so subclasses can force a blocking (non-async) load. + * Any exceptions that occur while building the value will escape this method, leaving the status as BUILDING. + */ protected V getBlocking (K key) { setProgress(key, 0, "Starting..."); V value = buildValue(key); @@ -111,6 +114,7 @@ protected V getBlocking (K key) { * Attempt to fetch the value for the supplied key. * If the value is not yet present, and not yet being computed / fetched, enqueue a task to do so. * Return a response that reports status, and may or may not contain the value. + * Any exception that occurs while building the value is caught and associated with the key with a status of ERROR. */ public LoaderState get (K key) { LoaderState state = null; From 050bfce75b43e722bf18fccdece0275ff299da91 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Thu, 31 Oct 2024 22:36:13 +0800 Subject: [PATCH 5/5] move initial message back to async runnable Just to keep behavior more similar to previous versions. This could be changed in the future if we do a more significant refactor of how exceptions are handled and passed up to the backend. --- src/main/java/com/conveyal/r5/util/AsyncLoader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/r5/util/AsyncLoader.java b/src/main/java/com/conveyal/r5/util/AsyncLoader.java index 62fe4034b..256ee5c2b 100644 --- a/src/main/java/com/conveyal/r5/util/AsyncLoader.java +++ b/src/main/java/com/conveyal/r5/util/AsyncLoader.java @@ -102,7 +102,6 @@ public String toString() { * Any exceptions that occur while building the value will escape this method, leaving the status as BUILDING. */ protected V getBlocking (K key) { - setProgress(key, 0, "Starting..."); V value = buildValue(key); synchronized (map) { map.put(key, new LoaderState(Status.PRESENT, "Loaded", 100, value)); @@ -135,6 +134,7 @@ public LoaderState get (K key) { if (enqueueLoadTask) { executor.execute(() -> { try { + setProgress(key, 0, "Starting..."); getBlocking(key); } catch (Throwable t) { // It's essential to trap Throwable rather than just Exception. Otherwise the executor