SONARPY-2893 Create rule S7487: Async functions should not contain synchronous subprocess calls #5004
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7487
branch
from
93636c0 to
c722a9e
Compare
guillaume-dequenne
changed the title
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7487
branch
2 times, most recently
from
74915f9 to
94d0aaa
Compare
rules/S7487/python/metadata.json
Outdated
| "tags": [ | ||
| ], |
rules/S7487/python/rule.adoc
Outdated
| 1. The event loop is completely blocked until the subprocess operation completes | ||
| 2. No other coroutines can run during this time, even if they're ready to execute | ||
| 3. The responsiveness of the application is degraded | ||
| 4. In server applications, this can cause timeouts or failures for other concurrent requests | ||
|
|
||
| Instead, async libraries provide dedicated APIs for running subprocesses in a non-blocking way: | ||
|
|
||
| * `asyncio.create_subprocess_exec()` and `asyncio.create_subprocess_shell()` for asyncio | ||
| * `trio.run_process()` for Trio | ||
| * `anyio.run_process()` for AnyIO |
There was a problem hiding this comment.
I am not sure about mixing bullet points and a numbered list
rules/S7487/python/rule.adoc
Outdated
| ==== Noncompliant code example | ||
|
|
||
| [source,python,diff-id=1,diff-type=noncompliant] | ||
| ---- | ||
| import asyncio | ||
| import subprocess | ||
|
|
||
| async def process_data(): | ||
| subprocess.run(["process_file", "data.txt"]) # Noncompliant | ||
| ---- | ||
|
|
||
| ==== Compliant solution | ||
|
|
||
| [source,python,diff-id=1,diff-type=compliant] | ||
| ---- | ||
| import asyncio | ||
|
|
||
| async def process_data(): | ||
| proc = await asyncio.create_subprocess_exec( | ||
| "process_file", "data.txt", | ||
| stdout=asyncio.subprocess.PIPE | ||
| ) | ||
| await proc.wait() | ||
| ---- | ||
|
|
||
| == How to fix it in Trio | ||
|
|
||
| Replace synchronous subprocess calls with `trio.run_process()`, which handles both command arrays and shell commands. | ||
|
|
||
| === Code examples | ||
|
|
||
| ==== Noncompliant code example | ||
|
|
||
| [source,python,diff-id=2,diff-type=noncompliant] | ||
| ---- | ||
| import trio | ||
| import subprocess | ||
|
|
||
| async def download_files(): | ||
| async with trio.open_nursery() as nursery: | ||
| result = subprocess.run(["wget", "https://example.com/file.zip"]) # Noncompliant | ||
| ---- | ||
|
|
||
| ==== Compliant solution | ||
|
|
||
| [source,python,diff-id=2,diff-type=compliant] | ||
| ---- | ||
| import trio | ||
|
|
||
| async def download_files(): | ||
| async with trio.open_nursery() as nursery: | ||
| result = await trio.run_process(["wget", "https://example.com/file.zip"]) | ||
| ---- | ||
|
|
There was a problem hiding this comment.
The exemples for different libraries not doing the same thing feels a bit strange to me
There was a problem hiding this comment.
The examples are still different, between image converting and downloading files. Is that expected?
There was a problem hiding this comment.
Good point, I didn't really focus on that specific part, I aligned this.
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7487
branch
from
c85516b to
9a4c170
Compare
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7487
branch
from
9a4c170 to
5b2d5a4
Compare
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "async", "asyncio", "AnyIO", "Trio" |
There was a problem hiding this comment.
The case is different than in other rules
| "impacts": { | ||
| "RELIABILITY": "HIGH" | ||
| }, | ||
| "attribute": "EFFICIENT" |
There was a problem hiding this comment.
I am not sure of the attribute, but I can't find a better one
There was a problem hiding this comment.
My rationale was that the resulting problem would mostly be a performance one.
rules/S7487/python/rule.adoc
Outdated
|
|
||
| === Highlighting | ||
| * Primary locations: the `subprocess` callee within an async function | ||
| * Secondary locations: the enclosing async function name (message: "this is an asynchronous function") |
There was a problem hiding this comment.
Sorry for the late notice, but shouldn't almost all of our coroutine rules have a secondary on the async keyword?
There was a problem hiding this comment.
Good point, I have modified it to reflect that. I believe we should do it for most of our rules, I'll double check the ones I've specified and update accordingly.
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7487
branch
from
299c9ca to
9778f95
Compare
|
|
rules/S7487/python/rule.adoc
Outdated
|
|
||
| Using these APIs allows other tasks to continue executing while waiting for the subprocess to complete. | ||
|
|
||
| == How to fix it in asyncio |
There was a problem hiding this comment.
| == How to fix it in asyncio | |
| == How to fix it in Asyncio |
|
|
Wohops
changed the title
You can preview this rule here (updated a few minutes after each push).
Review
A dedicated reviewer checked the rule description successfully for: