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

Request.post() stores data in temp files #1469

Closed
popravich opened this issue Dec 9, 2016 · 10 comments
Closed

Request.post() stores data in temp files #1469

popravich opened this issue Dec 9, 2016 · 10 comments
Assignees
Milestone

Comments

@popravich
Copy link
Member

Long story short

web_reqrep.Request.post() reads whole payload into memory and than stores uploads into temp files — implicitly by using cgi.FieldStorage.

Expected behaviour

Either no temporary files or explicit behavior.

Actual behaviour

cgi.FieldStorage creates temporary file for each upload (file).
The FieldStorage docstring states the following:

    The class is subclassable, mostly for the purpose of overriding
    the make_file() method, which is called internally to come up with
    a file open for reading and writing.  This makes it possible to
    override the default choice of storing all files in a temporary
    directory and unlinking them as soon as they have been opened.

So it is possible to control how uploads are handled — either stored into temp files or any other way...

PS: python documentation is missing FieldStorage description.

@asvetlov
Copy link
Member

asvetlov commented Dec 9, 2016

I believe getting rid of temp files is the proper solution: we already have req.multipart() coroutine for handling endless data streams.

@popravich would you create a Pull Request?

@popravich popravich self-assigned this Dec 9, 2016
@popravich
Copy link
Member Author

Will take a look this weekend

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 8, 2017

@popravich did you look into this issue?

@popravich
Copy link
Member Author

Sorry, had no time yet

@fafhrd91 fafhrd91 added this to the 2.0 milestone Feb 21, 2017
@fafhrd91
Copy link
Member

I added new .post() implementation to #1664, it stores data in temp files without loading whole payload into memory (our multipart implementation is better than cgi)

I do not think we should remove post() in favor of multipart(), ergonomics of post() is much better.
but we can combine post() with client_max_size attribute and raise exception if payload exceeds it

@popravich
Copy link
Member Author

Ouch, now imagine containerized application running with readonly filesystem...
I thought it should be possible to control this behavior.

@fafhrd91
Copy link
Member

Good point, I'll add config for that.
But i don't think it is a problem, developer should know what he is doing. .post() is convinience.

@fafhrd91
Copy link
Member

on other hand it is same behavior as in 1.x versions

@fafhrd91
Copy link
Member

added client_max_size support. but I think we should leave post in-place.
.post() is for simplicity, .multipart() is for streaming

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants