-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fix hanging upload of large files #3489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meta_repository.save_raw
and event_manager.publish
are sync methods. Calling sync methods from an async method will block the event loop. We could wrap those in sync_to_async
, but I am still puzzled why async_to_sync
doesn't seem to work correctly.
Me too. Super weird. Wanted to put this out here so you could test it to see the problem. |
Took me some time to figure out... If you turn on asyncio debug mode you do get an exception:
The problem is that async_to_sync seems to create its own event loop and that goes wrong with some code in starlette. So it seems we have to create the function the other way away, create it as an async and run the sync code in a safe way. It seems that the general way to do that with FastAPI is to call I also added an integration test that uploads a big file. This one failed before and succeeds now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work
Nice @dekkers, thanks for the additions. I still don't really understand whats going on here but glad it works now :). |
Checklist for QA:
What works:Seems to fix the async errors that were shown in the logs 👍 What doesn't work:n/a Bug or feature?:n/a |
The new create_raw doesn't use request.body anymore so doesn't have the problem. I think it would still be good to add the integration test and we need to backport the fix to 1.17. |
Changes
Make bytes method async. Fixes hanging bytes client.
Issue link
Closes: #3488
Demo
Please add some proof in the form of screenshots or screen recordings to show (off) new functionality, if there are interesting new features for end-users.
QA notes
Please add some information for QA on how to test the newly created code.
Code Checklist
.env
changes files if required and changed the.env-dist
accordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.