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(subscriber): fix memory leak from historical PollOps #311

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Mar 14, 2022

Motivation

Currently, console-subscriber retains all PollOps that have been
recorded since the application started, and sends all of them in the
first resource update for a new subscription.

I don't think this is correct, for a few reasons. It's necessary to send
all currently live tasks and resources to new subscriptions, because
future events may reference those tasks or resources. This isn't the
case for PollOps --- unlike tasks, resources, and even async ops, a
PollOp doesn't represent an object, it represents an event, a
single time an object was polled. There's no reason to send all previous
poll ops in the first update for a new subscription.

Storing all the poll ops that have ever occurred results in a memory
leak (see #256); unlike other tracked entities, we never clear anything
out of the vector of all poll ops that have ever occurred in the
program.

Solution

This branch just removes the all_poll_ops Vec entirely, and changes
the subscriber to only send new poll ops in the first update.

We could change this so that poll ops are associated with a timestamp,
and older ones are cleared out of a vec of all poll ops when they time
out. However, the tokio-console CLI doesn't currently have a way of
showing historical poll ops anyway, so there's no reason to keep this
data. If that changes, we can put them back, but for now, the simplest
solution is to just remove this.

Fixes #256

## Motivation

Currently, `console-subscriber` retains all `PollOp`s that have been
recorded since the application started, and sends all of them in the
first resource update for a new subscription.

I don't think this is correct, for a few reasons. It's necessary to send
all currently live tasks and resources to new subscriptions, because
future events may reference those tasks or resources. This isn't the
case for `PollOp`s --- unlike tasks, resources, and even async ops, a
`PollOp` doesn't represent an *object*, it represents an *event*, a
single time an object was polled. There's no reason to send all previous
poll ops in the first update for a new subscription.

Storing all the poll ops that have ever occurred results in a memory
leak (see #256); unlike other tracked entities, we never clear anything
out of the vector of all poll ops that have ever occurred in the
program.

## Solution

This branch just removes the `all_poll_ops` `Vec` entirely, and changes
the subscriber to only send new poll ops in the first update.

We *could* change this so that poll ops are associated with a timestamp,
and older ones are cleared out of a vec of all poll ops when they time
out. However, the `tokio-console` CLI doesn't currently have a way of
showing historical poll ops _anyway_, so there's no reason to keep this
data. If that changes, we can put them back, but for now, the simplest
solution is to just remove this.

Fixes #256
@hawkw hawkw added S-bug Severity: bug C-subscriber Crate: console-subscriber. labels Mar 14, 2022
@hawkw hawkw requested a review from zaharidichev March 14, 2022 16:41
@hawkw hawkw requested a review from a team as a code owner March 14, 2022 16:41
@hawkw
Copy link
Member Author

hawkw commented Mar 14, 2022

cc @zaharidichev

@hawkw hawkw self-assigned this Mar 14, 2022
Copy link
Collaborator

@zaharidichev zaharidichev left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry I could not get to that in time.

@hawkw hawkw merged commit 9178ecf into main Mar 14, 2022
@hawkw hawkw deleted the eliza/drop-async-ops branch March 14, 2022 21:59
hawkw added a commit that referenced this pull request Apr 11, 2022
<a name="0.1.4"></a>
## 0.1.4  (2022-04-11)

#### Bug Fixes

*  fix memory leak from historical `PollOp`s (#311)
   ([9178ecf](9178ecf), closes [#256](256))

#### Features

* **console-api:**  Update `tonic` to `0.7` (#318) ([83d8a87](83d8a87))
*  don't trace tasks spawned through the console server (#314)
   ([0045e9b](0045e9b))
grahamking pushed a commit to grahamking/console that referenced this pull request Dec 14, 2023
Do not record poll_ops if there are no current connected clients
(watchers). Without this `Aggregator::poll_ops` would grow forever.

Follow up to tokio-rs#311 and
fix for these two:
- tokio-rs#184
- tokio-rs#500
hawkw pushed a commit that referenced this pull request Jan 22, 2024
Do not record poll_ops if there are no current connected clients
(watchers). Without this `Aggregator::poll_ops` would grow forever.

Follow up to #311 and
fix for these two:
- #184
- #500

Fixes #184 

Co-authored-by: Graham King <grahamk@nvidia.com>
Co-authored-by: Hayden Stainsby <hds@caffeineconcepts.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-subscriber Crate: console-subscriber. S-bug Severity: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subscriber: poll ops appear to basically be a memory leak
2 participants