abort pending requests in Backbone.sync

[ryanflorence]

Sending a pull request to start a conversation, happy to make adjustments to my branch if the team feels like this is a valid patch. @amccloud and I collaborated a bit on this. He's got a plugin for the same purpose here.

Purpose

It's not uncommon to have user interaction trigger a collection.fetch or model.fetch. Imagine a user clicking several filters for a list of items, or a search field fetching results as the user types. Usually the requests sent all come back in the right order--but not always.

Consider this:

1. User clicks "Filter A" for an activity stream.
2. A collection's url changes to represent the filter
3. collection.fetch is called
4. User quickly clicks "Filter B" before the xhr for "Filter A" returns to reset the collection.
5. "Filter B" returns from the server, collection.reset is called automatically for "Filter B"
6. "Filter A" finally returns (after "Filter B") and resets the collection to "Filter A".
7. The app is now in the wrong state.

The solution is to cancel the pending request.

Commit Message

when collection.fetch is called multiple times
(perhaps due to user-interaction with the page)
an earlier request could land after a later
request, causing unexpected state in the app

this commit tracks the last requests across all
collections and models and their sync methods,
checks if its pending, and aborts it.

can be overridden with {safe: false} in the sync
options object

[ryanflorence]

One concern is people who have overridden Backbone.ajax need to return something that behaves like a deferred object, but usually people just define a new Backbone.sync function, so I think impact will be minimal, but I could be wrong.

If they return nothing, the code will not break. If they return something, it needs to act like a deferred object.

[braddunbar]

From the docs:

Backbone's only hard dependency is Underscore.js ( > 1.3.1). For RESTful persistence, history support via Backbone.Router and DOM manipulation with Backbone.View, include json2.js, and either jQuery ( > 1.4.2) or Zepto.

Since jqXHR did not implement the promise api until jQuery 1.5, the minimum compatibility requirements would need to be updated to reflect it's use.

While I understand the usefulness of this approach (I've implemented something similar several times), I think Backbone.sync is best kept as simple as possible and this type of interaction should be implemented in user/plugin code.

[amccloud]

Plugin of this https://github.com/amccloud/backbone-safesync

[ryanflorence]

@braddunbar @collection.fetch() is more than a request, it's telling your app to get the current state of the collection. With improper handling of the asynchronous nature, the method doesn't "return" what it is supposed to. From the docs:

When the model data returns from the server, the collection will reset.

When would you ever want the collection to reset to anything other than the last call to fetch?

More importantly, when would you ever want model.save(attrs) to ever save anything other than the last call to save?

[simenbrekken]

I just ran into this problem while implementing live search, where some partial word queries that weren't cached took longer than complete words ("snowboa" vs "snowboard").

It would be great if this was the the default behavior with an option such as concurrent set to true to disable it.

[braddunbar]

This is definitely a useful behavior, but I think it belongs in a plugin that wraps sync rather than in Backbone itself. Thanks for your request @rpflorence!

[ryanflorence]

So far nobody has said why it belongs in a plugin.

Again I ask:

When would you ever want the collection to reset to anything other than the last call to fetch?

More importantly, when would you ever want model.save(attrs) to ever save anything other than the last call to save?

Apps with the current Backbone.sync are naive w/o this behavior.

[braddunbar]

You're right, I didn't explain myself well. Sorry about that. :)

When would you ever want the collection to reset to anything other than the last call to fetch?

More importantly, when would you ever want model.save(attrs) to ever save anything other than the last call to save?

There are two reasons I think.

1. The last request sent does not necessarily have the most up to date data. For checking this, a custom method of discerning the freshness of the data would likely need to be implemented (e.g. a timestamp in the response that can be checked against a fetched_at property on the model/collection). This method is likely custom to the domain of your application and should not be dictated by Backbone.
2. Just because you don't want the data returned from a request doesn't mean you want to indicate that the request has failed/aborted. For instance, sync could be used for persisting tracking data rather than the current state of a model. In this case, you want to know the success of each request, regardless of when it was sent.

[pwightman]

@braddunbar Speaking to your second point, and perhaps both points, that's why a safe: false option could be passed to calls in those situations, no? I think @rpflorence is catering toward the rule, and the cases you pointed out are the exception. At least that's been my experience. I kind of view this to be in the same boat as asynchronous model updates, which was (wisely) switched to be the default a while back, with the wait option provided for exceptions.

[braddunbar]

@pwightman As for point one, I think requests come back in reverse order more than you might think. With multiple backend-endpoints for a system you can never be sure how long a request will take.

That said, point number two is what really worries me here. It makes otherwise successful requests appear to have failed and renders the result of the returned promise useless since I can't tell if it failed due to a subsequent request or an actual failure. For instance, the following code snippet will display an error message for a request that was successful, but resent before completion. I don't think this is the behavior we're looking for.

model.save(...)
  .done(function(){
    // handle response
  })
  .fail(function() {
    // display error message
  });

[ryanflorence]

model.save(...)
  .done(function(){
    // handle response
  })
  .fail(function(xhr, status) {
    if (status === 'abort') return;
    // display error message
  });

Or do the abort check elsewhere before it gets here (in jQuery).

[braddunbar]

Or do the abort check elsewhere before it gets here (in jQuery).

I'm not sure what you mean here. Are you saying I should patch jQuery?

[amccloud]

@braddunbar You could surely check the textStatus and errorThrown passed to fail and complete respectively.

Edit: @rpflorence beat me to it.

[braddunbar]

Also, there is no way to tell why the request was aborted. Was it aborted due to a resent request or some other legitimate reason for which an error should be displayed?

[ryanflorence]

http://api.jquery.com/ajaxError/ is an option (not requiring patching), but I personally just do the check in the fail handler.

[ryanflorence]

Also, there is no way to tell why the request was aborted. Was it aborted due to a resent request or some other legitimate reason for which an error should be displayed?

The only way its aborted is if this version of Backbone.sync aborts it or you do it yourself somewhere, in which case you display the error message then.

There might be an argument here to do something to say it was aborted by backbone.sync, maybe trigger an event on the collection / model.

[ryanflorence]

Summarizing a conversation in IRC, we can abort the request and send a textStatus to the jqXHR deferred of 'stale', so you can do different error handling. These changes are incorporated in the pull request now.

collection.fetch().fail(function(jqXHR, status) {
  if (status === 'stale') {
    // do something
  }
});

And renamed the option to abortPending, so you can tell it not to abort pending requests if you don't want it to.

collection.fetch({abortPending: false});

[jashkenas]

There are actually three different cases that I'm aware of -- and your app can have mixes of all three, even within a single "interaction":

• Multiple parallel requests are fine, and idempotent. The order in which they return doesn't matter.
• If another request is made, the previous one should be canceled -- DocumentCloud search works like this.
• If another request is made, it should wait for the previous one to finish before it begins. Spine works like this by default.

Part of the problem is that this logic can span different models and different collections. If I delete a collection, and then modify a model that was in it, that's inherently a broken action. If I wait for the collection delete to succeed, then my queued request is broken.

I don't think that this is something that's wise for Backbone to try to do for you by default -- especially when many of the interesting interactions in this space are across different types of models that the system doesn't necessarily know are logically related. We want to give you the primitives to sequence these things yourself.

[pituki]

I was having similar issue, where I had requests that returned at different times, and sometimes the success would render in the wrong order. I created what I call LastCall, it is a function factory/decorator. https://github.com/pituki/LastCall
see sample in readme.