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

Handle very long timeouts in Hoek.wait() #373

Merged
merged 1 commit into from
Apr 23, 2022
Merged

Conversation

kanongil
Copy link
Contributor

This enhances Hoek.wait() to properly handle timeouts of more than 2^31 - 1 ms.

The existing implementation makes no mention of the fact that it is based on setTimeout() and its inherent limitation of 2^31 - 1 ms. As it is right now, any value larger than this (which equates to around 25 days), is treated as if 1 was passed, and triggers a TimeoutOverflowWarning from node:

(node:99363) TimeoutOverflowWarning: 2147483648 does not fit into a 32-bit signed integer.
Timeout duration was set to 1.

While fixing this, I also took the opportunity to support timeout values using BigInt numbers, which I think makes sense for a modern API.

Note that the implementation now returns a never resolving Promise for exceptionally long timeout values (including Infinity), making it work similar to Hoek.block().

Alternatives considered

Update the docs to note the limitation, and immediately throw an error if passed values larger than 2^31 - 1.

The implementation could also be more accurate for longs waits, but it requires manually tracking elapsed time, which does not seem worth the effort. The API should still wait at least the time specified.

@kanongil kanongil added bug Bug or defect feature New functionality or improvement labels Oct 25, 2021
Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

LGTM just a little comment

lib/wait.js Show resolved Hide resolved
Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

Nicely done @kanongil. I like how you approached it. Thanks for the improvement.

@devinivy devinivy added this to the 9.3.0 milestone Apr 23, 2022
@devinivy devinivy merged commit 71250a0 into hapijs:master Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants