Cancel outstanding trip plan request #1000

flibbertigibbet · 2018-02-22T17:48:38Z

Overview

When finding directions in response to directions form input, cancel previous routing request
before issuing a new one.

Fixes outdated results being displayed when the routing requests resolve out of order.

Notes

Changed throttle to debounce in case this lodash issue still exists (issue was closed in favor of a related PR, but the PR was never merged).

Changed trip plan routing function to optionally return Deferred instead of an immutable Promise so it can be cancelled in the requesting control. Did that instead of modifying the trip plan routing function to cancel requests itself, as in other cases we do want to allow multiple routing requests to fire simultaneously and return as soon as they resolve, particularly when finding travel times from the origin to all destinations.

Testing Instructions

(To reproduce the issue, try the steps below on the production site.)

Plan a bicycle trip; the longer the better, to increase query time
After loading animation displays, toggle the transit button repeatedly while animation is still showing
Loading animation should continue to show until results display
Itineraries displayed should match the last directions form input options set (with transit, or not)
Finding travel times to full destinations list should continue to work as expected

Fixes #998.

When finding directions in response to directions form input, cancel previous routing request before issuing a new one. Fixes outdated results being displayed when the routing requests resolve out of order. Fixes azavea#998.

tnation14 · 2018-02-22T18:49:23Z

build again

KlaasH

Looks good. A few comments on some of the nuances below.

KlaasH · 2018-02-27T17:31:56Z

src/app/scripts/cac/control/cac-control-directions.js

