Skip to content

Make UploadFile check for future rollover #2962

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
23 changes: 19 additions & 4 deletions starlette/datastructures.py
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,11 @@ def __init__(
self.size = size
self.headers = headers or Headers()

# Capture max size from SpooledTemporaryFile if one is provided. This
# slightly speeds up future checks. Note 0 means unlimited mirroring
# SpooledTemporaryFile's __init__
self._max_mem_size = getattr(self.file, "_max_size", 0)

@property
def content_type(self) -> str | None:
return self.headers.get("content-type", None)
Expand All @@ -438,14 +443,24 @@ def _in_memory(self) -> bool:
rolled_to_disk = getattr(self.file, "_rolled", True)
return not rolled_to_disk

def _will_roll(self, size_to_add: int) -> bool:
# If we're not in_memory then we will always roll
if not self._in_memory:
return True

# Check for SpooledTemporaryFile._max_size
future_size = self.file.tell() + size_to_add
return bool(future_size > self._max_mem_size) if self._max_mem_size else False

async def write(self, data: bytes) -> None:
new_data_len = len(data)
if self.size is not None:
self.size += len(data)
self.size += new_data_len

if self._in_memory:
self.file.write(data)
else:
if self._will_roll(new_data_len):
await run_in_threadpool(self.file.write, data)
else:
self.file.write(data)

async def read(self, size: int = -1) -> bytes:
if self._in_memory:
Expand Down
13 changes: 12 additions & 1 deletion tests/test_formparsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

import os
from contextlib import AbstractContextManager, nullcontext as does_not_raise
from io import BytesIO
from pathlib import Path
from typing import Any

import pytest

from starlette.applications import Starlette
from starlette.datastructures import UploadFile
from starlette.formparsers import MultiPartException, _user_safe_decode
from starlette.formparsers import MultiPartException, MultiPartParser, _user_safe_decode
from starlette.requests import Request
from starlette.responses import JSONResponse
from starlette.routing import Mount
Expand Down Expand Up @@ -303,6 +304,16 @@ def test_multipart_request_mixed_files_and_data(tmpdir: Path, test_client_factor
}


def test_multipart_request_large_file(tmpdir: Path, test_client_factory: TestClientFactory) -> None:
data = BytesIO(b" " * MultiPartParser.spool_max_size * 2)
client = test_client_factory(app)
response = client.post(
"/",
files=[("test_large", data)],
)
assert response.status_code == 200
Copy link
Member

Choose a reason for hiding this comment

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

This test passes on master. We want some condition that proves this PR actually solves the issue.

Copy link

Choose a reason for hiding this comment

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

This type of issue is hard to verify without profiling or adding breakpoints and checking if a write operation that caused a rollover happened on the main thread. I do not know if it is feasable or even possible to provide a unit test that fails if the blocking rollover happens on the wrong thread, that would require assert statements and additional logic in the actual code.

Copy link
Member

Choose a reason for hiding this comment

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

Then we can remove the test, can't we?

Copy link
Author

@HonakerM HonakerM Jul 11, 2025

Choose a reason for hiding this comment

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

@Kludex yeah in it's current state it's not doing a whole lot. Let met try editing the test to use a monkey patched SpooledTemporaryFile to ensure the IO thread is separate from the event thread

Copy link

Choose a reason for hiding this comment

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

It does not ensure that there is no blocking call on the event loop, but it does cover the new code paths that would otherwise not be covered.

Copy link
Author

Choose a reason for hiding this comment

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

@Kludex Just pushed the new test! I ended up creating a new class/app to track where rollover was happening. I'm still new to starlette development so please let me know if you have any code-quality suggestions.



def test_multipart_request_with_charset_for_filename(tmpdir: Path, test_client_factory: TestClientFactory) -> None:
client = test_client_factory(app)
response = client.post(
Expand Down