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

Ensure actor cancel callbacks are cleaned up #13074

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

temas
Copy link
Contributor

@temas temas commented Feb 7, 2024

While testing a long running web application that we uncovered a slow building memory leak related to cancel callbacks. The application did very little tile work, but did very high rate source data updates on multiple layers.

The primary issue found was that priority 0 tasks would store a cancel callback that would never be cleared from the actor::cancelCallbacks array. In tracing the code it seemed that priority 0 could never be truly canceled, so we opted to remove the callback return. We still saw a few issues after this change so we also moved the cancel callback delete to the start of the actor::processTask function. The function appeared no longer cancellable at this point and this created a safe cleanup.

With these minor changes, the application has been run for many hours, across multiple test machines, with no memory increase.

coauthor: @nsatter

Possibly helps for #12400

@temas temas requested a review from a team as a code owner February 7, 2024 21:30
Copy link
Contributor

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

Hi @temas and @nsatter,

Good catch, thanks for making a PR for this! It looks good to me, but it's worth @mourner to also take a look.

@CLAassistant
Copy link

CLAassistant commented Feb 8, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Looks good to me, just waiting on the CLA agreement to merge this. Thanks for the contribution!

src/util/actor.js Show resolved Hide resolved
@temas
Copy link
Contributor Author

temas commented Feb 12, 2024

Bump for you all, this should be 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.

4 participants