Unref the interval to allow server to exit

[bttger]

If not unref'ed the event loop would always keep a single item in the loop and not exit

[TimDaub]

thanks for your contribution. Please add a test

[bttger]

To be honest, I am not sure how to test this case. Locally, I test this via pressing Ctrl+C to exit the server. Do you have an idea?

[bttger]

I think, what we need to test is, if the interval still continues to work as expected (debugging this with a simple log output says yes), and the two cases that the server keeps running when unrefInterval = false and after the user interrupts it with a SIGINT/SIGTERM and vice versa.

[TimDaub]

To be honest, I am not sure how to test this case. Locally, I test this via pressing Ctrl+C to exit the server. Do you have an idea?

Can we read the items in the event loop? Because then we can just test if the conditions were met to see if unref was called or not

[bttger]

It seems like there is no simple way of checking what is left in the event queue. But I think it would be possible to observe all asynchronous calls and check for the Timeout type in the init and destroy async-hooks. I'll look into it when I have more time.

[TimDaub]

ok sounds good thank you!

[bttger]

I have added the tests with async_hooks.

[TimDaub]

I'm currently traveling until after Oct 18 and this is something I have to take a little while to understand so I hope you don't mind if this will take a week or two to get reviewed. Sorry!

[TimDaub]

I don't understand why we're trying to clear the interval with unref, I've not seen that anywhere. I'm only familiar with clearInterval and I've checked the nodejs docs and it seems to be the right usage: https://nodejs.org/docs/latest-v14.x/api/timers.html#timers_clearinterval_timeout

[TimDaub]

we'd have to add any new option property to the documentation. And why do we need "|| false" ?

[bttger]

Yes, I will add it to the docs if you accept it to be added. Regarding the "false", that's redundant of course and I'll remove it.

[TimDaub]

If not unref'ed the event loop would always keep a single item in the loop and not exit

I'm not sure I understand the use case. In an application like a webserver, I can't think of a moment when the interval should truly be cleared. E.g. a webserver runs indefinitely and so the interval should technically only be cleared when the server shuts down, in which case that takes care of itself, or not?

[bttger]

I don't understand why we're trying to clear the interval with unref, I've not seen that anywhere. I'm only familiar with clearInterval and I've checked the nodejs docs and it seems to be the right usage: https://nodejs.org/docs/latest-v14.x/api/timers.html#timers_clearinterval_timeout

No I don't want to clear the interval because then it wouldn't remove stale sessions anymore. I only want to ensure that the server can gracefully terminate. E.g. when I start a fastify server and then press Ctrl+C, it stops the server but the background interval to clean the sessions keeps it from terminating the process. The following snippet shows a common way to implement graceful shutdowns:

const SHUTDOWN_TIMEOUT = 10 * 1000; // default 10 secs
const signals = ["SIGINT", "SIGTERM"];
signals.forEach((signal) => {
  process.once(signal, () => {
    // Make sure server terminates after timeout
    setTimeout(() => {
      process.exit(1);
    }, SHUTDOWN_TIMEOUT).unref();

    // Do stuff to clean up
    server.close();
  });
});

Here we also unref the timeout that ensures that it will definitely terminate. If the event loop empties first, then the server can terminate before.

[awidener3]

Any chance of this being merged soon? I see it's been stuck with 1 requested change for a while.

These changes resolved an issue where my mocha tests would not properly exit once the tests have completed due to the lingering open handles from the session store.

[TimDaub]

sorry I should probably mark the repo as unmaintained. feel free to fork and maintain yourself with this PR!