Skip to content

feat: option to recycle timer context #11

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: option to recycle timer context #11

wants to merge 2 commits into from

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Dec 21, 2020

a timer context is implemented as a request under the hood. This means that it is supposed to be short-lived. As such some calls leak memory within a context, which is not a problem when the context is short-lived and freed as a whole. But if we hold on to timer contexts indefinitely, it becomes a real memory leak. Hence the new option to recycle the timer context after max_use number of invocations (by default 1000).

It is implemented in such a way that if the creation of the replacement timer fails (due to lack of timer resources), it will continue in the existing context. This makes the timer resilient to lack-of-timer-resource type errors.

@Tieske Tieske force-pushed the refactor branch 7 times, most recently from 182f04c to 0dfc4c7 Compare December 22, 2020 13:06
@Tieske Tieske requested review from bungle and dndx December 23, 2020 06:35
Copy link

@locao locao left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we should hear from @dndx before merging this.

Keeping the timer context alive can cause memory leaks. Hence
a new option to set the number of invocations to use a context
before recreating the timer in a new context.
@Tieske Tieske force-pushed the refactor branch 5 times, most recently from 7710e68 to 0b0d797 Compare August 12, 2021 12:45
@Tieske
Copy link
Member Author

Tieske commented Aug 12, 2021

@locao since we figured this one out: https://github.com/Kong/lua-resty-timer/pull/11/files#diff-62bd0b4b2f58444dda4d468eb58071b931b443ddb4be4d9797c3cd2f61ca059bR34-R38 (credits to Javier), I think this is now good to go

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.

2 participants