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

Fix memory leak in task manager task runner #193612

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Sep 20, 2024

In this PR, I'm fixing a memory leak that was introduced in #190093 where every task runner class object wouldn't free up in memory because it subscribed to the pollIntervalConfiguration$ observable. To fix this, I moved the observable up a class into TaskPollingLifecycle which only gets created once on plugin start and then pass down the pollInterval value via a function call the task runner class can call.

@mikecote mikecote added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Sep 20, 2024
@mikecote mikecote marked this pull request as ready for review September 20, 2024 16:14
@mikecote mikecote requested a review from a team as a code owner September 20, 2024 16:14
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@mikecote mikecote added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.16.0 Feature:Task Manager labels Sep 20, 2024
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@mikecote mikecote enabled auto-merge (squash) September 20, 2024 17:21
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mikecote mikecote merged commit cf6e8b5 into elastic:main Sep 20, 2024
45 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 20, 2024
In this PR, I'm fixing a memory leak that was introduced in
elastic#190093 where every task runner
class object wouldn't free up in memory because it subscribed to the
`pollIntervalConfiguration$` observable. To fix this, I moved the
observable up a class into `TaskPollingLifecycle` which only gets
created once on plugin start and then pass down the pollInterval value
via a function call the task runner class can call.

(cherry picked from commit cf6e8b5)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch!

kibanamachine added a commit that referenced this pull request Sep 20, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [Fix memory leak in task manager task runner
(#193612)](#193612)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Mike
Côté","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-20T17:52:26Z","message":"Fix
memory leak in task manager task runner (#193612)\n\nIn this PR, I'm
fixing a memory leak that was introduced
in\r\nhttps://github.com//pull/190093 where every task
runner\r\nclass object wouldn't free up in memory because it subscribed
to the\r\n`pollIntervalConfiguration# Backport

This will backport the following commits from `main` to `8.x`:
- [Fix memory leak in task manager task runner
(#193612)](#193612)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT observable. To fix this, I moved the\r\nobservable up a
class into `TaskPollingLifecycle` which only gets\r\ncreated once on
plugin start and then pass down the pollInterval value\r\nvia a function
call the task runner class can
call.","sha":"cf6e8b5ba971fffe2a57e1a7c573e60cc2fbe280","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Feature:Task
Manager","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"Fix
memory leak in task manager task
runner","number":193612,"url":"https://github.com/elastic/kibana/pull/193612","mergeCommit":{"message":"Fix
memory leak in task manager task runner (#193612)\n\nIn this PR, I'm
fixing a memory leak that was introduced
in\r\nhttps://github.com//pull/190093 where every task
runner\r\nclass object wouldn't free up in memory because it subscribed
to the\r\n`pollIntervalConfiguration# Backport

This will backport the following commits from `main` to `8.x`:
- [Fix memory leak in task manager task runner
(#193612)](#193612)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT observable. To fix this, I moved the\r\nobservable up a
class into `TaskPollingLifecycle` which only gets\r\ncreated once on
plugin start and then pass down the pollInterval value\r\nvia a function
call the task runner class can
call.","sha":"cf6e8b5ba971fffe2a57e1a7c573e60cc2fbe280"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193612","number":193612,"mergeCommit":{"message":"Fix
memory leak in task manager task runner (#193612)\n\nIn this PR, I'm
fixing a memory leak that was introduced
in\r\nhttps://github.com//pull/190093 where every task
runner\r\nclass object wouldn't free up in memory because it subscribed
to the\r\n`pollIntervalConfiguration# Backport

This will backport the following commits from `main` to `8.x`:
- [Fix memory leak in task manager task runner
(#193612)](#193612)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT observable. To fix this, I moved the\r\nobservable up a
class into `TaskPollingLifecycle` which only gets\r\ncreated once on
plugin start and then pass down the pollInterval value\r\nvia a function
call the task runner class can
call.","sha":"cf6e8b5ba971fffe2a57e1a7c573e60cc2fbe280"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Mike Côté <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Task Manager release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants