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.

Sign up for GitHub

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

Detect blocking calls in coroutines using BlockBuster #2858

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Detect blocking calls in coroutines using BlockBuster #2858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34fc433
Detect blocking calls in coroutines using BlockBuster
cbornet Feb 1, 2025
0d86ba1
Update .gitignore
cbornet Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ types-PyYAML==6.0.12.20241230
types-dataclasses==0.6.6
pytest==8.3.4
trio==0.28.0
blockbuster==1.5.13

# Documentation
black==24.10.0
Expand Down
3 changes: 2 additions & 1 deletion starlette/datastructures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import sys
import typing
from shlex import shlex
from urllib.parse import SplitResult, parse_qsl, urlencode, urlsplit
Expand Down Expand Up @@ -438,7 +439,7 @@ async def write(self, data: bytes) -> None:
if self.size is not None:
self.size += len(data)

if self._in_memory:
if self._in_memory and self.file.tell() + len(data) <= getattr(self.file, "_max_size", sys.maxsize):
Copy link
Member

Choose a reason for hiding this comment

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

what's this about?

Copy link
Author

Choose a reason for hiding this comment

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

If the data to write makes the file exceed the SpooledTemporaryFile _max_size, self.file.write will do a blocking rollover operation.

self.file.write(data)
else:
await run_in_threadpool(self.file.write, data)
Expand Down
4 changes: 3 additions & 1 deletion starlette/middleware/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import traceback
import typing

import anyio

from starlette._utils import is_async_callable
from starlette.concurrency import run_in_threadpool
from starlette.requests import Request
Expand Down Expand Up @@ -167,7 +169,7 @@ async def _send(message: Message) -> None:
request = Request(scope)
if self.debug:
# In debug mode, return traceback responses.
response = self.debug_response(request, exc)
response = await anyio.to_thread.run_sync(self.debug_response, request, exc)
elif self.handler is None:
# Use our default 500 error handler.
response = self.error_response(request, exc)
Expand Down
2 changes: 1 addition & 1 deletion starlette/testclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ async def receive() -> Message:
await response_complete.wait()
return {"type": "http.disconnect"}

body = request.read()
body = await anyio.to_thread.run_sync(request.read)
Copy link
Author

Choose a reason for hiding this comment

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

Httpx request.read can be blocking (eg: reading multipart file).
await request.aread() can't be used as-is since the request stream is a SyncByteStream not an AsyncByteStream.

if isinstance(body, str):
body_bytes: bytes = body.encode("utf-8") # pragma: no cover
elif body is None:
Expand Down
9 changes: 9 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
from __future__ import annotations

import functools
from collections.abc import Iterator
from typing import Any, Literal

import pytest
from blockbuster import blockbuster_ctx

from starlette.testclient import TestClient
from tests.types import TestClientFactory


@pytest.fixture(autouse=True)
def blockbuster() -> Iterator[None]:
with blockbuster_ctx("starlette") as bb:
bb.functions["os.stat"].can_block_in("/mimetypes.py", "init")
Copy link
Author

Choose a reason for hiding this comment

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

FileResponse's constructor calls mimetypes .guess_type which is blocking the first time it is called.

yield


