timers: fix refresh did not call active to make the node exit early. by zero1five · Pull Request #28192 · nodejs/node · GitHub

Refresh.() does not get a timer active that has already been invoked. Add an or option for put refresh back to work.

const timer = setTimeout(() => {
  console.log(1); // emit once
}, 1);

setTimeout(() => {
  timer.refresh();
}, 1);

#26721 one reason it can't be overridden it only works in the callback of the current timer(before finally).

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

The test does not fail without this change. Are we not able to reliably reproduce an issue that this change addresses?

@Trott The test failed in v13.0.0-pre and master

const timer = setTimeout(() => {
  console.log(1); // emit once
}, 1);

setTimeout(() => {
  timer.refresh();
}, 1);

This should be printed twice.

edited: I will change about a valid example during the day, I am too tired. AM4.00...

const timer = setTimeout(() => {
console.log(1); // emit once
}, 1);

setTimeout(() => {
timer.refresh();
}, 1);

It only prints once for me. Perhaps it is platform and/or environment dependent?

@Trott I think it has little to do with platform, because this is a 🐛 at the js level.

node/lib/internal/timers.js

Lines 505 to 544 in 43a5170

    
           // The actual logic for when a timeout happens. 
        
           L.remove(timer); 
        
           const asyncId = timer[async_id_symbol]; 
        
           if (!timer._onTimeout) { 
        
             if (timer[kRefed]) 
        
               refCount--; 
        
             timer[kRefed] = null; 
        
             if (destroyHooksExist() && !timer._destroyed) { 
        
               emitDestroy(asyncId); 
        
               timer._destroyed = true; 
        
             } 
        
             continue; 
        
           } 
        
           emitBefore(asyncId, timer[trigger_async_id_symbol]); 
        
           let start; 
        
           if (timer._repeat) 
        
             start = getLibuvNow(); 
        
           try { 
        
             const args = timer._timerArgs; 
        
             if (args === undefined) 
        
               timer._onTimeout(); 
        
             else 
        
               timer._onTimeout(...args); 
        
           } finally { 
        
             if (timer._repeat && timer._idleTimeout !== -1) { 
        
               timer._idleTimeout = timer._repeat; 
        
               if (start === undefined) 
        
                 start = getLibuvNow(); 
        
               insert(timer, timer[kRefed], start); 
        
             } else if (!timer._idleNext && !timer._idlePrev) { 
        
               if (timer[kRefed]) 
        
                 refCount--; 
        
               timer[kRefed] = null;

L.remove(timer) will delete _idleNext and _idlePrev make !timer._idleNext && !timer._idlePrev invalid.

@nodejs/timers

I don't really understand what was incorrect before?

Does this change / fix a problem that could be described as "timer.refresh() does not re-activate a timer which has already timed out"? Does it only change that case?

I don't really understand what was incorrect before?

I'm well sorry about my English, The most accurate description: When refresh calls a timer that has already been called, the current timer cannot be re-marked as an active event in the event loop. If there is no other event activity, node will exit directly.

Does this change / fix a problem that could be described as "timer.refresh() does not re-activate a timer which has already timed out"?

Can not... I looked at the 10.x code and it seems to be a bit different.

Does it only change that case?

I think yes, timer[kRefed] is null only after initialization and after invocation(for master).

Add an or option for put refresh back to work. nodejs#26721 one reason it can't be overridden it only works in the callback of the current timer(before `finally`).

Ok I think this may also be fixed by #27345, but this might be able to land sooner / be backported more. cc @apapirovski

If #27345 works well with 10.x, I like it better because it's more like a refactoring, and this one is a bit like monkey patch.

To make sure that the test fails on older platforms it's probably useful to start a child process and to log something. That way the main process could verify the output and the broken behavior would be observable.

Closing, since #27345 landed. Please leave a comment if this is not fixed by that PR!

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jun 12, 2019

Trott requested changes Jun 12, 2019

View reviewed changes

zero1five force-pushed the timer/fix-end-loop branch from f1054f5 to 261530e Compare June 12, 2019 19:42

zero1five changed the title timers: fix refresh too early to exit timers: fix refresh not call active make node well early to exit Jun 13, 2019

zero1five changed the title timers: fix refresh not call active make node well early to exit timers: fix refresh did not call active to make the node exit early. Jun 13, 2019

Fishrock123 reviewed Jun 13, 2019

View reviewed changes

Comment thread test/parallel/test-timers-refresh-called.js Outdated

timers: fix refresh too early to exit

33ab12e

Add an or option for put refresh back to work. nodejs#26721 one reason it can't be overridden it only works in the callback of the current timer(before `finally`).

zero1five force-pushed the timer/fix-end-loop branch from 261530e to 33ab12e Compare June 13, 2019 18:00

BridgeAR reviewed Jul 5, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

BridgeAR closed this Jan 12, 2020

timers: fix refresh did not call active to make the node exit early. by zero1five · Pull Request #28192 · nodejs/node · GitHub

Uh oh!

Uh oh!

timers: fix refresh did not call active to make the node exit early. - #28192

timers: fix refresh did not call active to make the node exit early.#28192zero1five wants to merge 1 commit intonodejs:masternodejs/node:masterfrom zero1five:timer/fix-end-loopzero1five/node:timer/fix-end-loopCopy head branch name to clipboard

Conversation

zero1five commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

zero1five commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jun 12, 2019

Uh oh!

zero1five commented Jun 13, 2019

Uh oh!

Trott commented Jun 13, 2019

Uh oh!

Uh oh!

Fishrock123 commented Jun 13, 2019

Uh oh!

Fishrock123 commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zero1five commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Jun 13, 2019

Uh oh!

zero1five commented Jun 13, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

timers: fix refresh did not call active to make the node exit early.#28192
zero1five wants to merge 1 commit into
nodejs:masterfrom
zero1five:timer/fix-end-loop

zero1five commented Jun 12, 2019 •
edited
Loading

zero1five commented Jun 12, 2019 •
edited
Loading

Fishrock123 commented Jun 13, 2019 •
edited
Loading

zero1five commented Jun 13, 2019 •
edited
Loading