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

timers: truncate decimal values #24819

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@Fishrock123
Copy link
Member

Fishrock123 commented Dec 4, 2018

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.


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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@Fishrock123 Fishrock123 added the timers label Dec 4, 2018

@Fishrock123 Fishrock123 requested a review from apapirovski Dec 4, 2018

@Fishrock123 Fishrock123 force-pushed the Fishrock123:timers-non-integer-delay branch from a0b2aa4 to 77d6fdf Dec 4, 2018

@Fishrock123

This comment has been minimized.

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Dec 4, 2018

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.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Dec 4, 2018

libuv doesn't even support non-integer / sub-millisecond timers

Adding a uv_hrtimer_t handle type isn't out of the question, if there is a need for it.

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 4, 2018

libuv doesn't even support non-integer / sub-millisecond timers

Adding a uv_hrtimer_t handle type isn't out of the question, if there is a need for it.

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 Fishrock123 force-pushed the Fishrock123:timers-non-integer-delay branch from 77d6fdf to 387130e Dec 4, 2018

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Dec 7, 2018

I’m going to be testing the issue I have in mind tomorrow.

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Dec 7, 2018

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.

That commit is semver-major afaik. Not sure I follow.

@danbev

This comment has been minimized.

@trivikr

trivikr approved these changes Dec 8, 2018

@danbev

This comment has been minimized.

Copy link
Member

danbev commented Dec 11, 2018

Re-run of failing node-test-commit-osx

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 11, 2018

@apapirovski did you get around to testing it?

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Dec 13, 2018

@Fishrock123 you need to make sure that any time something like lists[item._idleTimeout] occurs, we use the truncated value. So line 384, needs to be lists[Math.trunc(item._idleTimeout)]. Then the behavior should be good. I'm going to spot check if any other places are affected.

@Fishrock123 Fishrock123 force-pushed the Fishrock123:timers-non-integer-delay branch from 387130e to 4c0f21e Dec 13, 2018

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 13, 2018

@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..)

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Dec 13, 2018

Potentially dumb question: why don't we truncate during setTimeout() or setInterval()?

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 14, 2018

@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 active() is deprecated, this will no longer be an issue, hopefully.

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Dec 19, 2018

test.parallel/test-child-process-execfile failed on nodes=alpine-latest-x64, I don't know why but as far as I am aware there aren't any other tests with non-decimal timeouts.

assert.js:753
    throw newErr;
    ^

AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/out/Release/node /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/test/fixtures/exit.js 0

    at ChildProcess.exithandler (child_process.js:294:12)
    at ChildProcess.emit (events.js:189:13)
    at maybeClose (internal/child_process.js:978:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:265:5)

@Fishrock123 Fishrock123 force-pushed the Fishrock123:timers-non-integer-delay branch from 4c0f21e to 9c7c99e Jan 14, 2019

@Fishrock123

This comment was marked as outdated.

Copy link
Member Author

Fishrock123 commented Jan 14, 2019

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 23, 2019

Another rebase + CI. Hopefully everything is more stable this time around.

https://ci.nodejs.org/job/node-test-pull-request/20295/

Edit: again, after rebase to pull in more test fixes https://ci.nodejs.org/job/node-test-pull-request/20378/

timers: truncate decimal values
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.

@Fishrock123 Fishrock123 force-pushed the Fishrock123:timers-non-integer-delay branch from 339e7b8 to 10e70b6 Jan 28, 2019

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Jan 29, 2019

Landed in a7c66b6

@Fishrock123 Fishrock123 deleted the Fishrock123:timers-non-integer-delay branch Jan 29, 2019

Fishrock123 added a commit that referenced this pull request Jan 29, 2019

timers: truncate decimal values
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>

targos added a commit that referenced this pull request Jan 29, 2019

timers: truncate decimal values
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>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment