lib: remove redundant code from timers.js by Trott · Pull Request #3143 · nodejs/node

Trott · 2015-10-01T05:21:46Z

insert() is only called from one place where there is already a check
that msecs is greater than or equal to zero, so do not repeat the check
inside insert().

Trott · 2015-10-01T05:24:19Z

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

Fishrock123 · 2015-10-01T06:10:30Z

Seems to have originated along with the start of timers, ala pre- bc47353, perhaps insert was once used in more than one place. Not sure.

LGTM. Can you ensure there is something to test that negative timeouts do not fire?

thefourtheye · 2015-10-01T08:27:08Z

Can you leave this check and remove the one at the place where it is called?

Trott · 2015-10-02T15:40:10Z

@Fishrock123 Regarding testing negative timeouts: test/parallel/test-timers.js has a bunch of tests using setTimeout() and setInterval() for various negative numbers. But the very first thing setTimeout() and setInterval() both do is change any value less than 1 to 1. So active() never actually gets the negative number. Does that cover the situation you are concerned about? Or not quite because of the guard code in setTimeout() and setInterval()?

Since active() is exported by timers.js, I can try to rig up a test calling it directly. But I dont' think active() should actually be exported. I can't find anything else that uses it, it's not documented, and the argument it requires is awfully specific to timers.js internal working, so I think it should be deprecated and made internal to timers.js. (But uh, that's a much bigger thing than I'd want to tackle with this small PR which I thought was going to be a two-line change...)

Trott · 2015-10-02T15:57:21Z

@thefourtheye Yes, that is a slightly better approach. Thanks for suggesting it. I've made the change and pushed.

thefourtheye · 2015-10-02T16:04:08Z

LGTM.

Fishrock123 · 2015-10-02T16:08:56Z

isn't this line also redundant?

Yes. In fact, the whole separation of insert() from active() at this point seems redundant. Just pushed an additional commit.

Fishrock123 · 2015-10-02T16:13:45Z

Yes, active is public but should not be -- see: #896

(I'll deal with this soon(tm).)

I'd add a test anyways since it currently is public, if the others don't already test it. I haven't looked.

Trott · 2015-10-02T21:57:41Z

@Fishrock123 Per your suggestion, I've added a test for timers.active(). Thanks.

Trott · 2015-10-03T02:42:11Z

@Fishrock123 Regarding #896, if (and only if) you want, I'd be happy to take the first step of that off your plate and make those APIs private within the file and replace the existing exposed functions with deprecation warning-wrapped versions and put it in a semver major PR.

Fishrock123 · 2015-10-05T19:27:36Z

@Trott not worthwhile I think. They should be reworked at the same time. I'll look at it once I'm back from vacation.

Fishrock123 · 2015-10-05T19:29:01Z

This LGTM if it works & CI is happy.

Trott · 2015-10-05T22:58:23Z

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

thefourtheye · 2015-10-06T02:11:31Z

Why not just hasOwnProperty? Also I feel that if we can check the values also it would be better.

Shouldn't this check _idleStart also?

Also note that we do now have notDeepStrictEqual :D

@thefourtheye @Fishrock123 OK, I've improved the tests quite a bit, I think. Looks good to you now?

insert() is only called from one place where there is already a check that msecs is greater than or equal to zero, so do not repeat the check inside insert(). timers.active() is not documented and should not be exposed, but since it is exposed for now, let's test it.

Trott · 2015-10-06T04:15:46Z

Made the tests a fair bit more rigorous with input from @thefourtheye and @Fishrock123. New CI: https://ci.nodejs.org/job/node-test-pull-request/426/

Fishrock123 · 2015-10-06T04:16:55Z

should, or should not?

AH, this should be "should". :)

Fixed the typo. Thanks for the catch.

thefourtheye · 2015-10-06T05:28:44Z

Nit: these?

thefourtheye · 2015-10-06T05:29:16Z

LGTM

insert() is only called from one place where there is already a check that msecs is greater than or equal to zero, so do not repeat the check inside insert(). timers.active() is not documented and should not be exposed, but since it is exposed for now, let's test it. PR-URL: nodejs#3143 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott · 2015-10-06T15:29:25Z

Landed in 070aac4

insert() is only called from one place where there is already a check that msecs is greater than or equal to zero, so do not repeat the check inside insert(). timers.active() is not documented and should not be exposed, but since it is exposed for now, let's test it. PR-URL: #3143 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 1, 2015

Trott force-pushed the timer-tweak branch from bd6ec3f to 0d1a625Compare October 2, 2015 15:19

Trott force-pushed the timer-tweak branch from 0d1a625 to 6e752ddCompare October 2, 2015 15:57

Fishrock123 reviewed Oct 2, 2015
View reviewed changes

thefourtheye reviewed Oct 6, 2015
View reviewed changes

Trott added 2 commits October 5, 2015 20:55

fixup: improve test
4271293

Trott force-pushed the timer-tweak branch from 879e1f2 to 4271293Compare October 6, 2015 04:13

Fishrock123 reviewed Oct 6, 2015
View reviewed changes

typo fix
17e09ec

thefourtheye reviewed Oct 6, 2015
View reviewed changes

Comment threadtest/parallel/test-timers-active.js
thefourtheyeOct 6, 2015
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these?

Trott closed this Oct 6, 2015

jasnell mentioned this pull request Oct 8, 2015
Preparing v4.x for LTS #3252
Closed
29 tasks

jasnell added the lts-landed-on-v4.x label Oct 8, 2015

jasnell mentioned this pull request Oct 8, 2015
Release proposal: v4.2.0 'Argon' (LTS) #3258
Closed

webzwo0i mentioned this pull request Oct 13, 2015
Running etherpad-lite on nodejs 4.2.0 doesn't connect to Mysql ether/etherpad#2803
Closed

Trott deleted the timer-tweak branch January 13, 2022 22:28

Uh oh!

Uh oh!

Conversation

Trott commented Oct 1, 2015

Uh oh!

Trott commented Oct 1, 2015

Uh oh!

Fishrock123 commented Oct 1, 2015

Uh oh!

thefourtheye commented Oct 1, 2015

Uh oh!

Trott commented Oct 2, 2015

Uh oh!

Trott commented Oct 2, 2015

Uh oh!

thefourtheye commented Oct 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Oct 2, 2015

Uh oh!

Trott commented Oct 2, 2015

Uh oh!

Trott commented Oct 3, 2015

Uh oh!

Fishrock123 commented Oct 5, 2015

Uh oh!

Fishrock123 commented Oct 5, 2015

Uh oh!

Trott commented Oct 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Oct 6, 2015

Uh oh!

Trott commented Oct 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants