Unref timers

[novemberborn]

I'm using Cockatiel to renew cryptographic keys from service A, in the background of service B. Service B also runs a server.

The retry and timeout policies (and, presumably, the circuit breaker, but I can't immediately see how this works) create timer objects. These keep the Node.js process alive.

Since I'm renewing keys in the background I'd rather the timers be unreferenced. This way, my process can shut down gracefully when the server is closed.

Currently if a renewal is stuck in a retry delay (or presumably a circuit breaker delay) my process won't shut down.

We could address this by adding an unref option in the policy creation arguments. It should default to false, which would be the current behavior, but if set to true it would ensure all timers are unreferenced.

Let me know if this is something you'd like and I can submit a PR.

[connor4312]

I think unrefing the timeout timer by default would be a good idea. If there's wor going on in the timeout's execute that's keeping the process alive, we may as well shut down. That's here:

cockatiel/src/TimeoutPolicy.ts

Line 41 in ee47684

const timer = setTimeout(() => cts.cancel(), this.duration);

I don't think doing the same on the retry timer is good; if that was the case, a process could shut down instead of retrying on failure if the timer wasn't keeping the process alive.

PR's would be welcome.

[novemberborn]

I think unrefing the timeout timer by default would be a good idea. If there's wor going on in the timeout's execute that's keeping the process alive, we may as well shut down.

The timer is cleared once the execution completes. Presumably the execution itself will keep the process alive. To clarify, are you suggesting that just in case the execution doesn't, but it does not complete either, you don't want the timer to keep the process alive?

I don't think doing the same on the retry timer is good; if that was the case, a process could shut down instead of retrying on failure if the timer wasn't keeping the process alive.

Agreed.

I just realized that the circuit breaker causes errors to be thrown while the circuit is open, so it doesn't have any timers.

[connor4312]

Ah, didn't notice that. Apologies, that's what I get for going through GH notifications before I have my 🍵

Indeed, the circuit breaker looks at the current time but doesn't have any timers--the test opening is done on the next call after the specified interval has past, not in the background.

Looks like timeouts and retries are the only places we use setTimeout aside from tests. Were there other places in here you were looking at which needed timers to be unrefed?

[novemberborn]

Indeed, the circuit breaker looks at the current time but doesn't have any timers--the test opening is done on the next call after the specified interval has past, not in the background.

Yea still figuring out how to compose all this stuff.

Looks like timeouts and retries are the only places we use setTimeout aside from tests. Were there other places in here you were looking at which needed timers to be unrefed?

No, those are the only places I could find as well.

[connor4312]

Okay. Let me know there's actions to take on this issue, feel free to close it otherwise 🙂

Yea still figuring out how to compose all this stuff.

I should get some diagrams in the docs to help with that. I've been sort of directing people to the Polly wiki for more in-depth info, but that's an extra hop that not everyone will make/notice.

[novemberborn]

I'll try and get some PRs done on Monday.