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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Close FormData (uploaded files) after the request is done #5465

Merged
merged 8 commits into from Nov 3, 2022

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Oct 6, 2022

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2022

📝 Docs preview for commit 1dbbab6 at: https://633efa56cd26a21a02038871--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (adcdf4d) compared to base (e92a864).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #5465   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       540           
  Lines        13946     13969   +23     
=========================================
+ Hits         13946     13969   +23     
Impacted Files Coverage Δ
fastapi/__init__.py 100.00% <100.00%> (ø)
fastapi/routing.py 100.00% <100.00%> (ø)
tests/test_datastructures.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adriangb adriangb changed the title Close uploadfile Close FormData Oct 6, 2022
@adriangb
Copy link
Contributor Author

adriangb commented Oct 6, 2022

Looks like failures are unrelated to this change: #5467

@github-actions
Copy link
Contributor

📝 Docs preview for commit c9d151f at: https://6345ec6f26f61e4cf776afde--fastapi.netlify.app

@adriangb
Copy link
Contributor Author

Failures resolved, @tiangolo I think this is ready for review :)

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2841a7a at: https://635395c8c62971556f3d83d9--fastapi.netlify.app

@tiangolo
Copy link
Owner

Amazing, thank you @adriangb! 🙇

Do you think you could add a test or two for this?

@adriangb
Copy link
Contributor Author

I think this might difficult to test without touching the internals of something. It does run for every test, so we know that it doesn’t crash at least. I’ll think a bit about how to test this.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 8a28c02 at: https://636003d3f67bae05d1213030--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 1887507 at: https://636014d7a81e9f143d87f066--fastapi.netlify.app

@adriangb
Copy link
Contributor Author

So it seems like even Starlette is not testing that UploadFile.close gets called. I don't see any way to test this other than doing some monkey patching of stuff like that. @tiangolo would you accept this without a test or is that a no-go?

@github-actions
Copy link
Contributor

📝 Docs preview for commit a1fa01b at: https://636033086842892df4729409--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

📝 Docs preview for commit adcdf4d at: https://6363ae2be9a9080232218a2a--fastapi.netlify.app

@tiangolo tiangolo changed the title Close FormData 🐛 Close FormData (uploaded files) after the request is done Nov 3, 2022
@tiangolo
Copy link
Owner

tiangolo commented Nov 3, 2022

I found a way to test it! 🎉 I added the commit on top.

Thanks for the contribution! 🚀

@tiangolo tiangolo merged commit ac9f56e into tiangolo:master Nov 3, 2022
@adriangb adriangb deleted the close-uploadfile branch November 3, 2022 13:17
@adriangb
Copy link
Contributor Author

adriangb commented Nov 3, 2022

That was genius!

@tiangolo
Copy link
Owner

tiangolo commented Nov 3, 2022

Haha thank you! 😊🙈🤓

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

Successfully merging this pull request may close these issues.

None yet

4 participants