-
Notifications
You must be signed in to change notification settings - Fork 7.3k
The timers' ms arg should be an integer #897
Conversation
Timers do strange things when it isn't.
Patch LGTM. What strange things did you observe? Anyway, I just tested and |
The weird thing that happens when passing a float is that it takes longer to fire than expected. This demonstrates the problem https://gist.github.com/932844 You can pass strings: |
Seems like the Bad Thing happens when passing in a number between 0 and 1. JS doens't have sub-millisecond precision anyhow, so flooring is the right thing to do. |
i don't think there is any problem. we already take the IntegerValue in the binding. |
@ry: Somehow it ends up doing a setTimeout for 1000 when provided a number between 0 and 1. Try it:
When provided with a float greater than 1, it works as expected. |
ah okay. then it is a bug in the binding and should be fixed there. test should be added too. |
`test-regress-nodejsGH-897` is dependent on a timer firing within a period of time. Especially on some of the FreeBSD hosts on CI, we have seen tests like that fail when run in parallel. (This may have nothing to do with FreeBSD and may just mean that the hosts are resource-constrained.) Move this test to sequential as we have done with several other timer-dependent tests recently. The test has also been refactored and documented via comments. PR-URL: nodejs/node#9487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
`test-regress-nodejsGH-897` is dependent on a timer firing within a period of time. Especially on some of the FreeBSD hosts on CI, we have seen tests like that fail when run in parallel. (This may have nothing to do with FreeBSD and may just mean that the hosts are resource-constrained.) Move this test to sequential as we have done with several other timer-dependent tests recently. The test has also been refactored and documented via comments. PR-URL: nodejs/node#9487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
`test-regress-nodejsGH-897` is dependent on a timer firing within a period of time. Especially on some of the FreeBSD hosts on CI, we have seen tests like that fail when run in parallel. (This may have nothing to do with FreeBSD and may just mean that the hosts are resource-constrained.) Move this test to sequential as we have done with several other timer-dependent tests recently. The test has also been refactored and documented via comments. PR-URL: nodejs/node#9487 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
In test-timers, confirm that all input values that should be coerced to 1 ms are not being coerced to a significantly larger value. This eliminates the need for the separate test-regress-nodejsGH-897. PR-URL: nodejs/node#10960 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Even after being moved to `sequential` in 1ce05ad, `test-regress-nodejsGH-897` still was occasionally flaky on Raspberry Pi devices on CI. The test is especially sensitive to resource constraints. It failed reliably on my laptop if I moved it to `parallel` and ran 32 competing node test processes. Even for a flaky test, that's unusually low. I typically don't see problems, even for flaky tests, until I get up to around four times that number. On a Raspberry Pi, of course, that sensitivity to resource constraints will manifest much sooner. This change checks the order of timers firing, rather than the duration before a timer is fired. This eliminates the sensitivity to resource constraints. The test can now be moved back to `parallel`. I am able to run many copies of the test simultaneously without seeing test failures. PR-URL: nodejs/node#10903 Fixes: nodejs/node#10073 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
In test-timers, confirm that all input values that should be coerced to 1 ms are not being coerced to a significantly larger value. This eliminates the need for the separate test-regress-nodejsGH-897. PR-URL: nodejs/node#10960 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
setTimeout() / setInterval() do strange things when it isn't.