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

fix: yield form in fastui_form to keep file handler open #170

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gradient-ascent-ai-lab
Copy link

@gradient-ascent-ai-lab gradient-ascent-ai-lab commented Feb 4, 2024

I ran into the same issue as #146, where in all cases the file handler of an UploadFile is already closed when it becomes available in the endpoint function. I found where the closing of a context manager leads to the closing of the filehandler, and found that yielding inside the request.form() context instead of returning after it fixes it. I'm not sure if this is the ideal solution (probably not as CI is breaking on the typechecker) but at least it's pretty minimal and fixes the issue, so I thought it useful enough to share.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much. Please fix tests and and some more tests that would fail without this.

@gradient-ascent-ai-lab
Copy link
Author

Glad to hear the contribution is appreciated!

I'd love to fix the tests and add more, but looking deeper into the issue I am not sure my fix is the correct approach, as it requires changing all lines with .dependency(request) to .dependency(request).__anext__(), and I can't imagine this is desired syntax, it feels hacky. Besides this, if I add the line assert not m.profile_pic.file.closed to for example the test_file_submit test, and I do not include my fix, the test actually passes, which is surprising.

I find my mental model of the control flow through the layers of FastUI/FastAPI/Starlette (especially with classes like Starlette'sAwaitableOrContextManagerWrapper) is not sufficient at this time to be confident in further code changes.
I can dive deeper, but don't have the bandwidth right now. I hope to find time for this in the near future, but if anyone else feels confident in the changes that are required here, I'm open to guidance or someone else stepping in.

ninoseki added a commit to ninoseki/FastUI that referenced this pull request Feb 8, 2024
@samuelcolvin
Copy link
Member

Well the tests having odd syntax isn't the end of the world as long as real world cases are handled correctly and elegantly.

I think the solution might be as simple as adding @asynccontextmanager to run_fastui_form, then using async with in the tests instead of just await. To get tests to fail correctly, I guess you'll need to update FakeRequest to to someone close any files after they're returned.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3cacbb) 94.30% compared to head (e6ccc96) 94.31%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
+ Coverage   94.30%   94.31%   +0.01%     
==========================================
  Files          11       11              
  Lines         755      757       +2     
==========================================
+ Hits          712      714       +2     
  Misses         43       43              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gradient-ascent-ai-lab
Copy link
Author

Thanks for the guidance!
I've added the @asynccontextmanager, updated the syntax of the tests to use async with, updated the FakeRequest to close files after yielding, added assert not {filehandler}.closed wherever UploadFile is used, and all tests now pass.
I've rebased on main, if there's anything else this PR needs let me know.

@abuchnick-aiola
Copy link

abuchnick-aiola commented Mar 22, 2024

@samuelcolvin Any progress on this ?
Currently I'm using the monkeypatch @gradient-ascent-ai-lab suggested here, but prefer avoiding this of course.

@matthewbelcher
Copy link

I've run into this issue myself. Is this PR going to make it to master or is there another better fix?

@rgimen3z
Copy link
Contributor

@sydney-runkle / @samuelcolvin, seems like this PR is ready to be merged and would be
very useful for many users. Mind taking another look when you have a chance? Thanks

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

Successfully merging this pull request may close these issues.

5 participants