Set Status.PRESENT after synchronous preload #950
Conversation
|
Thanks @ansoncfit for identifying this problem and tracking down the cause! The proposed fix already looks good, but I have a sense we could reduce code duplication slightly, and reduce the risk of similar future problems by moving the synchronousPreload logic up into the AsyncLoader abstract class as a public getSynchronous() method, which is then also called by the runnable inside get(). This would have all code paths calling the exact same instance of the crucial buildValue / setComplete sequence, so it can't be done wrong in any future extensions of AsyncLoader. I'll give it some thought tomorrow and propose a patch. |
The nonblocking version of get just runs getBlocking in a thread. NetworkPreloader has corresponding preload and preloadBlocking methods.
Method comments said it was called in the buildValue implementation methods but it was actually only ever called in AsyncLoader error handling code.
|
What do you think of the approach shown in the three commits I just pushed to this branch? The NetworkPreloader has blocking and nonblocking preload methods, which call blocking and nonblocking methods on the base class. Then the nonblocking method on the base class just runs the blocking method in a background thread. So everything leads down to a single blocking get method that always includes the logic to mark the element present. This should provide identical behavior to your version before my changes, I just find the version where everything converges on a single method to be a little less susceptible to similar problems in the future. The question arises whether the try/catch error handling code should be inside getBlocking, which would make it apply to both preload and preloadSynchronous. Errors in either case would then set the status to ERROR. This would be more consistent but would interfere with the way we currently shuttle regional task exception messages back to the backend. So I just added some comments clarifying how it works currently. |
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.
|
Well, actually there was one small difference in behavior: the status was being set to "Starting..." just before a synchronous load begins. I moved it back just to keep behavior mostly identical 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. |
There was a problem hiding this comment.
Thanks for the revisions. I agree with having both paths converge on a single method.
I will make sure we test workers switching between scenarios and single-point/regional tasks on staging before our next release.
Let's go ahead and merge this PR (I would mark it as approved, but GH doesn't give me the option as the original author).
This PR fixes #949, a longstanding issue (possibly since #746).
We have two pathways for loading transport network data on a worker:
preloadData(asynchronously, for single-point tasks) andsynchronousPreload(for regional tasks). Once data are loaded in an AsyncLoader, they should be marked withStatus.PRESENT. The async pathway was doing this correctly. But once a worker switched to a regional task, it would switch the status toLOADINGand never switch it back toPRESENT.This PR factors out a
setCompletemethod and calls it in both pathways.