Skip to content

Conversation

akenneth
Copy link
Contributor

Overview

While using memoize in our code we identified an issue where the order of promise execution got scrambled by the use of memoize.
This PR fixes this issue.

Root cause

The first call to memoize with a certain key returns one promise, all subsequent calls to memoize with the same key return a different promise.
This is because the first call creates a new promise with the .catch call and returns it, but caches the original "inner" promise.
Following calls to memoize return only the "inner" promise.

Suggested fix

In the case of having a promise object as the result of the evaluated function, cache the complex promise with the catch call in it.
This way the first and subsequent calls will return the same promise and all will register their events on it.

Details:

We are using memoize to memoize the suggested autocomplete entries for a text input in our web page, they all have the same cache key.
When the user is typing a string into the input, each letter invokes the onchange event that uses memoize and awaits the promise that fetches data from the server.

What we see is that when the promise returned by the memoized function is resolved, the first caller's then method is being invoked last (instead of first as expected).

Repro steps:

  1. Call memoize with a promise returning function with key 'a' and await it.
  2. While it's executing have one (or more) calls to the same key and await it.

Expected behavior - the then statement from item 1 is invoked before the then statement of item 2.

Note: also see the unit test in this PR to illustrate this scenario. This test is failing in the current master branch.

@akenneth akenneth requested a review from a team as a code owner January 18, 2021 10:40
@akenneth
Copy link
Contributor Author

Thanks @anleac for helping out here!

Copy link

@octatone octatone left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@akenneth
Copy link
Contributor Author

@keithamus should I bump the package version as well?

Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

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

This looks good to me. I did a bit of red-green testing to make sure that the test fails when with the original code in master and it checks out on my machine.

I'll leave the final approval to @keithamus

@koddsson
Copy link
Contributor

@keithamus should I bump the package version as well?

Normally we do that as part of the publishing process after merging the PR.

This test can be run in isolation and more readily describes the
expected pattern of behavior. This simplifies debugging later.
The caught Promise _must_ be allowed to be re-caught to allow for proper
rejection handling. This test affirms that.
The earlier test which affirms Promise equality with subsequent calls
guarantees the behavior of sequential ordering as per the spec. It is
pointless to test this as if the test breaks it is a bug with the JS
engine and one we're unlikely able to fix (or is at least out of the
scope of this project). Our interest is only in the earlier test of
referential equality.

This test also tests the cache implementation, but this is needless as
there are other tests which affirm these semantics.
@keithamus
Copy link
Contributor

I've simplified the test cases to be more explicit about the behaviours we expect:

  1. That memoized calls return the same Promise each call.
  2. That the memoized Promise can be caught without leaking errors.

I think this is good to go now 😉 😄

@keithamus keithamus merged commit 260db15 into github:master Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants