sindresorhus / p-memoize

Memoize promise-returning & async functions
MIT License
396 stars 30 forks source link

while waiting for promise to resolve, maxAge is Number.POSITIVE_INFINITY which breaks mapAgeCleaner #39

Closed dylang closed 2 years ago

dylang commented 3 years ago

Version

This is from p-memoize@4.0.3 and possibly caused by #37.

Stack

warning (TimeoutOverflowWarning)  Infinity does not fit into a 32-bit signed integer.
Timeout duration was set to 1.
    at new Timeout (internal/timers.js:169:15)
    at setTimeout (timers.js:164:19)
    at ./node_modules/map-age-cleaner/dist/index.js:42:31
    at Generator.next (<anonymous>)
    at ./node_modules/map-age-cleaner/dist/index.js:7:71
    at new Promise (<anonymous>)
    at __awaiter (./node_modules/map-age-cleaner/dist/index.js:3:12)
    at setupTimer (./node_modules/map-age-cleaner/dist/index.js:27:38)
    at ./node_modules/map-age-cleaner/dist/index.js:58:23
    at Generator.next (<anonymous>)

Theory

  1. Before the promise is resolve, we are setting maxAge to Number.POSITIVE_INFINITY. https://github.com/sindresorhus/p-memoize/blob/version-4/index.js#L29
  2. Until pSettle is resolved (https://github.com/sindresorhus/p-memoize/blob/version-4/index.js#L32) maxAge will continue to be Number.POSITIVE_INFINITY.
  3. When the next call occurs, mapAgeCleaner is run. https://github.com/sindresorhus/p-memoize/blob/version-4/index.js#L13
  4. mapAgeCleaner does not check if maxAge is a safe integer. It uses setTimeout(..., maxAge).
  5. If the original memoized promise has not been resolved (which would set maxAge to an integer), then setTimeout throws the above stack because Number.POSITIVE_INFINITY is not a 32 bit number.
sindresorhus commented 3 years ago

// @niksy

niksy commented 3 years ago

I don’t know if we should fix this here or in map-age-cleaner.

@dylang Do you have reproducible test case so we can create test for it?

dylang commented 3 years ago

Hi @niksy - thanks for taking a look at this.

Here's a minimal reproducible case: https://stackblitz.com/edit/p-memoize-issues-39?file=index.js

I agree map-age-cleaner should address this. It was reported there in April and not addressed: https://github.com/SamVerschueren/map-age-cleaner/issues/8

Maybe we could use Number.MAX_SAFE_INTEGER instead of Number.POSITIVE_INFINITY as a workaround? That's 285,427 years, which hopefully our promises have been resolved by then. 😅

BTW, it seems that removing maxAge hides the issue.

niksy commented 2 years ago

Maybe we could use Number.MAX_SAFE_INTEGER instead of Number.POSITIVE_INFINITY as a workaround? That's 285,427 years, which hopefully our promises have been resolved by then. 😅

Unfortunately, Number.MAX_SAFE_INTEGER throws same warning, since that’s also not 32-bit signed integer. Maximum value is 2147483647 and that’s (2 ** 31) - 1 which is like 596 hours. This seems like a reasonable solution if we don’t want to wait for the fix in map-age-cleaner. What do you think @sindresorhus ?

sindresorhus commented 2 years ago

Sounds good to me 👍

Happy new year 🎉

niksy commented 2 years ago

@sindresorhus done! And happy new year to you all!

sindresorhus commented 2 years ago

https://github.com/sindresorhus/p-memoize/releases/tag/v4.0.4