test: add tests for ensuring proper timer delays

[Fishrock123]

Checklist

• make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

tests, timers

Description of change

Adds some test cases for #5426 ... it's a pretty strange bug and I'm not immediately sure what is going on but it may be useful to have these in here so we don't accidentally regress while trying to fix it.

Includes a passing test that should keep passing and an issue test to
be fixed.

Note: I think the known_issues test is testing the correct thing, but verification would be helpful.

See also: #7346 & #7386

cc @nodejs/testing and also @misterdjules

[misterdjules]

As indicated by #5426 (comment) and confirmed by #5426 (comment), #5426 is fixed by #3063.

[Fishrock123]

Oh crap is this that issue again? Whoops..

[Fishrock123]

That being said, these tests are still valuable, one in the mean time, the other because that behavior is not covered yet.

[misterdjules]

I believe the tests in this PR already exist in part in #3063 and, as I mentioned previously in #3063 several times in one test added by #2232 that needs to be ported to #3063.

[Fishrock123]

@misterdjules Maybe for the known issue, yes. But the other test tests a behavior that we don't yet...

[misterdjules]

Regarding test/parallel/test-timers-later-scheduled-delay.js, it seems that its purpose is to test that timers fire after a duration that is within a margin of error of the delay that was set at the time of their creation.

However I'm not sure it is what it intends to test, since that file mentions #5426 (comment), which seems to describe another problem: the fact that code running on node's main thread offsets the time at which nested timers using the same delay fire. Is that comment still relevant?

Also, does this test need to use one timer that is scheduled from within another timer's callback? if so, why?

If the purpose of that test is indeed to test that timers fire after a duration that is within a margin of error of the delay that was set at the time of their creation, it seems that test/parallel/test-timers-later-scheduled-delay.js wouldn't provide a better coverage than the test/parallel/test-timers-blocking-callback.js test in #2232: that test already checks that the delay at which a nested timer fire is < TIMEOUT *1.5, which in the current version of this test is 50ms, a smaller value than the 200ms value used in this PR. So I don't think it would be worth adding test/parallel/test-timers-later-scheduled-delay.js in its current form.

Regardless, it seems that both tests have a similar issue: picking an appropriate "margin of error" is subjective. Node.js' documentation only guarantees that the timers will fire after the delay, but isn't more specific.

Moreover, it potentially makes the test flaky when running on a slower/heavily loaded machine. On a busy system, the node process that runs the test can be scheduled off CPU at any time and for an arbitrary duration between when the timer was scheduled and when its callback fires. It is likely that this problem wouldn't surface often, but it still indicates a design issue with both tests. Unfortunately I don't know how to solve it.

I think these two issues are OK for the test/parallel/test-timers-blocking-callback.js test in #2232 because its goal is to be a regression test for a specific known issue that was reproduced with that exact same code but with a node version that didn't have the appropriate fix, so it doesn't need to be a general test.

But if test/parallel/test-timers-later-scheduled-delay.js is to be a more general test, as it seems it intends to be, I don't think its current implementation achieves that.

For instance, if a future change adds 150ms of delay to the time when every timer actually fires, which could be a regression, it seems that test wouldn't catch it in its current form. Maybe the solution is to lower the margin of error to a value that is lower, but that we empirically determine to not make the test too flaky. It might be tricky to make that work across all platforms.

I think test/parallel/test-timers-later-scheduled-delay.js would also need to test timers scheduled in different ways: with setInterval, with setTimeout but not from a timer's callback, with setTimeout called from a timer's callback, etc.

[Fishrock123]

@misterdjules The test in question is to ensure that this patch does not appear to fix the problem, because it would create another problem that we do not yet test for.

That being said I'm suspecting there is also another problem floating around that none of these tests catch:

When large sync work is done, existing scheduled same-duration callbacks will be re-scheduled too late.

That is, this inner logic from #3063 is correct, but it should (as I have said before) be put into this conditional.

However, I am having a hard time testing that issue.

[misterdjules]

The test in question is to ensure that this patch does not appear to fix the problem, because it would create another problem that we do not yet test for.

If I understand correctly, we would add a test for some code that is not in the code base? I'm not sure I understand why that would be useful.

That being said I'm suspecting there is also another problem floating around that none of these tests catch:

When large sync work is done, existing scheduled same-duration callbacks will be re-scheduled too late.

This seems to be what #3063 fixes.

That is, this inner logic from #3063 is correct, but it should (as I have said before) be put into this conditional.

May I ask you to comment in #3063 directly? I couldn't find any previous comment from you on that part of #3063.

It seems to me that the best way to move forward would be to land #3063 after:

1. it's updated to add the test test/parallel/test-timers-blocking-callback.js from Port from joyent/node: timers: fix timeout when added in timer's callback #2232 as mentioned before.
2. @onnlucky's comments are considered and maybe integrated.

What do you think?

[Fishrock123]

If I understand correctly, we would add a test for some code that is not in the code base? I'm not sure I understand why that would be useful.

Kinda? It's to make sure we don't regress it?

[misterdjules]

If I understand correctly, we would add a test for some code that is not in the code base? I'm not sure I understand why that would be useful.

Kinda? It's to make sure we don't regress it?

OK, I got confused by the part that says:

The test in question is to ensure that this patch does not appear to fix the problem

but now I understand what you meant.

As I mentioned before, and contrary to what its comments mention, that test (test/parallel/test-timers-later-scheduled-delay.js) is not related to #5426 (comment) (I'll comment in #5426 about that).

I would suggest keeping only test/parallel/test-timers-later-scheduled-delay.js in this PR, making it more general (it is currently specific to nested timers scheduled with setTimeout), and addressing the question I mentioned in #7404 (comment): "what would an appropriate margin of error be?".

A test similar to test/known_issues/test-timers-sync-work-delay-problem.js is already present in #2232 and should be ported by #3063.

[Fishrock123]

I don't trust my tests here. Will continue to try to test this (issue no.2) at some point in the future.