Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace setInterval with a setTimeout #174

Merged

Conversation

davideroffo
Copy link
Contributor

@davideroffo davideroffo commented Jul 7, 2022

This PR fixes #164 replacing the setInterval timer with setTimeout.

Checklist

@simoneb simoneb requested a review from mcollina July 7, 2022 15:34
@davideroffo
Copy link
Contributor Author

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

@mcollina
Copy link
Member

mcollina commented Jul 7, 2022

I would just use a mock to verify this.

@mcollina mcollina marked this pull request as ready for review July 7, 2022 16:31
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

under-pressure/index.js

Lines 85 to 86 in a0c3b20

externalHealthCheckTimer = setInterval(doCheck, healthCheckInterval)
externalHealthCheckTimer.unref()

@davideroffo davideroffo requested a review from climba03003 July 8, 2022 08:49
@davideroffo
Copy link
Contributor Author

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.

under-pressure/index.js

Lines 85 to 86 in a0c3b20

externalHealthCheckTimer = setInterval(doCheck, healthCheckInterval)
externalHealthCheckTimer.unref()

I addressed the requested changes @climba03003

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@davideroffo davideroffo marked this pull request as draft July 11, 2022 08:28
@davideroffo
Copy link
Contributor Author

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

Replace the externalHealthCheckTimer with setTimeout and renamed some methods
Added the additional information section in the README file in order to explain why setTimeout has replaced setInterval
@simoneb simoneb force-pushed the feat/replace-set-interval-with-set-timeout branch from 70a6072 to d730b91 Compare August 24, 2022 09:38
@simoneb simoneb marked this pull request as ready for review August 24, 2022 09:38
@simoneb simoneb force-pushed the feat/replace-set-interval-with-set-timeout branch from d730b91 to b891000 Compare August 24, 2022 09:41
@simoneb simoneb force-pushed the feat/replace-set-interval-with-set-timeout branch from b891000 to 91590db Compare August 24, 2022 09:43
})

fastify.get('/', (req, rep) => rep.send('A'))
test('interval reentrance', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only real change in this file, a new test being added

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 24, 2022

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

@simoneb
Copy link
Contributor

simoneb commented Aug 24, 2022

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

What does this mean?

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 24, 2022

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

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simoneb
Copy link
Contributor

simoneb commented Aug 24, 2022

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

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 24, 2022

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

@mcollina mcollina merged commit 4e73a53 into fastify:master Aug 24, 2022
@simoneb simoneb deleted the feat/replace-set-interval-with-set-timeout branch August 24, 2022 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consider setTimeout instead of setInterval for polling
7 participants