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.

By clicking “Sign up for GitHub”, you agree to ourterms of serviceand privacy statement.We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing UploadFile objects into a StreamingResponse closes it in v0.106.0 but not v0.105.0 #10857

Open
9 tasks done
Kludexopened this issue Dec 29, 2023 Discussed in #10856 · 26 comments
Open
9 tasks done

Comments

@Kludex
Copy link
Member

Kludex commented Dec 29, 2023

Discussed in#10856

Originally posted byadrwzDecember 28, 2023

First Check

  • I added a very descriptive title here.
  • I used the GitHub search to find a similar question and didn't find it.
  • I searched the FastAPI documentation, with the integrated search.
  • I already searched in Google "How to X in FastAPI" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to FastAPI but toPydantic.
  • I already checked if it is not related to FastAPI but toSwagger UI.
  • I already checked if it is not related to FastAPI but toReDoc.

Commit to Help

  • I commit to help with one of those options 👆

Example Code

asyncdefupload_file_event(files:List[UploadFile]):
print("2",files[0].filename,files[0].file._file.closed)

#... more code...


@router.post("/upload_session_file")
asyncdefupload_file(files:List[UploadFile]=File(...)):
print("1",files[0].filename,files[0].file._file.closed)

try:
returnStreamingResponse(
upload_file_event(files),
media_type="text/event-stream",
)
exceptExceptionaserror:
logging.error(error)
raiseHTTPException(status_code=500,detail="Internal Server Error")

Description

Hitting this endpoint with an uploaded file on v0.105.0 will print:

1 filename false
2 filename false

Hitting this endpoint with an uploaded file on v0.106.0 will print:

1 filename false
2 filename true

Unfortunately this means I'm unable to upload files in a StreamingResponse with fastapi>=0.106.0

Operating System

macOS

Operating System Details

No response

FastAPI Version

0.105.0

Pydantic Version

2.5.3

Python Version

3.11.4

Additional Context

No response

@inverse-panda
Copy link

inverse-panda commented Jan 8, 2024

Is there any update on this issue? I have the same problem, just migrated fromv0.103.0tov0.108.0and realized my tests are failing due the same issue. Is is related tothis breaking change?
If yes, what is the proposed approach for handling the UploadFile in the background task?
my old practice is written below

asyncdefupload_csv_to_gcloud_blob_storage(
self,file:UploadFile,bucket_name:str,blob_storage_path:str
):
"""write the data to a bucket with" ""
storage_client=storage.Client()
csv_bucket=storage_client.bucket(bucket_name)
blob=csv_bucket.blob(blob_storage_path)

try:
withblob.open("wb")asbuffer:
shutil.copyfileobj(file.file,buffer)
finally:
file.file.close()

@app.post("/csv_tables")
asyncdefupload_csv(
csv_file:Annotated[UploadFile,File()],
background_tasks:BackgroundTasks,
):

background_tasks.add_task(
upload_csv_to_gcloud_blob_storage,
csv_file=csv_file,
bucket_name="csv",
blob_storage_path="csv_path",
)
return"OK"

@daviddavis
Copy link

daviddavis commented Jan 9, 2024

We ran into this issue. Callingfile.read()in code our began raising an error after upgrading fastapi:ValueError: I/O operation on closed file.

@adlmtl
Copy link

adlmtl commented Jan 10, 2024

I'm having similar issue with a tempfile I'm yielding....

def create_temp_file():
fd, path = tempfile.mkstemp(suffix='.csv')
try:
yield fd, path
finally:
os.unlink(path)

When I try to return the file in a FileResponse I hit:
RuntimeError(f "File at path {self.path} does not exist." )

@msehnout
Copy link

Is there any update on this issue? I have the same problem, just migrated fromv0.103.0tov0.108.0and realized my tests are failing due the same issue. Is is related tothis breaking change?If yes, what is the proposed approach for handling the UploadFile in the background task? my old practice is written below

It seems to be, removing this line prevent the file in the body from being closed:https://github /tiangolo/fastapi/blob/958425a899642d1853a1181a3b89dcb07aabea4f/fastapi/routing.py#L231

It was introduced in the only commit changing the code between versions 105 and 106:
a4aa79e#diff-4d573079004a9f3d148baa4658e68e82b8a3d1a95d603fee8177daa92cf65c93R231

Unfortunately theget_request_handleris not trivial, so I cannot create PR without a bit of help.

@msehnout
Copy link

I went through theget_request_handlerfunction again and maybe the behavior is intentional? I don't know. Anyway the problem is that it simply runs the endpoint handler:
https://github /tiangolo/fastapi/blob/958425a899642d1853a1181a3b89dcb07aabea4f/fastapi/routing.py#L294
and then closes the file. But in case the handler is asynchronous, like a background task:

@app.post(...)
asyncdeffoo():
background_tasks.add_task(process_recording_file,file=file,...)

it closes the file before the task can actually run. So I created a deep copy like this:

-background_tasks.add_task(process_recording_file, file=file,...)
+background_tasks.add_task(process_recording_file, file=copy.deepcopy(file),...)

and I close the file myself in the function:

asyncdefprocess_recording_file(file:UploadFile...):
try:
...
finally:
awaitfile.close()

which fixes the issue even with the newest version of FastAPI. It is probably not very efficient given that the file is few MB in my case.

So maybe a better question would be, what is the expected usage of UploadFile combined with BackgroundTasks?https://fastapi.tiangolo /tutorial/background-tasks/

@wu-clan
Copy link

wu-clan commented Jan 10, 2024

Hi,@msehnout

Interesting thinking, but I don't think it's a backstage task. Look at this.:encode/starlette#2176 (comment)

@Kludex
Copy link
Member Author

Kludex commented Jan 10, 2024

How is my comment related?

@wu-clan
Copy link

wu-clan commented Jan 10, 2024

I just want to express that the idea of @ msehnout may be backward incompatible because he mentioned background tasks, it seems to have nothing to do with your comments?

@rushilsrivastava
Copy link

rushilsrivastava commented Jan 11, 2024

I'm having similar issue with a tempfile I'm yielding....

def create_temp_file():
fd, path = tempfile.mkstemp(suffix='.csv')
try:
yield fd, path
finally:
os.unlink(path)

When I try to return the file in a FileResponse I hit:RuntimeError(f "File at path {self.path} does not exist." )

Related discussion:#10850

I am assumingcreate_temp_fileis used as a dependency. This error is happening because the dependency is fully resolved before the response is returned to the client. Previously, this worked because the exit code was executed after the response was sent. At the moment, I think the only way to work around this is using Background Tasks, as discussed in#2512.

@msehnout
Copy link

I'm not sure the problem I'm experiencing is actually an issue or a bad design choice on my side, so I created a discussion instead:#10936

@tjbck
Copy link

tjbck commented Mar 22, 2024

Any updates on this? deepcopying isn't an option for me.

@uniartisan
Copy link

uniartisan commented Mar 31, 2024

Looking forward to an update. copy.deepcopy not work for me. I have no choice but to downgrade fastapi

@chrisk314
Copy link

I'm also hitting this. Need to perform some long running processing steps on uploaded files and it's no longer possible with BackgroundTasks due to the file being closed when returning the response. So this promise from theFastAPI docs on BackgroundTasksis now broken.

For example, let's say you receive a file that must go through a slow process, you can return a response of "Accepted" (HTTP 202) and process it in the background.

@chrisk314
Copy link

chrisk314 commented Apr 26, 2024

In case it helps others, for now I'm working around this issue by adding my ownUploadFileclass which wraps and patches thefastapi.UploadFileclass. Tested and it's working as expected.BackgroundTasknow processes data (i.e. uploads to S3) correctly in the background. Don't forget, using this approach you need to close theUploadFilemanually in yourBackgroundTaskor otherwise. Not ideal or a long-term solution but it's doing the job.

fromfastapiimportUploadFileas_UploadFile

classUploadFile(_UploadFile):
"""Patches `fastapi.UploadFile` due to buffer close issue.

