timers: fix processing of nested same delay timers

[whitlockjc]

This is a port of https://github.com/nodejs/nodejs/node-v0.x-archive/pull/17203 and nodejs/node-v0.x-archive#25763 which fixesnodejs/node-v0.x-archive#9333, nodejs/node-v0.x-archive#15447, nodejs/node-v0.x-archive#25607 and #5426.

/cc @Fishrock123, @misterdjules, @nodejs/collaborators and @nodejs/tsc

[mscdex]

Why add this for just one test?

[whitlockjc]

Good question. :) I'm glad to remove it until others need something similar. My only worry is removing it means people will probably write their own stuff not knowing that we have a common need for a busy loop. Your call.

[Fishrock123]

Yes, if you could move it to a function in the bottom of the test file that would be better for now.

[Fishrock123]

Edit, maybe don't pending #2232

[whitlockjc]

Just let me know.

[Fishrock123]

yeah this should go into the test itself imo

[whitlockjc]

If this PR is still up for consideration, I'll update this. I'm not sure where it stands so I defer to your judgement.

[Fishrock123]

Seems like there is an actual bug here, so yeas, please update it. :)

[whitlockjc]

Will do. Thanks for your help.

[mscdex]

/cc @bnoordhuis@trevnorris ?

[Fishrock123]

Does #2232 need to land first, or does this incorporate that?

Can we just port the test maybe? We should probably preserve julien's commit though.

[whitlockjc]

@misterdjules should comment. Last I talked to Julien I was asked to port this over but if there is already work to port things and this is superfluous, no big deal to me. Just let me know how I can help.

[misterdjules]

I'd suggest adding a comment that this assert is done within a nextTick callback so that it runs after A completed, but before the next turn of libuv's event loop. Otherwise it's not clear for anyone but who wrote the code why it's done this way.

[misterdjules]

This PR fixes one issue with #2232 by basically refactoring it, so most of the changes in #2232 are superseded by this PR. However, we need test/parallel/test-timers-blocking-callback.js and another one line change from #2232.

[whitlockjc]

I have made the changes @misterdjules requested and the PR is updated with those changes. Let me know if there is more to be done and thanks for your help.

[misterdjules]

@whitlockjc

However, we need test/parallel/test-timers-blocking-callback.js and another one line change from #2232.

I don't see these changes in this PR, do you have some time to update this PR with these changes? Then we'll close github.com//pull/2232 in favor of this one.

[whitlockjc]

Sure thing. I am mobile right now but when I get back to the laptop, I will update this PR with those changes and resolve conflicts.

[whitlockjc]

I'll take care of this today. I didn't manage to get laptop time over the holiday.

[Fishrock123]

I'm not sure this is actually a bug. Shouldn't the timer be scheduled in setImmediate() if you want to guarantee this?

Also, how does this not only apply to 0ms timeout timers?

setTimeout(function foo(){setTimeout(function bar(){}, 500) }, 500);

In this example, the 2nd timeout should be caught by https://github.com/nodejs/node/blob/master/lib/timers.js#L68

[misterdjules]

I'm not sure this is actually a bug.

It is indeed a grey area. However, this is how node v4.x and later behaves now. It is also how node v0.10.x and v0.12.x used to behave before nodejs/node-v0.x-archive@d38e865, which is a regression that is fixed by nodejs/node-v0.x-archive#25763.

Shouldn't the timer be scheduled in setImmediate() if you want to guarantee this?

Scheduling the timer with setImmediate would basically make it not a timer anymore, as immediates are scheduled and ran in a completely different way.

Also, how does this not only apply to 0ms timeout timers?

setTimeout(function foo(){setTimeout(function bar(){}, 500) }, 500);

The comment in this PR should be clearer and mention that it happens with non-0 delay timers if the callback of the outer timer blocks the JS thread for a duration that is greater or equal to the timer delay. So the snippet should be:

setTimeout(function foo(){setTimeout(function bar(){}, 500); codeThatBlocksFor500ms()}, 500); 

In this example, the 2nd timeout should be caught by https://github.com/nodejs/node/blob/master/lib/timers.js#L68

Except that then we'd hit the bug fixed by nodejs/node-v0.x-archive@d38e865.

That bug does not have a big impact for timers with short delays, but it can be a problem for timers with large delays that are scheduled before code that blocks for a long time, as it can make the timer fire with a delay of originalDelay + timeSpentBlockingInOutterTimer.

[Fishrock123]

Sorry, after figuring out how timers work fully for #4007 I'm not convinced this is the correct behavior, or necessary?

[whitlockjc]

Have we ran test-timers-blocking-callback.js and test-timers-nested.js as added/changed by this PR and #2232 to see if these tests run successfully and non-flaky? As for deciding whether this is necessary or not, I defer to @misterdjules as these issues were originally reported by him and initial fixes were by him as well. I know that when the work was done, there was definitely an issue with timers and this PR reproduced the issue with its tests and fixed the issue with its changes. In the meantime, I'll go look at #4007 to get acquainted with the new approach.

[Fishrock123]

I think the included test isn't actually valid, so it doesn't really matter if it is flaky or not. :s

[whitlockjc]

It seemed to work with the original sources. ;) I guess that doesn't matter now.

[Fishrock123]

@whitlockjc I think you may be misunderstanding. It may work but that doesn't mean it is correct. :/

[Fishrock123]

Is this PR just trying to make sure the callback is always Async per-se?

[whitlockjc]

Seeing as I'm not always right and I love to learn, I'd love to hear more about why it's not correct. At the time the issue was reported, the test was capable of reproducing the reported issue and when the code to fix the issue was in place, the test passed showing the issue had been addressed. Instead of just saying it's not right, why not provide a little context? Worst case someone learns something.

[Fishrock123]

Seeing as I'm not always right and I love to learn, I'd love to hear more about why it's not correct.

Sorry, I put some reasoning into #3063 (comment) -- basically I'm not sure we guarantee a callback will actually be called at least next tick, and timers by nature timeout as soon as possible.

[Fishrock123]

I suppose users may expect it to be async. Would it also be possible to do this by always calling insert() within a setimmediate() in active()? That may be cleaner if it works?

[misterdjules]

I suppose users may expect it to be async

It is at least how it behaves now. Having nested timers that expire when their outer timer's callback is done fire asynchronously has the nice side effect of not starving the event loop too.

Would it also be possible to do this by always calling insert() within a setimmediate() in active()? That may be cleaner if it works?

I don't think we should use setImmediate for rescheduling timers for the reason I mentioned in #3063 (comment):

Scheduling the timer with setImmediate would basically make it not a timer anymore, as immediates are scheduled and ran in a completely different way.

Using active with a different timer delay (the remaining time before the timer fires) would create a new underlying timer, and it seems doing that in a general fashion would make the number of underlying timers grow to a large number, for no added benefit except readability. Instead, the code might be made clearer by just factoring it out into separate functions.

[Fishrock123]

I don't really understand how this won't be hit when there are just timers scheduled for a later date?

[misterdjules]

first._idleStart is the time at which a timer was scheduled. Since now is updated to be the time at the start of listOnTimeout execution, now is always > first._idleStart, unlessfirst was scheduled from within the call to listOnTimeout.

[Fishrock123]

Oh duh, this is _idleStart not _idleTimeout, I'm sorry.

[bnoordhuis]

For the record: #7866 is caused by this PR although it it may not be a regression in a strict sense.

[whitlockjc]

I'll take a peek.

[misterdjules]

@bnoordhuis Thanks for the heads up, I responded in #7866.

[MylesBorins]

Not 100% if this should land in v4.x. Thoughts @misterdjules / @whitlockjc / @bnoordhuis

It seems like there is a non zero chance this could break production code, even if the production code itself is wrong.

As v6 is nearing LTS and this exists in there I would opt to simply pass on this for v4 unless one of you says otherwise.

Adding dont-land, please feel free to change

[Fishrock123]

@thealphanerd This should be backported along with any fixes.

I'm not sure how it could break, but the new behavior is definitely the expected one. (And inferred by docs too)

[Fishrock123]

Here's another issue about this problem: #8354 (comment)

@thealphanerd lmk if you need any help backporting

[MylesBorins]

@nodejs/lts how does everyone feel about backporting this given the above information

@Fishrock123 is this commit dependent on your other timers changes?

[Fishrock123]

I don't think so?

[MylesBorins]

@Fishrock123 will need help with the backport if you can