@@ -98,7 +102,7 @@ CAC.Control.Directions = (function (_, $, moment, Control, Routing, UserPreferen
     * Set user preferences before planning trip.
     * Throttled to cut down on requests.
     */
-    var planTrip = _.throttle(function() {  // jshint ignore:line
+    var planTrip = _.debounce(function() {  // jshint ignore:line


What are the pros and cons of this? On the pro side, it will drop more useless requests in the case of fast switching back and forth (though that doesn't seem like a huge real-world case) or someone clicking into directions then immediately changing mode options. But the downside is that for simple interactions where the user clicks into directions with the desired mode options already selected--i.e. there's only one request total--this will make it .75 seconds slower.

We could set the option leading=true to avoid that, though that would mean the "get directions, switch mode" cases would always produce 2 requests rather than sometimes dropping the first. It would still save requests vs. the current throttle in a rapid-toggling situation.

See notes section above: there is an issue with the lodash throttle implementation.

In testing, I haven't found any noticeable difference in responsiveness; the actual query time is much longer than the debounce window.

I had to go back and read the debounce vs. throttle explainer again and by the time I had finished I had somehow dropped the bit about the throttle bug from my brain. And yeah, I think debounce makes more sense here, since even without the bug in throttle, sending requests in the middle of a stream of rapid invocations wouldn't be useful.

But I still think debounce-with-leading-edge seems preferable. It still wouldn't be the same as throttle (which is, for Lodash, debounce-with-leading-edge-and-max-wait), though it would be susceptible to a similar effect, where if the user waits 751ms between requests the debounced grouping would close and send the trailing request and 1ms later the leading request of the next batch would go.

Running a few simple directions lookups on the live site, I'm getting responses in the 400-500ms range. For longer trips, I'm seeing 1-1.25 seconds. Adding 750ms to all of those seems to me like a potentially significant/noticeable difference, and I don't think it's worth it to add that lag to what seems to me like a core/common interaction to save load in what seem like less common scenarios.

On adding leading edge to debounce, the issue is reintroduced. Possibly related in implementation to the throttle issue.

Since there's logic now to throw away outdated, pending requests, maybe simply reducing the debounce timeout would be fine.

Even with calls made in quick succession, as can happen with leading turned on, it shouldn't be possible for stale results to get displayed because the calls will always be in order and superseded ones should be getting canceled. I think I see how it's happening. I set leading to true and added some logging, then tested with this trip, which is significantly faster without transit than with (maybe that's true in most cases, but just to have a working example written down...). Start from no-transit, I click three times quickly (on-off-on), wait the moment it takes for the debounce timeout to pass and the spinner to appear, then click once more to turn transit back off. So it sends the first click (with transit) as the first leading request, ignores the second click, sends the third (no transit) as the trailing request, then sends the fourth click (with transit) as the leading request of the next debounce grouping. That leaves me in a state where I should be getting walking directions but the transit directions are reliably coming back last and showing up.

The console looks like this:

I've annotated it with my interpretation of which requests the lines are talking about. The interesting part is that when the trailing request comes back, it should have been canceled by the 2nd leading request, and in fact planTripRequest is null, but the .then callback is still firing. My interpretation is that when planTripRequest gets set to null, that doesn't destroy the object, and the callback is still attached to its success handler. On this theory, the fact that the right itinerary is always winning currently has mainly to do with the 750ms minimum gap between requests being enough to keep things in order.

Adding a check and bailing out if planTripRequest is falsy at the top of the then function appears to cause the final selection to always end up being the one displayed.

Modified to never nullify the trip request, since rejecting an already completed request does nothing and does not error, and it seems to resolve the leading edge issue.

Yay! Repeating the above with the new code, I now see two rejections--i.e. Leading 1 gets rejected by Trailing, which in turn gets rejected by Leading 2--which is as it should be and results in it showing the right thing.

KlaasH · 2018-02-27T17:36:32Z

src/app/scripts/cac/places/cac-places.js

@@ -127,7 +127,7 @@ CAC.Places.Places = (function(_, $, moment, Routing, UserPreferences) {
            if (xCoord && yCoord) {
                var placeCoords = [yCoord, xCoord];
                // get travel time to destination and update place card
-                Routing.planTrip(exploreLatLng, placeCoords, date, otpOptions)
+                Routing.planTrip(exploreLatLng, placeCoords, date, otpOptions, false)


Having Routing.planTrip switch its return type based on an argument seems not ideal to me. This is only invoked here and in cac-control-directions.js, right? Couldn't we just tack .promise() onto the end of this line and have the function always return a Deferred?

Given that it's convention everywhere else in this app and recommended by jQuery to always pass around non-mutable promises, I thought keeping that as the default behavior would be the least confusing thing to do. Could change that if you feel strongly otherwise.

I see what you mean. Though this still looks weird to me, at least it's explicit. I don't feel strongly either way.

KlaasH · 2018-02-27T17:43:00Z

src/app/scripts/cac/control/cac-control-directions.js

@@ -164,7 +180,15 @@ CAC.Control.Directions = (function (_, $, moment, Control, Routing, UserPreferen
            itineraryListControl.show();
            // highlight first itinerary in sidebar as well as on map
            findItineraryBlock(currentItinerary.id).addClass(options.selectors.selectedItineraryClass);
+            planTripRequest = null;


If planTrip got invoked again while the previous one was in this block, it would call planTripRequest.reject() after it had already resolved. Maybe that's not possible in practice, and maybe it wouldn't be a big deal if it happened, but I'd be inclined to put this at the top of the callback just in case, since I don't see any disadvantage to doing so.

Reasoning in nullifying the request as late as possible is so that display of known outdated results can be aborted, rather than doing unnecessary work and potentially delaying display of the newer response. According to this docs example, though, it looks like it would have no effect on a completed promise.

Ok, yeah, no difference then, and calling reject after it's resolved is ignored (I was thinking it might throw an error). So this is fine as-is.

Because it may have already been set to a new request.

KlaasH

Looks good! 👍

KlaasH · 2018-03-01T19:26:51Z

src/app/scripts/cac/control/cac-control-directions.js

@@ -98,7 +102,7 @@ CAC.Control.Directions = (function (_, $, moment, Control, Routing, UserPreferen
     * Set user preferences before planning trip.
     * Throttled to cut down on requests.
     */
-    var planTrip = _.throttle(function() {  // jshint ignore:line
+    var planTrip = _.debounce(function() {  // jshint ignore:line


Yay! Repeating the above with the new code, I now see two rejections--i.e. Leading 1 gets rejected by Trailing, which in turn gets rejected by Leading 2--which is as it should be and results in it showing the right thing.

Cancel outstanding trip plan request

3fc4eda

When finding directions in response to directions form input, cancel previous routing request before issuing a new one. Fixes outdated results being displayed when the routing requests resolve out of order. Fixes azavea#998.

flibbertigibbet assigned KlaasH Feb 22, 2018

flibbertigibbet requested a review from KlaasH February 22, 2018 17:48

KlaasH requested changes Feb 27, 2018

View reviewed changes

flibbertigibbet force-pushed the feature/kak/directions-request-cancellation#998 branch from a862574 to 3fc4eda Compare February 28, 2018 16:24

Never nullify trip request

956486d

Because it may have already been set to a new request.

KlaasH approved these changes Mar 1, 2018

View reviewed changes

flibbertigibbet merged commit 5530728 into azavea:develop Mar 1, 2018

flibbertigibbet deleted the feature/kak/directions-request-cancellation#998 branch March 1, 2018 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel outstanding trip plan request #1000

Cancel outstanding trip plan request #1000

flibbertigibbet commented Feb 22, 2018

tnation14 commented Feb 22, 2018

KlaasH left a comment

KlaasH Feb 27, 2018

flibbertigibbet Feb 27, 2018

KlaasH Feb 27, 2018

flibbertigibbet Feb 28, 2018 •

edited

flibbertigibbet Mar 1, 2018

KlaasH Mar 1, 2018

flibbertigibbet Mar 1, 2018

KlaasH Mar 1, 2018

KlaasH Feb 27, 2018

flibbertigibbet Feb 27, 2018

KlaasH Feb 27, 2018

KlaasH Feb 27, 2018

flibbertigibbet Feb 27, 2018

KlaasH Feb 27, 2018

KlaasH left a comment

KlaasH Mar 1, 2018

Cancel outstanding trip plan request #1000

Cancel outstanding trip plan request #1000

Conversation

flibbertigibbet commented Feb 22, 2018

Overview

Notes

Testing Instructions

tnation14 commented Feb 22, 2018

KlaasH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flibbertigibbet Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlaasH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flibbertigibbet Feb 28, 2018 •

edited