fix(concurrency): ensure responses resolve in order

[sarahdayan]

This fixes a concurrency issue with sources by tracking promises.

Problem

When users type a query, each keystroke triggers a call to retrieve their sources. As we retrieve the data, we update the Autocomplete state based on it (e.g., collections, whether the panel should be open or not, etc.)

When these sources are dynamic (e.g., a fetch call to a remote service, a call to Algolia using getAlgoliaResults, etc.), they can take some time, and not necessarily resolve in order. This means we can potentially update the Autocomplete state with stale data. For example, if a user types "a", "b" and "c", three requests will go out with queries "a", "ab", and "abc", but if the query with "ab" is the last one to resolve, Autocomplete will update its state based on this query and its results, making the state inconsistent with the actual query "abc".

This can also be observed with UI state, as Autocomplete derives its next state from results. Debouncing results makes the Autocomplete experience particularly vulnerable to this bug.

Solution

Instead of trying to artificially "cancel" state updates, the fix uses a mechanism introduced in #347 to track promises and make sure they resolve with the most fresh data, even if they resolve out of order. This allows us not to worry about promises resolving "late" and solve the issue by controlling what they return instead.

The fix wraps the entire promise chain into a tracked promise, as we only care about the final result before we make state mutations. We don't need to track calls to sources then calls to items independently, as long as we only mutate state outside of the tracked promise.

Enregistrement.de.l.ecran.2021-10-06.a.21.00.34.mov

This should also solve the concurrency issue observed on the Algolia documentation search and in DocSearch v3.

fixes #654

[sarahdayan]

@shortcuts Looping you in for DocSearch.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5c02873:

Sandbox	Source
@algolia/autocomplete-example-github-repositories-custom-plugin	Configuration
@algolia/autocomplete-example-instantsearch	Configuration
@algolia/autocomplete-example-playground	Configuration
@algolia/autocomplete-example-preview-panel-in-modal	Configuration
@algolia/autocomplete-example-react-renderer	Configuration
@algolia/autocomplete-example-starter-algolia	Configuration
@algolia/autocomplete-example-starter	Configuration
@algolia/autocomplete-example-reshape	Configuration
@algolia/autocomplete-example-vue	Configuration
@algolia/autocomplete-example-twitter-compose-with-typeahead	Configuration
amazing-tharp-6bgts	Issue #654

[francoischalifour]

Huge improvement! Thanks!

[francoischalifour]

Why do we push to the state history?

[sarahdayan]

We add the new states recorded in the onStateChange spy.

That's basically to avoid declaring a stateHistory2 variable. Is it confusing?

[francoischalifour]

Well I think we want to do stateHistory = onStateChange.mock.calls (pseudo-code), otherwise we have the first mock calls that are duplicated.

[sarahdayan]

Yes, it's fine because we don't use them though, but it's indeed clearer.

Addressed in 5c02873.

[francoischalifour]

I think the name collections is sufficient, wdyt?

[sarahdayan]

Addressed in 16a3b45.

[francoischalifour]

I'm not sure to understand this last part.

[sarahdayan]

This fix relies on wrapping the entire promise chain in a tracked promise.

To reuse your diagram, but with nested promises:

+--------------------------------------+
|        100ms                         |
| run(1) +-R1->+-R1A->                 |
|        300ms                         |
| run(2) +---R2--->+---R2A---> (SKIP)  |
|        200ms                         |
| run(3) +--R3-->+--R3A-->             |
+--------------------------------------+

Here, R1A, R2A and R3A are promises triggered in the then callback of respectively R1, R2 and R3. The run function wraps all that, not individual promises. It's fine because all we care about is that when the full chain resolves, we know the data is fresh, and we can now update the Autocomplete state based on it.

Therefore, it's important that any state mutation is done only outside of the big tracked promise, because it's only then that we know we have fresh data.

runConcurrentSafePromise(
  props
    .getSources({
      /* ... */
    })
    .then((sources) => {
      return Promise.all(
        sources.map((source) =>
          source.getItems({
            /* ... */
          })
        )
      ).then((items) => {
        // This promise isn't individually tracked so `items` isn't guaranteed
        // to be fresh. In this `then`, we should never perform side-effects
        // like mutating the Autocomplete state.
        return items;
      });
    })
).then((collections) => {
  // We know the data is fresh because we controlled it before returning it
  // It's fine to perform state mutations here.

  setCollections(collections);
});

[francoischalifour]

Right, but the state mutations already happened after getSources, right? Besides, we still need to call setStatus('loading') before the root promise is resolved, so I find that part of the comment confusing.

[sarahdayan]

Right, but the state mutations already happened after getSources, right?

Yes, that's a warning for future readers. How would you phrase it?

Besides, we still need to call setStatus('loading') before the root promise is resolved

Yeah, I saw. But with this call, we might have situations where the status goes back to loading just because a really long, stale call to getSources finally resolved, even if the user is no longer typing and has received results.

Do we really need this call? We do set the status to loading in l. 75. If the chain takes a while to resolve and the search goes to stalled, why would we want to go back to loading when we only have the sources? There's nothing to display at this stage until we have items.

[francoischalifour]

That's right, I don't think we need this setStatus.

Regarding the comment, the word "outside" is confusing:

Suggested change

	// meaning we should only ever manipulate the state outside of this call.
	// meaning we should only ever manipulate the state once this concurrent-safe
	// promise is resolved.

[sarahdayan]

Addressed in 8de5df5.