Implement asyncio-dangling-task to track asyncio.create_task calls (#2935)

This rule guards against `asyncio.create_task` usages of the form:

```py
asyncio.create_task(coordinator.ws_connect())  # Error
```

...which can lead to unexpected bugs due to the lack of a strong reference to the created task. See Will McGugan's blog post for reference: https://textual.textualize.io/blog/2023/02/11/the-heisenbug-lurking-in-your-async-code/.

Note that we can't detect issues like:

```py
def f():
    # Stored as `task`, but never used...
    task = asyncio.create_task(coordinator.ws_connect())
```

So that would be a false negative. But this catches the common case of failing to assign the task in any way.

Closes #2809.
This commit is contained in:
Charlie Marsh 2023-02-15 15:19:03 -05:00 committed by GitHub
parent 294cd95c54
commit f8d46d09ef
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 159 additions and 0 deletions

View file

@ -1503,6 +1503,7 @@ For more, see [tryceratops](https://pypi.org/project/tryceratops/1.1.0/) on PyPI
| RUF003 | ambiguous-unicode-character-comment | Comment contains ambiguous unicode character `{confusable}` (did you mean `{representant}`?) | 🛠 |
| RUF004 | keyword-argument-before-star-argument | Keyword argument `{name}` must come after starred arguments | |
| RUF005 | unpack-instead-of-concatenating-to-collection-literal | Consider `{expr}` instead of concatenation | 🛠 |
| RUF006 | [asyncio-dangling-task](https://beta.ruff.rs/docs/rules/asyncio-dangling-task/) | Store a reference to the return value of `asyncio.create_task` | |
| RUF100 | unused-noqa | Unused `noqa` directive | 🛠 |
<!-- End auto-generated sections. -->

View file

@ -0,0 +1,52 @@
import asyncio
# Error
def f():
asyncio.create_task(coordinator.ws_connect()) # Error
# OK
def f():
background_tasks = set()
for i in range(10):
task = asyncio.create_task(some_coro(param=i))
# Add task to the set. This creates a strong reference.
background_tasks.add(task)
# To prevent keeping references to finished tasks forever,
# make each task remove its own reference from the set after
# completion:
task.add_done_callback(background_tasks.discard)
# OK
def f():
ctx.task = asyncio.create_task(make_request())
# OK
def f():
tasks.append(asyncio.create_task(self._populate_collection(coll, coll_info)))
# OK
def f():
asyncio.wait([asyncio.create_task(client.close()) for client in clients.values()])
# OK
def f():
tasks = [asyncio.create_task(task) for task in tasks]
# Ok (false negative)
def f():
task = asyncio.create_task(coordinator.ws_connect())
# Ok (potential false negative)
def f():
do_nothing_with_the_task(asyncio.create_task(coordinator.ws_connect()))

View file

@ -1795,6 +1795,13 @@ where
{
flake8_simplify::rules::use_capital_environment_variables(self, value);
}
if self.settings.rules.enabled(&Rule::AsyncioDanglingTask) {
if let Some(diagnostic) = ruff::rules::asyncio_dangling_task(value, |expr| {
self.resolve_call_path(expr)
}) {
self.diagnostics.push(diagnostic);
}
}
}
_ => {}
}

View file

@ -596,6 +596,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Ruff, "003") => Rule::AmbiguousUnicodeCharacterComment,
(Ruff, "004") => Rule::KeywordArgumentBeforeStarArgument,
(Ruff, "005") => Rule::UnpackInsteadOfConcatenatingToCollectionLiteral,
(Ruff, "006") => Rule::AsyncioDanglingTask,
(Ruff, "100") => Rule::UnusedNOQA,
// flake8-django

View file

@ -558,6 +558,7 @@ ruff_macros::register_rules!(
rules::ruff::rules::AmbiguousUnicodeCharacterComment,
rules::ruff::rules::KeywordArgumentBeforeStarArgument,
rules::ruff::rules::UnpackInsteadOfConcatenatingToCollectionLiteral,
rules::ruff::rules::AsyncioDanglingTask,
rules::ruff::rules::UnusedNOQA,
// flake8-django
rules::flake8_django::rules::NullableModelStringField,

View file

@ -18,6 +18,7 @@ mod tests {
#[test_case(Rule::KeywordArgumentBeforeStarArgument, Path::new("RUF004.py"); "RUF004")]
#[test_case(Rule::UnpackInsteadOfConcatenatingToCollectionLiteral, Path::new("RUF005.py"); "RUF005")]
#[test_case(Rule::AsyncioDanglingTask, Path::new("RUF006.py"); "RUF006")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(

View file

@ -0,0 +1,78 @@
use rustpython_parser::ast::{Expr, ExprKind};
use ruff_macros::{define_violation, derive_message_formats};
use crate::ast::types::{CallPath, Range};
use crate::registry::Diagnostic;
use crate::violation::Violation;
define_violation!(
/// ## What it does
/// Checks for `asyncio.create_task` calls that do not store a reference
/// to the returned result.
///
/// ## Why is this bad?
/// Per the `asyncio` documentation, the event loop only retains a weak
/// reference to tasks. If the task returned by `asyncio.create_task` is
/// not stored in a variable, or a collection, or otherwise referenced, it
/// may be garbage collected at any time. This can lead to unexpected and
/// inconsistent behavior, as your tasks may or may not run to completion.
///
/// ## Example
/// ```python
/// import asyncio
///
/// for i in range(10):
/// # This creates a weak reference to the task, which may be garbage
/// # collected at any time.
/// asyncio.create_task(some_coro(param=i))
/// ```
///
/// Use instead:
/// ```python
/// import asyncio
///
/// background_tasks = set()
///
/// for i in range(10):
/// task = asyncio.create_task(some_coro(param=i))
///
/// # Add task to the set. This creates a strong reference.
/// background_tasks.add(task)
///
/// # To prevent keeping references to finished tasks forever,
/// # make each task remove its own reference from the set after
/// # completion:
/// task.add_done_callback(background_tasks.discard)
/// ```
///
/// ## References
/// * [_The Heisenbug lurking in your async code_](https://textual.textualize.io/blog/2023/02/11/the-heisenbug-lurking-in-your-async-code/)
/// * [`asyncio.create_task`](https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task)
pub struct AsyncioDanglingTask;
);
impl Violation for AsyncioDanglingTask {
#[derive_message_formats]
fn message(&self) -> String {
format!("Store a reference to the return value of `asyncio.create_task`")
}
}
/// RUF006
pub fn asyncio_dangling_task<'a, F>(expr: &'a Expr, resolve_call_path: F) -> Option<Diagnostic>
where
F: FnOnce(&'a Expr) -> Option<CallPath<'a>>,
{
if let ExprKind::Call { func, .. } = &expr.node {
if resolve_call_path(func).map_or(false, |call_path| {
call_path.as_slice() == ["asyncio", "create_task"]
}) {
return Some(Diagnostic::new(
AsyncioDanglingTask,
Range::from_located(expr),
));
}
}
None
}

View file

@ -1,4 +1,5 @@
mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod keyword_argument_before_star_argument;
mod unpack_instead_of_concatenating_to_collection_literal;
mod unused_noqa;
@ -7,6 +8,7 @@ pub use ambiguous_unicode_character::{
ambiguous_unicode_character, AmbiguousUnicodeCharacterComment,
AmbiguousUnicodeCharacterDocstring, AmbiguousUnicodeCharacterString,
};
pub use asyncio_dangling_task::{asyncio_dangling_task, AsyncioDanglingTask};
pub use keyword_argument_before_star_argument::{
keyword_argument_before_star_argument, KeywordArgumentBeforeStarArgument,
};

View file

@ -0,0 +1,15 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
expression: diagnostics
---
- kind:
AsyncioDanglingTask: ~
location:
row: 6
column: 4
end_location:
row: 6
column: 49
fix: ~
parent: ~

View file

@ -1864,6 +1864,7 @@
"RUF003",
"RUF004",
"RUF005",
"RUF006",
"RUF1",
"RUF10",
"RUF100",