Skip to content

Commit 7342717

Browse files
ghislainpiotsonartech
ghislainpiot
authored and
sonartech
committed
SONARPY-2942 Rule S7497: Cancellation exceptions should be re-raised after cleanup (#286)
GitOrigin-RevId: 6d63ebd45d824fcdacdf5e0f50b962a8c1b72357
1 parent dc0db87 commit 7342717

7 files changed

Lines changed: 614 additions & 0 deletions

File tree

‎python-checks/src/main/java/org/sonar/python/checks/CancellationReraisedInAsyncCheck.java‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2025 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
18+
package org.sonar.python.checks;
19+
20+
import javax.annotation.Nullable;
21+
22+
import org.sonar.check.Rule;
23+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
24+
import org.sonar.plugins.python.api.SubscriptionContext;
25+
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
26+
import org.sonar.plugins.python.api.tree.CallExpression;
27+
import org.sonar.plugins.python.api.tree.ExceptClause;
28+
import org.sonar.plugins.python.api.tree.Expression;
29+
import org.sonar.plugins.python.api.tree.Name;
30+
import org.sonar.plugins.python.api.tree.RaiseStatement;
31+
import org.sonar.plugins.python.api.tree.ReturnStatement;
32+
import org.sonar.plugins.python.api.tree.Token;
33+
import org.sonar.plugins.python.api.tree.Tree;
34+
import org.sonar.plugins.python.api.tree.Tree.Kind;
35+
import org.sonar.plugins.python.api.tree.TryStatement;
36+
import org.sonar.python.tree.TreeUtils;
37+
import org.sonar.python.types.v2.TypeCheckMap;
38+
39+
@Rule(key = "S7497")
40+
public class CancellationReraisedInAsyncCheck extends PythonSubscriptionCheck {
41+
42+
private static final String MESSAGE = "Ensure that the %s exception is re-raised after your cleanup code.";
43+
44+
private static final String ASYNCIO_CANCELLED_ERROR = "asyncio.CancelledError";
45+
private static final String TRIO_CANCELLED = "trio.Cancelled";
46+
private static final String ANYIO_CANCELLED = "anyio.get_cancelled_exc_class";
47+
private static final String MESSAGE_SECONDARY = "This function is async.";
48+
49+
private TypeCheckMap<String> cancelledExceptionTypeCheckMap;
50+
51+
@Override
52+
public void initialize(Context context) {
53+
context.registerSyntaxNodeConsumer(Kind.FILE_INPUT, this::initializeChecksForFile);
54+
context.registerSyntaxNodeConsumer(Kind.TRY_STMT, this::checkTryStatement);
55+
}
56+
57+
private void initializeChecksForFile(SubscriptionContext ctx) {
58+
cancelledExceptionTypeCheckMap = new TypeCheckMap<>();
59+
cancelledExceptionTypeCheckMap.put(ctx.typeChecker().typeCheckBuilder().isTypeOrInstanceWithName(ASYNCIO_CANCELLED_ERROR), ASYNCIO_CANCELLED_ERROR);
60+
cancelledExceptionTypeCheckMap.put(ctx.typeChecker().typeCheckBuilder().isTypeWithFqn(TRIO_CANCELLED), TRIO_CANCELLED);
61+
cancelledExceptionTypeCheckMap.put(ctx.typeChecker().typeCheckBuilder().isTypeOrInstanceWithName(ANYIO_CANCELLED), "cancellation");
62+
63+
}
64+
65+
private void checkTryStatement(SubscriptionContext context) {
66+
var tryStatement = (TryStatement) context.syntaxNode();
67+
var enclosingAsyncToken = TreeUtils.asyncTokenOfEnclosingFunction(tryStatement).orElse(null);
68+
69+
if (enclosingAsyncToken == null) {
70+
return;
71+
}
72+
73+
tryStatement.exceptClauses().forEach(exceptClause -> checkExceptClause(context, exceptClause, enclosingAsyncToken));
74+
}
75+
76+
private void checkExceptClause(SubscriptionContext subscriptionContext, ExceptClause exceptClause, Token enclosingAsyncToken) {
77+
var exception = exceptClause.exception();
78+
if (exception == null) {
79+
return;
80+
}
81+
String isOffendingException = isOffendingException(exception);
82+
if (isOffendingException.isEmpty()) {
83+
return;
84+
}
85+
86+
var isCompliant = true;
87+
var hasTopLevelPass = exceptClause.body().statements().stream().anyMatch(s -> s.is(Kind.PASS_STMT));
88+
if (hasTopLevelPass) {
89+
isCompliant = false;
90+
} else {
91+
var complianceChecker = new ComplianceChecker(exceptClause);
92+
exceptClause.accept(complianceChecker);
93+
isCompliant = complianceChecker.isCompliant();
94+
}
95+
96+
if (!isCompliant) {
97+
subscriptionContext.addIssue(
98+
exception,
99+
String.format(MESSAGE, isOffendingException))
100+
.secondary(enclosingAsyncToken, MESSAGE_SECONDARY);
101+
}
102+
}
103+
104+
private String isOffendingException(Expression exception) {
105+
var isOffendingTypeOpt = cancelledExceptionTypeCheckMap.getOptionalForType(exception.typeV2());
106+
if (isOffendingTypeOpt.isPresent()) {
107+
return isOffendingTypeOpt.get();
108+
}
109+
if (exception instanceof CallExpression callExpression) {
110+
var result = cancelledExceptionTypeCheckMap.getOptionalForType(callExpression.callee().typeV2());
111+
if (result.isPresent()) {
112+
return result.get();
113+
}
114+
}
115+
return "";
116+
117+
}
118+
119+
private static boolean isReRaise(RaiseStatement raiseStatement, ExceptClause exceptClause) {
120+
var isBareRaise = raiseStatement.expressions().isEmpty();
121+
var exceptionInstanceName = TreeUtils.toOptionalInstanceOf(Name.class, exceptClause.exceptionInstance());
122+
123+
var isReRaise = raiseStatement.expressions().size() == 1 && exceptionInstanceName.isPresent()
124+
&& raiseStatement.expressions().get(0).is(Kind.NAME)
125+
&& ((Name) raiseStatement.expressions().get(0)).name().equals(exceptionInstanceName.get().name());
126+
127+
return isBareRaise || isReRaise;
128+
}
129+
130+
static class ComplianceChecker extends BaseTreeVisitor {
131+
private final ExceptClause exceptClause;
132+
private boolean hasRaiseStatement = false;
133+
private boolean isCompliant = true;
134+
135+
ComplianceChecker(ExceptClause exceptClause) {
136+
this.exceptClause = exceptClause;
137+
}
138+
139+
boolean isCompliant() {
140+
// If we never found a raise statement, it's not compliant
141+
return isCompliant && hasRaiseStatement;
142+
}
143+
144+
@Override
145+
public void visitRaiseStatement(RaiseStatement raiseStatement) {
146+
hasRaiseStatement = true;
147+
148+
// Check if this is a proper re-raise
149+
if (!isReRaise(raiseStatement, exceptClause)) {
150+
isCompliant = false;
151+
return;
152+
}
153+
154+
super.visitRaiseStatement(raiseStatement);
155+
}
156+
157+
@Override
158+
public void visitReturnStatement(ReturnStatement returnStatement) {
159+
isCompliant = false;
160+
}
161+
162+
@Override
163+
protected void scan(@Nullable Tree tree) {
164+
if (!isCompliant) {
165+
return;
166+
}
167+
super.scan(tree);
168+
}
169+
}
170+
}

‎python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ public Stream<Class<?>> getChecks() {
136136
BusyWaitingInAsyncCheck.class,
137137
CancellationScopeNoCheckpointCheck.class,
138138
CaughtExceptionsCheck.class,
139+
CancellationReraisedInAsyncCheck.class,
139140
ChangeMethodContractCheck.class,
140141
ChildAndParentExceptionCaughtCheck.class,
141142
CipherBlockChainingCheck.class,

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S7497.html‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
<p>This rule raises an issue when a cancellation excception is caught without re-raising it.</p>
2+
<h2>Why is this an issue?</h2>
3+
<p>Asynchronous frameworks like <code>asyncio</code>, <code>trio</code>, and <code>anyio</code> use special exceptions to signal that a task or
4+
operation should be cancelled. These exceptions are not typical errors indicating a logical flaw in the task but are directives for the task to
5+
terminate its execution prematurely and perform necessary cleanup.</p>
6+
<p>When a task is cancelled, the framework typically injects this cancellation exception into it. The task is expected to:</p>
7+
<ul>
8+
<li> Catch this specific cancellation exception. </li>
9+
<li> Perform any urgent and brief cleanup actions (e.g., releasing locks or other resources). </li>
10+
<li> Re-raise the cancellation exception. </li>
11+
</ul>
12+
<p>If a cancellation exception is caught and not re-raised (e.g., it’s suppressed with a <code>pass</code> statement, only logged, the handler returns
13+
normally, or a different exception is raised instead), the cancellation signal is effectively "swallowed".</p>
14+
<p>This prevents the framework and any calling code from knowing that the task has acknowledged the cancellation and is stopping. The task might even
15+
continue running parts of its code after the <code>except</code> block, which is contrary to the purpose of cancellation.</p>
16+
<p>Properly propagating cancellation exceptions is crucial for the cooperative multitasking model these frameworks rely on.</p>
17+
<h3>What is the potential impact?</h3>
18+
<p>Suppressing cancellation exceptions can lead to significant problems:</p>
19+
<ul>
20+
<li> <strong>Unresponsive Applications</strong>: Tasks ignoring cancellation may run indefinitely, making the application unresponsive to shutdown
21+
signals. </li>
22+
<li> <strong>Resource Leaks</strong>: Tasks not stopping properly can leave resources (files, connections, locks) unreleased, leading to resource
23+
exhaustion. </li>
24+
<li> <strong>Incorrect State</strong>: Partial execution of cancelled operations can leave the application in an inconsistent state, risking data
25+
corruption. </li>
26+
<li> <strong>Debugging Difficulties</strong>: Troubleshooting why tasks continue running or why shutdown fails becomes challenging. </li>
27+
<li> <strong>Broken Abstractions</strong>: Reliable cancellation is essential for async patterns and libraries; ignoring it breaks timeouts and task
28+
groups. </li>
29+
</ul>
30+
<h2>How to fix it in Asyncio</h2>
31+
<p>If you need to catch cancellation exceptions for cleanup purposes, make sure to re-raise them after your cleanup code.</p>
32+
<p>Alternatively, you could add a specific handler for cancellation exceptions before your general exception handler.</p>
33+
<h3>Code examples</h3>
34+
<h4>Noncompliant code example</h4>
35+
<pre data-diff-id="1" data-diff-type="noncompliant">
36+
import asyncio
37+
38+
async def compute_result(data): ...
39+
40+
async def process_data(data):
41+
try:
42+
result = await compute_result(data)
43+
return result
44+
except asyncio.CancelledError:
45+
return # Noncompliant
46+
</pre>
47+
<h4>Compliant solution</h4>
48+
<pre data-diff-id="1" data-diff-type="compliant">
49+
import asyncio
50+
51+
async def compute_result(data): ...
52+
53+
async def process_data(data):
54+
try:
55+
result = await compute_result(data)
56+
return result
57+
except asyncio.CancelledError: # Compliant
58+
raise
59+
</pre>
60+
<h2>How to fix it in Trio</h2>
61+
<p>If you need to catch cancellation exceptions for cleanup purposes, make sure to re-raise them after your cleanup code.</p>
62+
<p>Alternatively, you could add a specific handler for cancellation exceptions before your general exception handler.</p>
63+
<h3>Code examples</h3>
64+
<h4>Noncompliant code example</h4>
65+
<pre data-diff-id="2" data-diff-type="noncompliant">
66+
import trio
67+
68+
async def compute_result(data): ...
69+
70+
async def process_data(data):
71+
try:
72+
result = await compute_result(data)
73+
return result
74+
except trio.Cancelled: # Noncompliant
75+
return
76+
</pre>
77+
<h4>Compliant solution</h4>
78+
<pre data-diff-id="2" data-diff-type="compliant">
79+
import trio
80+
81+
async def compute_result(data): ...
82+
83+
async def process_data(data):
84+
try:
85+
result = await compute_result(data)
86+
return result
87+
except trio.Cancelled: # Compliant
88+
raise
89+
</pre>
90+
<h2>How to fix it in AnyIO</h2>
91+
<p>If you need to catch cancellation exceptions for cleanup purposes, make sure to re-raise them after your cleanup code.</p>
92+
<p>Alternatively, you could add a specific handler for cancellation exceptions before your general exception handler.</p>
93+
<h3>Code examples</h3>
94+
<h4>Noncompliant code example</h4>
95+
<pre data-diff-id="3" data-diff-type="noncompliant">
96+
import anyio
97+
98+
async def compute_result(data): ...
99+
100+
async def process_data(data):
101+
try:
102+
result = await compute_result(data)
103+
return result
104+
except anyio.Cancelled: # Noncompliant
105+
return
106+
</pre>
107+
<h4>Compliant solution</h4>
108+
<pre data-diff-id="3" data-diff-type="compliant">
109+
import anyio
110+
111+
async def compute_result(data): ...
112+
113+
async def process_data(data):
114+
try:
115+
result = await compute_result(data)
116+
return result
117+
except anyio.Cancelled: # Compliant
118+
raise
119+
</pre>
120+
<h3>Pitfalls</h3>
121+
<p>Asynchronous cleanup operations in <code>except CancelledError</code> or <code>finally</code> blocks can themselves be interrupted by cancellation.
122+
While <code>asyncio.shield()</code> (or library equivalents) can protect critical cleanup code, use it sparingly as it may delay shutdown.</p>
123+
<h2>Resources</h2>
124+
<h3>Documentation</h3>
125+
<ul>
126+
<li> Asyncio documentation - <a href="https://docs.python.org/3/library/asyncio-task.html#task-cancellation">Task Cancellation</a> </li>
127+
<li> Trio documentation - <a href="https://trio.readthedocs.io/en/latest/reference-core.html#trio.Cancelled">Exceptions and warnings</a> </li>
128+
<li> AnyIO documentation - <a href="https://anyio.readthedocs.io/en/stable/cancellation.html#timeouts">Timeouts</a> </li>
129+
</ul>
130+

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S7497.json‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
{
2+
"title": "Cancellation exceptions should be re-raised after cleanup",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [
10+
"async",
11+
"asyncio",
12+
"trio",
13+
"anyio"
14+
],
15+
"defaultSeverity": "Major",
16+
"ruleSpecification": "RSPEC-7497",
17+
"sqKey": "S7497",
18+
"scope": "All",
19+
"quickfix": "unknown",
20+
"code": {
21+
"impacts": {
22+
"MAINTAINABILITY": "MEDIUM",
23+
"RELIABILITY": "HIGH"
24+
},
25+
"attribute": "LOGICAL"
26+
}
27+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,7 @@
265265
"S7492",
266266
"S7493",
267267
"S7494",
268+
"S7497",
268269
"S7498",
269270
"S7499",
270271
"S7500",

‎python-checks/src/test/java/org/sonar/python/checks/CancellationReraisedInAsyncCheckTest.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2025 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
18+
package org.sonar.python.checks;
19+
20+
import org.junit.jupiter.api.Test;
21+
import org.sonar.python.checks.utils.PythonCheckVerifier;
22+
23+
class CancellationReraisedInAsyncCheckTest {
24+
25+
@Test
26+
void test() {
27+
PythonCheckVerifier.verify("src/test/resources/checks/cancellationReraisedInAsync.py", new CancellationReraisedInAsyncCheck());
28+
}
29+
30+
}

0 commit comments

Comments
 (0)