under-pressure icon indicating copy to clipboard operation
under-pressure copied to clipboard

replace setInterval with a setTimeout

Open davideroffo opened this issue 2 years ago • 4 comments

This PR fixes https://github.com/fastify/under-pressure/issues/164 replacing the setInterval timer with setTimeout.

Checklist

davideroffo avatar Jul 07 '22 15:07 davideroffo

At the moment the only missing part is the implementation of the unit tests for this fix. Do you have any suggestions on which test I should create? @mcollina

davideroffo avatar Jul 07 '22 15:07 davideroffo

I would just use a mock to verify this.

mcollina avatar Jul 07 '22 16:07 mcollina

I think it needs to document the usage of setTimeout. It is not truly identical to setInterval and the user need to know that the interval is not guarantee to be run in exact same rate when system is under pressure / long running process. The choice is based on we do not want to add more pressure to the system.

I think here also worth this changes.

https://github.com/fastify/under-pressure/blob/a0c3b2062cdf522c1d74acc3a79923876aa5495c/index.js#L85-L86

I addressed the requested changes @climba03003

davideroffo avatar Jul 08 '22 08:07 davideroffo

I switched this PR into a Draft PR because some analysis on the issue are still in progress.

davideroffo avatar Jul 11 '22 08:07 davideroffo

Can we merge master into this PR, let the pipeline run and then if it passes, squash merge, please?

Uzlopak avatar Aug 24 '22 10:08 Uzlopak

Can we merge master into this PR, let the pipeline run and then if it passes, squash merge, please?

What does this mean?

simoneb avatar Aug 24 '22 11:08 simoneb

I just did this in my repo to check if it really merges correct.

Uzlopak avatar Aug 24 '22 11:08 Uzlopak

LGTM

If we have these concerns, we should consider enabling the setting in repos which require PR branches to be up to date with the base branch of a PR

simoneb avatar Aug 24 '22 11:08 simoneb

I was just worried in this case, as I modified the tests few days heavily. No worries.

Uzlopak avatar Aug 24 '22 12:08 Uzlopak