Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

The timers' ms arg should be an integer #897

Closed
wants to merge 1 commit into from
Closed

The timers' ms arg should be an integer #897

wants to merge 1 commit into from

Conversation

xk
Copy link

@xk xk commented Apr 11, 2011

setTimeout() / setInterval() do strange things when it isn't.

Timers do strange things when it isn't.
@felixge
Copy link

felixge commented Apr 13, 2011

Patch LGTM.

What strange things did you observe?

Anyway, I just tested and setTimeout seems to accept timeouts as strings in the browser, so doing it the same in node is probably a good idea.

@xk
Copy link
Author

xk commented Apr 13, 2011

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:
[Math.floor('3.14'), typeof Math.floor('3.14')] -> [ 3, 'number' ]

@isaacs
Copy link

isaacs commented Apr 22, 2011

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.

@ry
Copy link

ry commented Apr 22, 2011

i don't think there is any problem. we already take the IntegerValue in the binding.

@isaacs
Copy link

isaacs commented Apr 22, 2011

@ry: Somehow it ends up doing a setTimeout for 1000 when provided a number between 0 and 1. Try it:

var t = Date.now()
setTimeout(function () {
  console.error(Date.now() - t)
}, 0.1)

When provided with a float greater than 1, it works as expected.

@ry
Copy link

ry commented Apr 22, 2011

ah okay. then it is a bug in the binding and should be fixed there. test should be added too.

@ry ry closed this in c8e447e Apr 22, 2011
isaacs pushed a commit to isaacs/node-v0.x-archive that referenced this pull request Dec 8, 2016
`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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
gdams pushed a commit to ibmruntimes/node that referenced this pull request Jan 4, 2017
`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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
gdams pushed a commit to ibmruntimes/node that referenced this pull request Jan 4, 2017
`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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Mar 22, 2017
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request May 11, 2017
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request May 11, 2017
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants