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

feat(console): add large future lints #587

Merged
merged 6 commits into from
Oct 9, 2024
Merged

feat(console): add large future lints #587

merged 6 commits into from
Oct 9, 2024

Conversation

hds
Copy link
Collaborator

@hds hds commented Oct 1, 2024

In Tokio, the futures for tasks are stored on the stack unless they are
explicitly boxed. Having very large futures can be problematic as it can
cause a stack overflow.

This change makes use of new instrumentation in Tokio
(tokio-rs/tokio#6881) which includes the size of the future which drives
a task. The size isn't given its own column (we're running out of
horizontal space) and appears in the additional fields column. In the
case that a future was auto-boxed by Tokio, the original size of the
task will also be provided.

Two new lints have been added for large futures. The first will detect
auto-boxed futures and warn about them. The second will detect futures
which are large (over 1024 bytes by default), but have not been
auto-boxed by the runtime.

Since the new lints depend on the new instrumentation in Tokio, they
will only trigger if the instrumented application is running using
tokio 1.41.0 or later. The version is as yet, unreleased.

Both lints have been added to the default list.

PR Notes

Here's a screenshot with the new lints triggered on tasks from the app example.

large_future_lints

@hds
Copy link
Collaborator Author

hds commented Oct 1, 2024

I'll keep this in draft until the Tokio PR is merged, as this feature is useless without it.

@hds hds force-pushed the hds/large-future-lint branch 3 times, most recently from 991b06e to 92894ad Compare October 8, 2024 10:52
In Tokio, the futures for tasks are stored on the stack unless they are
explicitly boxed. Having very large futures can be problematic as it can
cause a stack overflow.

This change makes use of new instrumentation in Tokio
(tokio-rs/tokio#6881) which includes the size of the future which drives
a task. The size isn't given its own column (we're running out of
horizontal space) and appears in the additional fields column. In the
case that a future was auto-boxed by Tokio, the original size of the
task will also be provided.

Two new lints have been added for large futures. The first will detect
auto-boxed futures and warn about them. The second will detect futures
which are large (over 1024 bytes by default), but have not been
auto-boxed by the runtime.

Since the new lints depend on the new instrumentation in Tokio, they
will only trigger if the instrumented application is running using
`tokio` 1.41.0 or later. The version is as yet, unreleased.

Both lints have been added to the default list.
@hds hds force-pushed the hds/large-future-lint branch from 92894ad to d5af179 Compare October 8, 2024 11:50
@hds hds changed the title feat(console): add large future lint feat(console): add large future lints Oct 8, 2024
@hds hds marked this pull request as ready for review October 8, 2024 12:01
@hds hds requested a review from a team as a code owner October 8, 2024 12:01
@hds
Copy link
Collaborator Author

hds commented Oct 8, 2024

The dependent change in Tokio tokio-rs/tokio#6881 has been merged, so this is good to go.

@hds hds requested review from Rustin170506 and removed request for a team October 8, 2024 12:03
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This is really cool --- I love seeing new lints I hadn't thought of being added to the console!

I had some very small suggestions about the wording of the lint text, and noticed some stuff was missing from the docs. Other than that, the implementation looks great!

tokio-console/src/warnings.rs Outdated Show resolved Hide resolved
tokio-console/src/warnings.rs Outdated Show resolved Hide resolved
tokio-console/src/config.rs Show resolved Hide resolved
tokio-console/src/config.rs Show resolved Hide resolved
tokio-console/src/config.rs Outdated Show resolved Hide resolved
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Oh, one last thing: since this change depends on new Tokio instrumentation, can we add something stating which version of Tokio these lints will be available in to the list of Tokio versions here: https://github.com/tokio-rs/console/tree/main/console-subscriber#required-tokio-versions

Also, I assume you've tested that the console still works fine with prior tokio versions that are missing the task-size fields? It seems like it ought to, but it's probably worth making sure...

@hds
Copy link
Collaborator Author

hds commented Oct 8, 2024

@hawkw Good point about the Tokio version! I had tested an earlier version against the current version of Tokio, but it was good that I tested it again, after modifying the large blocking task I had accidentally made it so large that in the Tokio 1.40.0 (and lower) it was causing a stack overflow (just in the example application, but all the same) because that blocking task wasn't being auto-boxed.

Fixed that now and confirmed that everything works fine in the current version of Tokio, just that there is no size information and so the lints won't ever trigger.

Copy link
Member

@hawkw hawkw 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 whenever CI is satisfied!

@hds hds merged commit ae17230 into main Oct 9, 2024
17 checks passed
@hds hds deleted the hds/large-future-lint branch October 9, 2024 12:18
hds pushed a commit that referenced this pull request Oct 24, 2024
…onsole-v0.1.13

## 🤖 New release
* `tokio-console`: 0.1.12 -> 0.1.13 (✓ API compatible changes)
* `console-api`: 0.8.0 -> 0.8.1 (✓ API compatible changes)
* `console-subscriber`: 0.4.0 -> 0.4.1 (✓ API compatible changes)

## Changelog

## `tokio-console`

## 0.1.13 - (2024-10-24)

### Added

- Add large future lints ([#587](#587)) ([ae17230](ae17230))

### Fixed

- Correct the grammar issue ([#579](#579)) ([f8e1bee](f8e1bee))

## `console-api`

## 0.8.1 - (2024-10-24)

_No outward facing changes_

## `console-subscriber`

## 0.4.1 - (2024-10-24)

### Added

- Add large future lints ([#587](#587)) ([ae17230](ae17230))
hds pushed a commit that referenced this pull request Oct 24, 2024
…onsole-v0.1.13 (#581)

## 🤖 New release
* `tokio-console`: 0.1.12 -> 0.1.13 (✓ API compatible changes)
* `console-api`: 0.8.0 -> 0.8.1 (✓ API compatible changes)
* `console-subscriber`: 0.4.0 -> 0.4.1 (✓ API compatible changes)

## Changelog

## `tokio-console`

## 0.1.13 - (2024-10-24)

### Added

- Add large future lints ([#587](#587)) ([ae17230](ae17230))

### Fixed

- Correct the grammar issue ([#579](#579)) ([f8e1bee](f8e1bee))

## `console-api`

## 0.8.1 - (2024-10-24)

_No outward facing changes_

## `console-subscriber`

## 0.4.1 - (2024-10-24)

### Added

- Add large future lints ([#587](#587)) ([ae17230](ae17230))

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Nov 4, 2024
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