@pytest.fixture
def test_client_factory(
anyio_backend_name: Literal["asyncio", "trio"],
Expand Down
4 changes: 2 additions & 2 deletions tests/middleware/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,9 +466,9 @@ async def cancel_on_disconnect(
# before we start returning the body
await task_group.start(cancel_on_disconnect)

# A timeout is set for 0.1 second in order to ensure that
# A timeout is set for 0.2 second in order to ensure that
# we never deadlock the test run in an infinite loop
with anyio.move_on_after(0.1):
with anyio.move_on_after(0.2):
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Author

@cbornet cbornet Feb 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

BlockBuster has a small toll on performance.
In CI, the test machines are already slow, the cancellation sometimes arrives too late and the test becomes flaky (I had no issue on my computer, only in CI).

while True:
await send(
{
Expand Down
12 changes: 6 additions & 6 deletions tests/test_templates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def test_templates(tmpdir: Path, test_client_factory: TestClientFactory) -> None
with open(path, "w") as file:
file.write("<html>Hello, <a href='{{ url_for('homepage') }}'>world</a></html>")

async def homepage(request: Request) -> Response:
def homepage(request: Request) -> Response:
Copy link
Author

@cbornet cbornet Feb 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is it OK to change ?
templates.TemplateResponse is blocking.

Copy link
Member

Choose a reason for hiding this comment

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

Why is templates.TemplateResponse blocking?

Copy link
Author

Choose a reason for hiding this comment

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

It loads the index.html from the FS using jinja2.FileSystemLoader.get_source() which does blocking calls.
The path is:

test_templates.py:27: in homepage
    return templates.TemplateResponse(request, "index.html")
../starlette/templating.py:208: in TemplateResponse
    template = self.get_template(name)
../starlette/templating.py:131: in get_template
    return self.env.get_template(name)
../venv/lib/python3.11/site-packages/jinja2/environment.py:1016: in get_template
    return self._load_template(name, globals)
../venv/lib/python3.11/site-packages/jinja2/environment.py:975: in _load_template
    template = self.loader.load(self, name, self.make_globals(globals))
../venv/lib/python3.11/site-packages/jinja2/loaders.py:126: in load
    source, filename, uptodate = self.get_source(environment, name)
../venv/lib/python3.11/site-packages/jinja2/loaders.py:204: in get_source
    if os.path.isfile(filename):

path.isfile is a blocking call as it does an os.stat.
The get_source code also does a file.read() blocking call later.

return templates.TemplateResponse(request, "index.html")

app = Starlette(debug=True, routes=[Route("/", endpoint=homepage)])
Expand All @@ -40,7 +40,7 @@ def test_calls_context_processors(tmp_path: Path, test_client_factory: TestClien
path = tmp_path / "index.html"
path.write_text("<html>Hello {{ username }}</html>")

async def homepage(request: Request) -> Response:
def homepage(request: Request) -> Response:
return templates.TemplateResponse(request, "index.html")

def hello_world_processor(request: Request) -> dict[str, str]:
Expand Down Expand Up @@ -69,7 +69,7 @@ def test_template_with_middleware(tmpdir: Path, test_client_factory: TestClientF
with open(path, "w") as file:
file.write("<html>Hello, <a href='{{ url_for('homepage') }}'>world</a></html>")

async def homepage(request: Request) -> Response:
def homepage(request: Request) -> Response:
return templates.TemplateResponse(request, "index.html")

class CustomMiddleware(BaseHTTPMiddleware):
Expand All @@ -96,15 +96,15 @@ def test_templates_with_directories(tmp_path: Path, test_client_factory: TestCli
template_a = dir_a / "template_a.html"
template_a.write_text("<html><a href='{{ url_for('page_a') }}'></a> a</html>")

async def page_a(request: Request) -> Response:
def page_a(request: Request) -> Response:
return templates.TemplateResponse(request, "template_a.html")

dir_b = tmp_path.resolve() / "b"
dir_b.mkdir()
template_b = dir_b / "template_b.html"
template_b.write_text("<html><a href='{{ url_for('page_b') }}'></a> b</html>")

async def page_b(request: Request) -> Response:
def page_b(request: Request) -> Response:
return templates.TemplateResponse(request, "template_b.html")

app = Starlette(
Expand Down Expand Up @@ -150,7 +150,7 @@ def test_templates_with_environment(tmpdir: Path, test_client_factory: TestClien
with open(path, "w") as file:
file.write("<html>Hello, <a href='{{ url_for('homepage') }}'>world</a></html>")

async def homepage(request: Request) -> Response:
def homepage(request: Request) -> Response:
return templates.TemplateResponse(request, "index.html")

env = jinja2.Environment(loader=jinja2.FileSystemLoader(str(tmpdir)))
Expand Down