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

provide apis to assist in cleaning up temporary files created by webob during a request #415

Open
mmerickel opened this issue Jun 29, 2020 · 3 comments

Comments

@mmerickel
Copy link
Member

File uploads with webob.compat.cgi_FieldStorage should be cleaned up at the end of a request incase a user forgets to do it themselves. For example, request.POST['avatar'].file will open a temporary file and return a handle to it. This handle can leak if not closed when it's over the threshold triggering it to use a temporary file instead of temporary memory buffer.

I would posit that webob should make sure this file is not leaked at the end of the request and should more aggressively clean it up. It seems like the field storage could be opened with a reference to the request, and then request-level apis could be provided to cleanup those objects either at the end of the request, or before, with worst-case being in request.__del__. If webob had this api, I think Pyramid could hook into it and ensure it's used automatically to avoid any weird leakage between requests. It seems to me that it could be cleared automatically in app_iter.close(), as well, after the response is done being served.

I'm having trouble finding good reasons to support files being hung onto after a request is over, but an api to detach it could be provided as well on a per-field basis.

  • request.close_all_field_resources() could be called automatically by request.__del__.
  • request.detach_field_resource(request.POST['foo'].file) could be called earlier to detach a specific file pointer so that it isn't closed automatically.
  • response.add_closer() with a callback that's invoked after the app_iter is exhausted that frameworks could use to hook arbitrary callbacks, such as response.add_closer(request.close_all_field_resources).
@mmerickel
Copy link
Member Author

As a secondary note, wsgi.input also leaks the tempfile created when make_body_seekable() is invoked and there is not a good place to close it so it is left for the garbage collector. I don't think PEP 3333 allows for arbitrary invocations of environ['wsgi.input'].close() so it's specific to webob's patching of the value to something that is closable and thus webob should provide some sort of cleanup function around it.

@digitalresistor
Copy link
Member

This handle can leak if not closed when it's over the threshold triggering it to use a temporary file instead of temporary memory buffer.

https://github.com/python/cpython/blob/master/Lib/cgi.py#L484

It'll eventually get cleaned up by the garbage collector right now, should there be a finalizer to do it sooner, probably.

The tempfile also eventually gets cleaned up, but its not great. It's all not great.

@romuald
Copy link

romuald commented Feb 28, 2024

Just got here from having the issue in one of our test suites.

As far as I can tell, this is causing problem because the temporary file is removed using the garbage collector because there is a reference cycle.
I know nothing of the data structures of webob, but wouldn't using a weakref resolve the issue?

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

No branches or pull requests

3 participants