Fix download workspace zip file event loop hanging #6722
+20
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diwu-sf
commented
Feb 14, 2025
| @@ -21,12 +20,13 @@ | |||
|
|
|||
| from fastapi import Depends, FastAPI, HTTPException, Request, UploadFile | |||
| from fastapi.exceptions import RequestValidationError | |||
| from fastapi.responses import JSONResponse, StreamingResponse | |||
There was a problem hiding this comment.
dont try to hand build a StreamingResponse just let FileResponse handle it, it's already specialized for sending large files
diwu-sf
force-pushed
the
fix-download-file-event-loop-hang
branch
from
05a2aaf to
ecb7d53
Compare
diwu-sf
commented
Feb 14, 2025
| for root, _, files in os.walk(path): | ||
| for file in files: | ||
| file_path = os.path.join(root, file) | ||
| zipf.write( | ||
| file_path, arcname=os.path.relpath(file_path, path) | ||
| ) | ||
| temp_zip.seek(0) # Rewind the file to the beginning after writing | ||
| content = temp_zip.read() |
There was a problem hiding this comment.
this is bad, seek(0), read() just brings the entire file back in memory, won't scale to workspace with 1GB+ of files
diwu-sf
force-pushed
the
fix-download-file-event-loop-hang
branch
from
ecb7d53 to
1bf4bfa
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A bunch of problems with the whole download workspace:
action_execution_serverthe entire zip operation (compute intensive, blocking IO), and the entire zip file was read back in memory, on theasync def ...route handler, blocking that fastapi event loop, fix this by running that route as a sync handler, and not reading the file back in-mem, letFileResponsehandle streaming it back out asynchronouslyaction_execution_clienttheres unnecessary looping of the content stream, just letshutil.copyfileobjhandle streaming that raw socket out to the temp filezip_current_workspaceeven though its an async handler, it immediately throws the real work onto call_sync_from_async, so might as well just run this route handler entirely on the fastAPI blocking threadpool instead, fix the mimetype of the zip file to the right standard oneFileResponseobject, so when it finishes streaming, it will delete the temp fileVerified by adding a 1GB random incompressible file into the workspace and hitting the
/zip-directoryendpoint to retrieve the zip. The only issue remaining is that the entire workspace zip file has to be fully generated before the download stream begins. Ideally, it would be streaming the zip the entire way through incrementally as files are compressed.