Skip to content

Close file objects, requests.Responses - #1147

Merged
joshuagl merged 3 commits into
theupdateframework:developfrom
jku:close-all-the-things
Sep 22, 2020
Merged

Close file objects, requests.Responses#1147
joshuagl merged 3 commits into
theupdateframework:developfrom
jku:close-all-the-things

Conversation

@jku

@jkujku commented Sep 17, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Fixes issue #1099:

maybe also #1123 (but I've only gone through the code exercised by test_updater.py and test_download.py)

Description of the changes being introduced by the pull request:

Try to close all file objects and requests.Responses that are left open during test_updater.py and test_download.py execution.

Please verify and check that the pull request fulfills the following
requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@jku

jku commented Sep 17, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

These are good changes I think, but I guess there's also a case to be made for silencing ResourceWarnings about these things in the testsuite:

  • the same warnings don't appear in production
  • as far as I understand, cpython can handle automatic closing just fine. The issues would only appear on other interpreters (pypy?)

I'm not in favor of silencing: just mentioning it

@jkujku mentioned this pull request Sep 18, 2020
Merged
3 tasks
MVrachev
MVrachev reviewed Sep 18, 2020
View reviewed changes
Comment threadtuf/download.py Outdated
MVrachev
MVrachev reviewed Sep 18, 2020
View reviewed changes
Comment threadtuf/download.py Outdated
MVrachev
MVrachev reviewed Sep 18, 2020
View reviewed changes

@MVrachevMVrachev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great changes!
I tried them locally and I don't see Resource warnings when running tests_download.py and tests_updater.py.

Can you increase the scope of this pr and fix the other instances in the other tests files Resource warnings (as mentioned in #1123) or that will be much of work right now?

@jku

jku commented Sep 18, 2020

Copy link
Copy Markdown
MemberAuthor

Can you increase the scope of this pr and fix the other instances in the other tests files Resource warnings (as mentioned in #1123) or that will be much of work right now?

This is the scope for now I think. Finding/fixing the trickier ones can take quite a bit of time

joshuagl
joshuagl approved these changes Sep 22, 2020
View reviewed changes

@joshuagljoshuagl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for these changes, it's nice to see the number of warnings produced by the tests being reduced.

Comment threadtuf/client/updater.py Outdated
Comment threadtuf/download.py
Jussi Kukkonen added 3 commits September 22, 2020 14:22
Jussi Kukkonen
Avoid leaving unclosed file objects
b5a3c70
* move code to only create objects after potential raises
* Use 'with' when possible
* close manually if those did not help
Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
Jussi Kukkonen
Download: Don't leak requests.Responses
26f4585
Use 'with' to avoid leaking responses when response.raise_for_status()
is used.
Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
Jussi Kukkonen
Download: Fix documentation
fb9d8e7
* Remove dead urls: trust reader to find Requests documentation on
their own
* Mention that "stream" means that downloading the body is deferred
Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku
jkuforce-pushed the close-all-the-things branch from d79d3d9 to fb9d8e7CompareSeptember 22, 2020 11:29
@jku

jku commented Sep 22, 2020

Copy link
Copy Markdown
MemberAuthor

The last change only updated the code comment in the first commit based on Joshuas comment. No other changes.

@joshuagl
joshuagl merged commit 687506f into theupdateframework:developSep 22, 2020
@joshuagljoshuagl mentioned this pull request Sep 23, 2020
Closed
@jku
jku deleted the close-all-the-things branch December 30, 2024 09:07
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshuagljoshuagljoshuagl approved these changes
+1 more reviewer
@MVrachevMVrachevMVrachev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jku@joshuagl@MVrachev