timers: truncate decimal values #24819
Conversation
Fishrock123 added the timers 
Fishrock123 force-pushed the timers-non-integer-delay branch from a0b2aa4 to 77d6fdf Compare 
| cc @nodejs/timers |
| Approved but there might be a few edge cases that I need to test. Will let you know. If possible, please avoid merging until then. |
Adding a |
Right, but we probably want to have that discussion elsewhere (if we want to have it), since there is some broken behavior in here currently. |
Fishrock123 force-pushed the timers-non-integer-delay branch from 77d6fdf to 387130e Compare 
| I’m going to be testing the issue I have in mind tomorrow. |
That commit is semver-major afaik. Not sure I follow. |
| Re-run of failing node-test-commit-osx |
| @apapirovski did you get around to testing it? |
| @Fishrock123 you need to make sure that any time something like |
Fishrock123 force-pushed the timers-non-integer-delay branch from 387130e to 4c0f21e Compare | @apapirovski Done, looks like just covering that spot is good enough. New CI: https://ci.nodejs.org/job/node-test-pull-request/19491/ (guess I maybe should add a test for that case though..) |
| Potentially dumb question: why don't we truncate during |
| @TimothyGu not dumb. The reason is that the timer api is... well... a bit too flexible due to historic reasons, so this way more reliably catches what a user could feed in. One day, probably a year or so after |
|
|
Fishrock123 force-pushed the timers-non-integer-delay branch from 4c0f21e to 9c7c99e Compare This comment has been minimized.
This comment has been minimized.
Fishrock123 force-pushed the timers-non-integer-delay branch from 9c7c99e to 339e7b8 Compare |
Edit: again, after rebase to pull in more test fixes https://ci.nodejs.org/job/node-test-pull-request/20378/ |
Reverts some timers behavior back to as it was before 2930bd1 That commit introduced an unintended change which allowed non-integer timeouts to actually exist since the value is no longer converted to an integer via a TimeWrap handle directly. Even with the fix in e9de435 non-integer timeouts are still indeterministic, because libuv does not support them. This fixes the issue by emulating the old behavior: truncate the `_idleTimeout` before using it. See comments in nodejs#24214 for more background on this.
Fishrock123 force-pushed the timers-non-integer-delay branch from 339e7b8 to 10e70b6 Compare | Landed in a7c66b6 |
Reverts some timers behavior back to as it was before 2930bd1 That commit introduced an unintended change which allowed non-integer timeouts to actually exist since the value is no longer converted to an integer via a TimeWrap handle directly. Even with the fix in e9de435 non-integer timeouts are still indeterministic, because libuv does not support them. This fixes the issue by emulating the old behavior: truncate the `_idleTimeout` before using it. See comments in #24214 for more background on this. PR-URL: #24819 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Reverts some timers behavior back to as it was before 2930bd1 That commit introduced an unintended change which allowed non-integer timeouts to actually exist since the value is no longer converted to an integer via a TimeWrap handle directly. Even with the fix in e9de435 non-integer timeouts are still indeterministic, because libuv does not support them. This fixes the issue by emulating the old behavior: truncate the `_idleTimeout` before using it. See comments in #24214 for more background on this. PR-URL: #24819 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
11 participants
Reverts some timers behavior back to as it was before 2930bd1.
That commit introduced an unintended change which allowed non-integer timeouts to actually exist since the value is no longer converted to an integer via a TimeWrap handle directly.
Even with the fix in e9de435 non-integer timeouts are still indeterministic, because libuv does not support them.
This fixes the issue by emulating the old behavior: truncate the
_idleTimeoutbefore using it.See comments in #24214 for more background on this.
In hindsight, I actually think this discovery makes 2930bd1
semver-major. I think that commit has perhaps spread too far to just 'undo' it, so I think reverting the behavior (not necessarily the code) is the best choice of action.Note: libuv doesn't even support non-integer / sub-millisecond timers so really we can't even support sub-millisecond timers (the only real use for non-integer timeouts).
(I don't think supporting sub-millisecond timers is particularly useful, either.)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes