-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Conversation
I'll keep this in draft until the Tokio PR is merged, as this feature is useless without it. |
991b06e
to
92894ad
Compare
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.
92894ad
to
d5af179
Compare
The dependent change in Tokio tokio-rs/tokio#6881 has been merged, so this is good to go. |
There was a problem hiding this 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!
Co-authored-by: Eliza Weisman <[email protected]>
There was a problem hiding this 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...
It was causing a stack overflow when run on tokio 1.40.0.
@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. |
There was a problem hiding this 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!
…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))
…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>
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.