Skip to content

Commit

Permalink
Autocomplete: Don't handle remote data if it's not the most recent re…
Browse files Browse the repository at this point in the history 
…quest. Fixes #5982 - Autocomplete: Race condition causes incorrect suggestions.
  • Loading branch information
@scottgonzalez
scottgonzalez committed Aug 24, 2010
1 parent 1cca969 commit f115b48
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions ui/jquery.ui.autocomplete.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ $.widget( "ui.autocomplete", {
},

_initSource: function() {
var array,
var self = this,
array,
url;
if ( $.isArray(this.options.source) ) {
array = this.options.source;
Expand All @@ -220,7 +221,11 @@ $.widget( "ui.autocomplete", {
} else if ( typeof this.options.source === "string" ) {
url = this.options.source;
this.source = function( request, response ) {
$.getJSON( url, request, response );
self.xhr = $.getJSON( url, request, function( data, status, xhr ) {
if ( xhr === self.xhr ) {
response.apply( this, arguments );
}
});
};
} else {
this.source = this.options.source;
Expand Down

5 comments on commit f115b48

@jzaefferer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An addition or instead of the check within the success-callback, we could abort any previous still-running requests (self.xhr.abort()). That prevents the callback from getting called, and could have positive effects on performance.
@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug where calling xhr.abort() triggers the success callback, so we should probably add the xhr.abort() call but leave the check.
@badsyntax
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm speaking out of context here, if so please ignore my comment, but I don't believe xhr.abort() executing the success function is a bug, I believe that is the default behavior. I /think/ this change was added in 1.4.2. A simple test: http://badsyntax.co.uk/tests/jquery-xhr-abort.html and perhaps a bit of insight on why this is the default behavior. http://groups.google.com/group/jquery-dev/browse_thread/thread/3d8f7ac78c9b0117?pli=1
@scottgonzalez
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the background, but it seems like there's still some debate about what the expected behavior should be. There's still an open ticket for this: http://dev.jquery.com/ticket/6060 (and there have been more that were closed duplicates). In any case, I've added the abort call and kept this check in da2be6a.
@badsyntax
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the impression this behavior wasn't up for debate, certainly I'd prefer it if success wasn't executed on abort. Thanks for the link, keeping an eye on this.
Please sign in to comment.