Use with statements for handling files #34

rickheil · 2021-07-26T21:20:39Z

Fix #31 - close file handles when we are dealing with local files.

bryanheinz

Looks good to me!

disconnect3d · 2021-07-27T12:17:07Z

Hey, this PR is not correct and it breaks things even more :(.

The with statement will close the resource on its exit, so when we do:

            with open(binary, 'rb') as f:
                files['binary'] = f
        return self._post_data(self.url, data, files)

The binary file is opened, its opened file handle is assigned to files['binary'] and then when we go out of scope the file is closed. The closed file is then attempted to be read by the requests library somewhere deeper in the self._post_data function.

Here is an example that shows this issue, ran in an ipython interactive shell:

$ ipython
Python 3.8.2 (default, Apr  8 2021, 23:19:18)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.20.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import requests

In [2]: !echo example_content>data

In [3]: !cat data
example_content

In [4]: with open('data', 'rb') as f: files = {'upload_file': f}

In [5]: files
Out[5]: {'upload_file': <_io.BufferedReader name='data'>}

In [6]: r = requests.post('http://localhost/', files=files)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-a9d9f29c5118> in <module>
----> 1 r = requests.post('http://localhost/', files=files)

~/Library/Python/3.8/lib/python/site-packages/requests/api.py in post(url, data, json, **kwargs)
    117     """
    118
--> 119     return request('post', url, data=data, json=json, **kwargs)
    120
    121

~/Library/Python/3.8/lib/python/site-packages/requests/api.py in request(method, url, **kwargs)
     59     # cases, and look like a memory leak in others.
     60     with sessions.Session() as session:
---> 61         return session.request(method=method, url=url, **kwargs)
     62
     63

~/Library/Python/3.8/lib/python/site-packages/requests/sessions.py in request(self, method, url, params, data, headers, cookies, files, auth, timeout, allow_redirects, proxies, hooks, stream, verify, cert, json)
    526             hooks=hooks,
    527         )
--> 528         prep = self.prepare_request(req)
    529
    530         proxies = proxies or {}

~/Library/Python/3.8/lib/python/site-packages/requests/sessions.py in prepare_request(self, request)
    454
    455         p = PreparedRequest()
--> 456         p.prepare(
    457             method=request.method.upper(),
    458             url=request.url,

~/Library/Python/3.8/lib/python/site-packages/requests/models.py in prepare(self, method, url, headers, files, data, params, auth, cookies, hooks, json)
    317         self.prepare_headers(headers)
    318         self.prepare_cookies(cookies)
--> 319         self.prepare_body(data, files, json)
    320         self.prepare_auth(auth, url)
    321

~/Library/Python/3.8/lib/python/site-packages/requests/models.py in prepare_body(self, data, files, json)
    505             # Multi-part file uploads.
    506             if files:
--> 507                 (body, content_type) = self._encode_files(files, data)
    508             else:
    509                 if data:

~/Library/Python/3.8/lib/python/site-packages/requests/models.py in _encode_files(files, data)
    157                 fdata = fp
    158             elif hasattr(fp, 'read'):
--> 159                 fdata = fp.read()
    160             elif fp is None:
    161                 continue

ValueError: read of closed file

So TL;DR: the post/get/etc call needs to be within the with statement, which is not that trivial change when we open file only under a given condition. A simple solution could be to have two return self._post/get/etc in both cases - when we conditionally open the file and when we don't.

A more robust solution could be to write our own context manager which would do the open/close of the file if its needed (conditionally included in the context manager?).

rickheil added 2 commits July 26, 2021 17:15

31 - updates open statements to use with to get explicit close

3133a65

move file open down a bit for better readability

4d2e406

rickheil requested a review from bryanheinz as a code owner July 26, 2021 21:20

bryanheinz approved these changes Jul 26, 2021

View reviewed changes

rickheil merged commit 6ae0ebd into main Jul 27, 2021

rickheil mentioned this pull request Jul 27, 2021

Revert "Use with statements for handling files" #35

Merged

lucasjhall mentioned this pull request Jul 27, 2021

v3.0.6 #36

Merged

rickheil mentioned this pull request Dec 7, 2023

Files are opened, but not closed explicitly #31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use with statements for handling files #34

Use with statements for handling files #34

rickheil commented Jul 26, 2021

bryanheinz left a comment

disconnect3d commented Jul 27, 2021 •

edited

Loading

Use with statements for handling files #34

Use with statements for handling files #34

Conversation

rickheil commented Jul 26, 2021

bryanheinz left a comment

Choose a reason for hiding this comment

disconnect3d commented Jul 27, 2021 • edited Loading

disconnect3d commented Jul 27, 2021 •

edited

Loading