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

Fix refresh creating an infinite timeout loop #161

Merged
merged 3 commits into from
May 28, 2021
Merged

Fix refresh creating an infinite timeout loop #161

merged 3 commits into from
May 28, 2021

Conversation

Thomas101
Copy link
Contributor

If the date is more than a week away, then the refresh period would be set to Infinity. This gets passed to a setTimeout, which fires immediately causing a refresh loop, which in my instance resulted in 100% cpu usage.

It could also be the cause of #160

The code already does a true-y check on period before issuing the setTimeout, this patch also explicitly checks for Infinity to ensure that the timeout doesn't fire

Infinite timeouts are a bit of a js quirk that really don't behave as expected - https://github.com/denysdovhan/wtfjs#an-infinite-timeout

@nmn
Copy link
Owner

nmn commented May 27, 2021

If Infinity is causing this problem, let’s replace Infinity with a very large number instead?

…llout timeout to a week rather than infinity
@Thomas101
Copy link
Contributor Author

Yeah that sounds reasonable. I don't know if you're thinking very large as in Number.MAX_SAFE_INTEGER or just far enough in the future.

I set it to a week for now, unless it's a long-running app then it should never fire and if it is running for a long period a week makes sense from the unboundPeriod tree and performance point of view. Happy to update the PR if you think Number.MAX_SAFE_INTEGER or something else would be more suitable.

Also changed the maxPeriod default option. This was set to Infinity so without the guard invokes the same bug.

@nmn
Copy link
Owner

nmn commented May 27, 2021

Would you mind bumping the minor version number as well?

@nmn nmn merged commit 6b2b657 into nmn:master May 28, 2021
@namoscato namoscato mentioned this pull request May 29, 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.

2 participants