Make sure to handle the case when search results come back in a different order than retrieved

[stacey-gammon]

Adding debounce should have alleviated the problem but apparently didn't actually fix it. (see #14909)

I also extended the debounce time a bit since that really should have made this pretty rare.

I was able to reproduce the error by adding this code to visualize_listing.js:

 this.fetchItems = (filter) => {
    const isLabsEnabled = config.get('visualize:enableLabs');
    return visualizationService.find(filter, config.get('savedObjects:listingLimit'))
      .then(async result => {
        const randomSleepTime = Math.random() * 10000;
        console.log('going to sleep for ' + randomSleepTime);
        await new Promise(resolve => setTimeout(resolve, randomSleepTime));
        return result;
      })
      .then(result => {
        this.totalItems = result.total;
        this.showLimitError = result.total > config.get('savedObjects:listingLimit');

to verify that this does fix the problem.

[nreese]

lgtm

[LeeDr]

@nreese just curious if you tested this with IE 11 browser?

[nreese]

@LeeDr I am mac only. No windows for me

[stacey-gammon]

Me too, no IE 11 to test with. But it's not just IE11 we'd need to repro, I think you'd also need a setup that makes the fetches really slow, like how you had kibana running on a vm server separate from your client. That's why I tried to simulate the issue in code. Albiet not perfect. Any chance you can test this Lee with the same setup you used to repro originally?

[cjcenizal]

Nice solution! I haven't tested in browser but the code looks good. Had one small suggestion.

[cjcenizal]

This isn't necessary, is it? Can we remove it?

[LeeDr]

I probably won't get a chance to test this until after 6.1.0 ships (probably next week). But we shouldn't merge it into 6.1.0 now anyway. It could go into the 6.1 branch after 6.1.0 is released so it makes it into 6.1.1.

I'm concerned that we're tweaking a timing parameter to try to get this to work. Someone will have a heavy load or a bit more latency and they'll see the same problem (unless the order is really the issue). Its the same reason we try to avoid using sleeps in the UI tests.

If I understand the hypothesis of the problem, why would we send a second request for the first request has even returned? I guess it might appear more responsive, but if it's causing this ordering issue maybe it shouldn't make a request until the previous on has returned.

[stacey-gammon]

Failed on #13163, been meaning to pull together a PR to skip that test...

jenkins, test this

[stacey-gammon]

@LeeDr

I'm concerned that we're tweaking a timing parameter to try to get this to work. Someone will have a heavy load or a bit more latency and they'll see the same problem (unless the order is really the issue). Its the same reason we try to avoid using sleeps in the UI tests.

Increasing the debounce doesn't fix this, the other logic does, so don't worry - I'm not relying on that and it shouldn't matter how long of a latency some requests take, or how heavy of a load, this should still work. I increased the debounced value only because it seemed 200ms was too slow if it was trigging per character input on your searches. It wasn't necessary for the fix.

If I understand the hypothesis of the problem, why would we send a second request for the first request has even returned? I guess it might appear more responsive, but if it's causing this ordering issue maybe it shouldn't make a request until the previous on has returned.

I don't think this is the right solution because it could definitely slow responsiveness down a ton. The fix here should be sufficient. It will just ignore any earlier requests that come back, and only catch and save the most recent one that the user requested.