consider setTimeout instead of setInterval for polling

[cmawhorter]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched existing issues to ensure the issue has not already been raised

Issue

setInterval will repeatedly call at the scheduled time whether or not the previous call has finished. if the server is already under load, this will likely add to the problem, because those setInterval calls will start to stack. i imagine this would be especially bad in node v8 with setInterval(..., 5ms).

for node >= v10.2.0, using setTimeout and timer.refresh would avoid this issue. i swapped it in and tests pass in node v14, at least:

  const timer = setTimeout(() => {
    updateMemoryUsage()
    timer.refresh()
  }, sampleInterval)

[mcollina]

Good spot indeed! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[davideroffo]

While working on fixing this problem, I noticed that the behaviour @cmawhorter mentioned does not occur with setInterval. If you check this GIST: https://gist.github.com/davideroffo/b4e2adcc7ebb9e1fb431acb318a7aa45 you will notice that even if the event-loop is blocked manually, the function called by setInterval does not accumulate in the queue.

Could you provide a reproducible test for this @cmawhorter?

The only thing I noticed and that can be improved using setTimeout is the custom health check, where we can address this issue: https://developer.mozilla.org/en-US/docs/Web/API/setInterval#ensure_that_execution_duration_is_shorter_than_interval_frequency if healthCheck provided the user is taking longer than the programmed time interval.

[simoneb]

@cmawhorter can you provide a repro? We couldn't reproduce the issue, so we assume the issue doesn't actually happen.

[cmawhorter]

the most important thing here is probably the async healthcheck option. the built-in memory check might be a problem on node v8 with underpowered hardware, but that'd depend on how many ms eventLoopUtilization().utilization takes to calculate. i bet it'd rarely take > 5ms, so probably an edge case for that one.

healthcheck is the primary concern here unless the user is careful. the example in the readme will fall down if checking the db connectivity takes longer than 500ms e.g. it times out. AFAIK return order is essentially random when setInterval calls stack like that, and the healthcheck could show the db is up when it's not or visa versa.

personally, i treat setInterval like eval as a no-go except for very limited situations.

[mcollina]

@cmawhorter can you provide a test case? So that we know this is fixed and we don't regress.

[simoneb]

@cmawhorter as we explained earlier, setInterval doesn't behave the way you imply it behaves.

Try out this snippet of code.

setInterval(() => { 
    console.log('interval triggered on', new Date())
    
    const now = new Date(); 
    now.setSeconds(now.getSeconds() + 2);

    console.time('sleeping duration') 
    
    while(new Date() - now) {} 

    console.timeEnd('sleeping duration')     
}, 1000)

You will see that despite the interval being scheduled to run every 1 second, the body of the callback function blocks for 2 seconds.

What you suggest is that there are overlapping executions of the callback, whereas there aren't. The callback function is executed approximately every 2 seconds instead

[mcollina]

@simoneb I think you need to add some asynchronous action inside the setInterval(). If a db healtcheck takes more than the interval, we will fire it twice.

[climba03003]

@simoneb The below one should illustrate the problem.

import { setTimeout } from 'timers/promises'


setInterval(async () => { 
  const now = Date.now()
  console.log('interval triggered on', new Date(now))

  console.time('sleeping duration ' + now) 
  
  await setTimeout(Math.random() * 2000)

  console.timeEnd('sleeping duration ' + now)
}, 1000)

[simoneb]

It would be a lot easier if the OP included a repro so we don't need to guess. Anyway, the PR is ready now. Testing this is particularly cumbersome

[mcollina]

Indeed on all accounts @simoneb!

[cmawhorter]

apologies for the lack of repro. i ended up going a different route than the plugin and mainly posted the issue as a psa

[Uzlopak]

@cmawhorter

Well better than not reporting. ;)

btw. Its strage. I remember using your project named FLIR about a decade ago. I DM'ed you on twitter.

[cmawhorter]

@Uzlopak lol. holy crap... that's a lifetime ago. good to hear from you again man!