fix: Use anyio.SpooledTemporaryFile in UploadFile #2925
Conversation
| @@ -413,7 +414,7 @@ class UploadFile: | |||
|
|
|||
| def __init__( | |||
| self, | |||
| file: typing.BinaryIO, | |||
| file: anyio.SpooledTemporaryFile[bytes], | |||
There was a problem hiding this comment.
Is there a base class analogous to BinaryIO? This was not a spoiled temporary file type before on purpose.
There was a problem hiding this comment.
Because that shouldn't matter for the UploadFile.
There was a problem hiding this comment.
Because that shouldn't matter for the UploadFile.
Yeah, that makes sense — the original use of BinaryIO was definitely meant to keep things implementation-agnostic, and I get why switching directly to SpooledTemporaryFile might feel too specific.
If you're open to it, I think one possible middle ground could be introducing a small AsyncFile protocol — something simple that defines the expected async methods (read, write, seek, etc). That way, we can preserve the flexibility while still ensuring everything stays fully async.
Happy to adjust the PR in that direction if it sounds reasonable to you!
There was a problem hiding this comment.
Sounds reasonable, but maybe that structure already exists in anyio? 🤔
There was a problem hiding this comment.
Sounds reasonable, but maybe that structure already exists in anyio? 🤔
Yes, it seems like anyio provides a type called AsyncFile for async file operations
There was a problem hiding this comment.
AsyncFile is a concrete implementation not an abc, probably uh "AsyncFileIO[bytes]" protocol would make sense here.
If it's a protocol we can move it or have it in multiple places and have everything still work
There was a problem hiding this comment.
In fact, I don't think the file parameter should be exposed here. It seems to give users the power to customize the file, but in fact the other parts of UploadFile rely heavily on the implementation of file. If we need to make some abstractions to allow users to customize the file parsing results of multipart, we can use a design similar to baize. But at least in this PR, I think there is no problem with modifying the type in this way.
starlette/formparsers.py
Outdated
| except MultiPartException as exc: | ||
| # Close all the files if there was an error. | ||
| async with AsyncExitStack() as stack: | ||
| for f in self._files_to_close_on_error: |
There was a problem hiding this comment.
- I mean that _files_to_close_on_error would just be an exit stack
- these should be closed on any error including BaseException
| @@ -265,10 +267,9 @@ async def parse(self) -> FormData: | |||
| await part.file.seek(0) | |||
| self._file_parts_to_write.clear() | |||
| self._file_parts_to_finish.clear() | |||
| except MultiPartException as exc: | |||
| except BaseException as exc: | |||
There was a problem hiding this comment.
Why did this change?
I changed it to BaseException assuming it would help ensure cleanup even for cases like CancelledError or KeyboardInterrupt, beyond just MultiPartException.
But perhaps I misunderstood — please let me know if I’m thinking about this the wrong way!
There was a problem hiding this comment.
CancelledError being the most likely one
Summary
This PR updates the internal implementation of
UploadFileto useanyio.SpooledTemporaryFile[bytes]instead of a genericBinaryIOinterface.By using
anyio.SpooledTemporaryFile, all file operations (read,write,seek) are now fully asynchronous and consistent across both in-memory and disk-based file handling. This eliminates the need forrun_in_threadpooland simplifies the logic for determining whether the file is in memory or on disk.This is my first contribution to Starlette. If any part of this PR doesn't follow the project's conventions or needs to be revised or split, I’m more than happy to make the necessary changes.
Checklist