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

Sending >=500 KB file with test client causes warning #1785

Closed
Asa-Nisi-Masa opened this issue Apr 3, 2020 · 6 comments · Fixed by #2041
Closed

Sending >=500 KB file with test client causes warning #1785

Asa-Nisi-Masa opened this issue Apr 3, 2020 · 6 comments · Fixed by #2041
Milestone

Comments

@Asa-Nisi-Masa
Copy link

Asa-Nisi-Masa commented Apr 3, 2020

Expected Behavior

An endpoint with a POST method is created, along with a unittest to test its behavior. I create two dummy files (you can include your own files) one with size 499 KB, one with size 500 KB. I pass both files to the endpoint in my unittest (by running python3 -m unittest example.py)

python3.6 -m venv venv
venv/bin/pip install flask
import unittest
from flask import Flask

app = Flask(__name__)

@app.route("/", methods=["POST"])
def hello():
  return "hello world"

# create dummy files
with open("./small.txt", "wb") as f:
    f.write(b"0" * 1024 * 499)

with open("./big.txt", "wb") as f:
    f.write(b"0" * 1024 * 500)

# test the endpoint
class ApiTest(unittest.TestCase):
  def test_all(self):
    data = (
      ("hello world", "./big.txt"), # warning
      ("hello world", "./small.txt"), # no warning
    )

    for expected, path in data:
      with open(path, "rb") as file, app.test_client() as cl:
        response = cl.post("/", data={"file":file}).get_data().decode("utf-8")
        
      self.assertEqual(expected, response)
venv/bin/python3.6 -m unittest example.py

Actual Behavior

POST-ing the bigger file (>=500 KB) produces a ResourceWarning
Console output:

/home/myname/.local/lib/python3.6/site-packages/flask/testing.py:244: ResourceWarning: unclosed file <_io.BufferedRandom name=4>
  top = _request_ctx_stack.top
.
----------------------------------------------------------------------
Ran 1 test in 0.005s

OK

Changing from f.write(b"0" * 1024 * 500) to f.write(b"0" * 1024 * 499) the output is

.
----------------------------------------------------------------------
Ran 1 test in 0.004s

OK

Environment

  • Python version: 3.6.9
  • Flask version: 1.1.1
  • Werkzeug version: 1.0.0
@davidism davidism transferred this issue from pallets/flask Apr 3, 2020
@northernSage
Copy link
Contributor

From werkzeug.test.EnvironBuilder:

data can be any of these values:

  • a str or bytes object: The object is converted into an
    :attr:input_stream, the :attr:content_length is set and you have to
    provide a :attr:content_type.
  • a dict or :class:MultiDict: The keys have to be strings. The values
    have to be either any of the following objects, or a list of any of the
    following objects:
    • a :class:file-like object: These are converted into
      :class:FileStorage objects automatically.
    • a tuple: The :meth:~FileMultiDict.add_file method is called
      with the key and the unpacked tuple items as positional
      arguments.
    • a str: The string is set as form data for the associated key.
  • a file-like object: The object content is loaded in memory and then
    handled like a regular str or a bytes.

It seems the warning only shows when data is passed in the form
data={'key': file}. Passing the same file as data=file, data={'file': file.read()}
or data=file.read() does not trigger the warning. Perhaps this has something to do with
the conversion/handling of the FileStorage wrapper? On a side note, when the same test is
run with Pytest, no warning is issued. That makes me wonder if this is a Werkzeug issue at all.

Python: 3.7.7
Flask: 1.1.2
Werkzeug: 1.0.1

@kx-chen
Copy link
Contributor

kx-chen commented Jun 22, 2020

Working on this. Will keep updated.

kx-chen added a commit to MLH-Fellowship/werkzeug that referenced this issue Jun 22, 2020
kx-chen added a commit to MLH-Fellowship/werkzeug that referenced this issue Jun 22, 2020
@kx-chen
Copy link
Contributor

kx-chen commented Jul 1, 2020

A few findings:

  1. The ResourceWarning is caused from the TemporaryFile in

    new_stream = TemporaryFile("wb+")
    new_stream.write(stream.getvalue())
    new_stream.write(string)

    There doesn't seem to be a clean way of dealing with this. The TemporaryFile still needs to be read from once passed into the environ dict. This means closing it along with the other files won't work.
    "wsgi.input": input_stream,

  2. Like @northernSage said, this only happens when using unittest. Pytest does not show this warning. Additionally, this is still happening on python 3.8.

Screen Shot 2020-07-01 at 1 42 44 PM

No issues using pytest.
Screen Shot 2020-07-01 at 1 44 15 PM

  1. The reason passing data through with data={'file': file.read()} (or data=file) doesn't cause the warning is because it is handled differently through BytesIO
    if isinstance(data, bytes):
    self.input_stream = BytesIO(data)
    if self.content_length is None:
    self.content_length = len(data)

@davidism thoughts on next steps?

@davidism davidism changed the title Sending 500 KB files or larger via POST using unittest causes warning Sending >=500 KB file with test client causes warning Jan 21, 2021
@davidism
Copy link
Member

davidism commented Feb 11, 2021

I can't reproduce this issue on Python 3.6+, Werkzeug 1.0.x or latest. I tried both the original example and a simplified one that only used Werkzeug.

Werkzeug example
import unittest

from werkzeug import Request
from werkzeug import Response
from werkzeug import Client


@Request.application
def app(request):
    return Response("Hello, World!")


with open("zero.txt", "wb") as f:
    f.write(b"0" * 1_000_000)


class APITest(unittest.TestCase):
    def test_file(self):
        c = Client(app)

        with open("zero.txt", "rb") as f:
            r = c.post(data={"file": f})

        assert r.get_data(as_text=True) == "Hello, World!"


if __name__ == "__main__":
    unittest.main()
python -Walways -m unittest example

As far as I can tell, the open files passed to data={} are closed when EnvironBuilder.close() is called, so that's not the issue. The file that's not being closed is BufferedRandom, which means it's the temp file created by stream_encode_multipart to be passed as wsgi.input_stream. Client.open could close the input stream after getting the response object, but the problem is that the response might be streaming, so closing the input stream might cause the view that's streaming output to fail, depending on what it's doing.

Now that the test client always returns TestResponse objects, there's a few options, but they still require user intervention. You can call response.request.input_stream.close() to close the input stream if you're getting this error. We could also make this easier by registering it with Response.call_on_close(), so you could call response.close() (which you sometimes have to do for other reasons anyway, such as when serving a file).

You can close the input stream in the current 1.0.x release, it's just a bit more complicated. You need to pass as_tuple=True when making requests, which will return environ, result. Or use warnings.filterwarnings to ignore the warning.

environ, result = client.post("/", data={"file": f})
assert result.get_data() == expect
environ["wsgi.input"].close()

@davidism
Copy link
Member

I'm not sure if I should make my proposed fix right now, since I can't reproduce the issue. I'll see if I can have someone else test it.

@davidism
Copy link
Member

davidism commented Feb 11, 2021

Never mind, I just tried it again and now I can reproduce it consistently. Weird. But if I install the latest development Werkzeug instead of 1.0.1, the issue goes away. But if I run with python -Walways -m unittest example.py, it shows up again. Not sure what caused it to start being ignored.

@davidism davidism reopened this Feb 11, 2021
@davidism davidism added this to the 2.0.0 milestone Feb 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants