timers: fixing API refs to use safe internal refs #2500
Conversation
Added safe internal references for 'clearTimeout(..)', 'active(..)', and 'unenroll(..)'. Changed various API refs from 'export.*' to use these safe internal references. Now, overwriting the global API identifiers does not create potential breakage and/or race conditions. See Issue #2493.
Fishrock123
added
the
timers
|
The only failures appear to be two of the arm builds on raspberry pi's, and the failures seem to be related to not being able to reach a URL. |
| @@ -158,7 +158,7 @@ exports.enroll = function(item, msecs) { | |||
|
|
|||
| // call this whenever the item is active (not idle) | |||
| // it will reset its timeout. | |||
| exports.active = function(item) { | |||
| const active = exports.active = function(item) { | |||
There was a problem hiding this comment.
I've been thinking about it and I think it would be best to only do this for the globals, so only clearTimeout in this patch. (Plus we also don't test active/{un}enroll and there's not a terribly good reason to.
|
@getify Sorry for taking a bit. I'm not sure if it is better to define these locally or just always use the ones on |
|
@Fishrock123 @getify ... any further thoughts on this one? |
|
Closing due to lack of activity or response. |
|
I would still very much like this to be addressed. I'm not sure what else I can do? Please re-open. |
|
@getify... done! :-) can you rebase to make sure it's up to date. @Fishrock123 , would you have a chance to give this another review? |
|
I'll also review it once it's rebased |
|
If you want to get a head start on reviewing the rebased version: https://github.com/nodejs/node/compare/master...Trott:fix-timers-api-refs?expand=1 I resolved a couple conflicts and it's possible @getify will choose to resolve them differently, but the choices I made pass linting and testing.... |
|
Thanks @Trott ... I don't see anything in your version that I'd disagree with. 👍 |
|
@getify Probably best to get it in this PR so that the conversation thread isn't lost. Maybe clone my branch to your local working repo and force push it up to your branch on GitHub? |
|
I'm going to go ahead and make a PR from the rebase I did. @getify, if you're planning on updating this branch instead or if you otherwise would prefer I not proceed, let me know! |
|
Thanks for taking the lead, @Trott. Been a bit swamped and hadn't got to this yet. |
Added safe internal references for 'clearTimeout(..)', 'active(..)', and
'unenroll(..)'. Changed various API refs from 'export.*' to use these
safe internal references.
Now, overwriting the global API identifiers does not create potential
breakage and/or race conditions. See Issue #2493.