Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dylang opened this issue Nov 29, 2021 · 7 comments

Comments

@dylang
Copy link

dylang commented Nov 29, 2021

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
Copy link
Owner

// @niksy

@niksy
Copy link
Sponsor

niksy commented Nov 30, 2021

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
Copy link
Author

dylang commented Dec 1, 2021

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: SamVerschueren/map-age-cleaner#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
Copy link
Sponsor

niksy commented Dec 3, 2021

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
Copy link
Owner

Sounds good to me 👍

Happy new year 🎉

niksy added a commit to niksy/p-memoize that referenced this issue Jan 3, 2022
@niksy
Copy link
Sponsor

niksy commented Jan 3, 2022

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

@sindresorhus
Copy link
Owner

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

No branches or pull requests

3 participants