See the related github issue:
https://github /tiangolo/fastapi/issues/10857
"""

def__init__(self,upload_file:_UploadFile)->None:
"""Wraps and mutates input `fastapi.UploadFile`.

Swaps `close` method on the input instance so it's a no-op when called
by the framework. Adds `close` method of input as `_close` here, to be
called later with overridden `close` method.
"""
self.filename=upload_file.filename
self.file=upload_file.file
self.size=upload_file.size
self.headers=upload_file.headers

_close=upload_file.close
setattr(upload_file,"close",self._close)
setattr(self,"_close",_close)

asyncdef_close(self)->None:
pass

asyncdefclose(self)->None:# noqa: D102
awaitself._close()

@maxupp
Copy link

maxupp commented May 6, 2024

Also running into this, any plans for a timely fix?

@itssimon
Copy link
Contributor

We're also hitting this and are stuck with FastAPI 0.105.0 on multiple projects until this is fixed.

@Tuanshu
Copy link

Tuanshu commented Jun 7, 2024

Just encoutered similar issue.
I can confirm downgrade to v0.105.0 works for me.

@kuzvac
Copy link

kuzvac commented Jun 13, 2024

Can anyone upgrade status to this issue to make it more visible/urgent?
It's like very annoying issue and heavy breaking change from v105 to v106.
Simple saving files thats works in 105 stop working in 106. Can't find examples how it should work with background tasks, or guides to migrate to new version.
@Kludex,@tiangolocan you please some help with examples how made simple saving files?

@bhotkachoda
Copy link

Does anyone have a fix for the background task stuff?

@chrisk314
Copy link

@vicef5the solution I suggested abovehas been working fine for us as a patch until a more long term solution is introduced.

@bhotkachoda
Copy link

@vicef5the solution I suggested abovehas been working fine for us as a patch until a more long term solution is introduced.

Thanks man
I just downgraded to v105 to get it working

@M1guelJesus
Copy link

I have the same problem while using background tasks to upload a file (class UploadFile) to a bucket. My temporary solution was to use deepcopy

@EamonnHegarty
Copy link

Hey guys did anyone have a good work around for this? Or is downgrading still the best option

@bhotkachoda
Copy link

Hey guys did anyone have a good work around for this? Or is downgrading still the best option

Eamonn, downgrading is the easiest solution

@SiddarthNarayanan01
Copy link

+1

@bhotkachoda
Copy link

+1 😭😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests