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

[zero1five]

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

[Trott]

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

[zero1five]

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

[Trott]

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?

[zero1five]

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

[Trott]

@nodejs/timers

[Fishrock123]

I don't really understand what was incorrect before?

[Fishrock123]

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?

[zero1five]

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

[Fishrock123]

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

[zero1five]

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.

[BridgeAR]

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.

[BridgeAR]

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