Race condition/out of order results possible when data.src is an async function

[ethanhjennings]

• [x] System Information

  • [ ] Browser type and version Any (I'm on Chrome 94.0.4606.114)
  • [ ] OS type and version Any (I'm on Chrome OS)
  • [ ] WINDOWS: be sure to indicate which terminal you're using -- (i.e., cmd.exe, powershell, git- bash, cygwin, Ubuntu via windows subsystem for linux, etc...)
  • [ ] Node version (IF applicable)
    • [ ] Any error messages that may be in the console where you ran npm start
  • [ ] Any error messages in the JS console

• [x] Describe the bug

I have a simple async data.src function like this in the config that gets autocomplete results from an endpoint. I'm also using a no-op search engine so I just rely on results from the endpoint.

...
src: async (query) => {
        const response = await fetch(url);
        let json = await response.json();
        
        ... // do processing on json
        return json;
}
...
searchEngine: (q, r) => r,
...

The problem is that if the endpoint returns things out of order the final displayed results could be old and not make sense with the query. In practice this happens a lot because the geolocation API I'm using is rather unreliable (in terms of speed).

• [X] To Reproduce

This is difficult to reproduce because it requires an autocomplete endpoint/API to return results out of order. I'm using geoapify.

• [x] Expected behavior

Results from the src function be rejected if newer results have already been returned, or else some workaround allowing old results to be rejected.

I have tried this workaround which does work, but leaves uncaught errors:

...
src: async (query) => {
        const sent_timestamp = (new Date()).getTime();

        const response = await fetch(url);
        let json = await response.json();
        
        // Reject out of order results
        if (sent_timestamp < newest_timestamp) {
            throw Error("Old results");
        }
        newest_timestamp = sent_timestamp;
        
        ... // do processing on json
        return json;
}
...

I'm new to promises/async so might be missing some other way to reject old results cleanly. But regardless I believe this should be handled automatically.

[folknor]

You're correct that there's no built-in way to do this.

You could also use triggerCondition to check the timestamp. It is checked just before running the data source function. The benefit of using triggerCondition is that the library wont re-render the list. The downside is that it will close the list if the trigger condition fails. I mention this because I believe Tarek might want to think a bit about this.

What I'd do is exactly what you're doing - return an error. That way you hit if (ctx.feedback instanceof Error) return; in start.js and everything is peachy.

For Tareks future reference, it might be helpful to read this (unless he's a wizard and knows how already :-D): https://lsm.ai/posts/7-ways-to-detect-javascript-async-function/ when implementing/fixing this issue, if he does.

[folknor]

Although medium is the devil, you might want to read https://medium.com/@karenmarkosyan/how-to-manage-promises-into-dynamic-queue-with-vanilla-javascript-9d0d1f8d4df5 - but I'm sure there's way better articles about this topic out there.

[ethanhjennings]

Returning an Error instead of throwing it works just fine for me! Thanks! Also, FYI, I found jQuery autocomplete had this same issue and a fix was implemented: https://bugs.jqueryui.com/ticket/5982

[folknor]

Returning an Error instead of throwing

Haha! Can't believe I misread that in your code, silly me :-P I didn't even read the keyword throw, I just read the Error part and wrote my comment. Good thing accidentally helped you anyway!

[tomdav999]

This was driving me nuts, thanks for the insights on how to patch. Since I'm using standard promises, my solution was simple - never resolve promises for stale queries so they go to the garbage collector in the sky rather than littering the console with uncaught errors. I also threw in my own caching since the cache option must be disabled for query request (makes no sense, seems it should be the other way around)!

    src: function(query) { return new Promise(function(resolve) {
        var cache = autoCompleteJS.cache; if (!cache) cache = autoCompleteJS.cache = [];
        var tbd = { resolve: resolve, timestamp: autoCompleteJS.timestamp = (new Date()).getTime() };
        for (var i = 0; i < cache.length; i++) if (cache[i].query == query) {
            if (cache[i].src) resolve(cache[i].src); else cache[i].tbd = tbd; // leave prior tbd.resolve unresolved
            return;
        }
        cache.push({ query: query, tbd: tbd });
        var xhr = new XMLHttpRequest();
        xhr.open('GET', 'search.php?usernames=' + query, true);
        xhr.send(null);
        xhr.onreadystatechange = function() {
            if (xhr.readyState == 4) {
                cache[i].src = JSON.parse(xhr.responseText);
                if (cache[i].tbd.timestamp == autoCompleteJS.timestamp) {
                    cache[i].tbd.resolve(cache[i].src); // otherwise leave unresolved to avoid race condition
                }
                xhr = cache[i].tbd = null;
            }
        }
    })},