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

Close file handles of Popen #4

Merged
merged 5 commits into from
Apr 30, 2021
Merged

Close file handles of Popen #4

merged 5 commits into from
Apr 30, 2021

Conversation

3j14
Copy link
Contributor

@3j14 3j14 commented Apr 30, 2021

First of all, thanks for this awesome project. We recently added it to our WeasyPrint pipeline and enjoying it so far!

However, python started complaining that certain files/processes have been left unclosed:

/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/subprocess.py:1052: ResourceWarning: subprocess 4157 is still running
  _warn("subprocess %s is still running" % self.pid,
ResourceWarning: Enable tracemalloc to get the object allocation traceback

/path/to/project/venv/lib/python3.9/site-packages/markdown_katex/wrapper.py:285: ResourceWarning: unclosed file <_io.BufferedReader name=6>
  help_text = _get_cmd_help_text()
ResourceWarning: Enable tracemalloc to get the object allocation traceback

To fix this, I closed all file descriptors simply by wrapping the calls to Popen with a context manager as specified in the python documentation.

See: https://docs.python.org/3/library/subprocess.html#popen-constructor

Python started complaining that certain files have been left
unclosed. To fix this, always make sure to close all file
descriptors simply by wrapping the calls to Popen with a
context manager as specified in the python documentation

See: https://docs.python.org/3/library/subprocess.html#popen-constructor
@3j14
Copy link
Contributor Author

3j14 commented Apr 30, 2021

One note about Python 2 support. I tried testing it against Python 2.7 as it seems like Popen wasn't a context manager in Python 2.7. However, I was unable to run any test because Python 2.7 kept complaining about invalid syntax because of the mypy typing hints. I guess Python 2.7 is no longer supported? I would have suggested dropping support for Python 2.7 anyway so that's a good thing

@mbarkhau
Copy link
Owner

Thanks for your patch. Python 2.7 is still supported, don't worry about testing for it, the CI should take care of it.

It does mean I'll have to ask for the try..finally: proc.close() approach instead of a context manager.

@3j14
Copy link
Contributor Author

3j14 commented Apr 30, 2021

Sure, I can rewrite the code to use try..finally:... Unfortunately, Popen does not have a Popen.close method. Instead, we have to close Popen.stdout and Popen.stderr, etc.

As support for python 2.7 should be maintained, a different
approach has to be used. `Popen` does not implement a context
manager (i.e. no __enter__ and __exit__ methods). The
alternative is a `try: ... finally: ...` block. Note that I
decided to put `Popen` inside this block. Thus, I had to set
proc to be `None` as the `Popen` constructor itself could also
raise an exception. Whether this approach is actually reasonable
is debatable.
@3j14
Copy link
Contributor Author

3j14 commented Apr 30, 2021

Python 2.7 should work now 😄
I was unsure about whether or not to put proc inside the try block. I now opted for putting it inside it, but it should not make much of a difference.

@mbarkhau mbarkhau changed the title Wrap Popen with context manager Close file handles of Popen Apr 30, 2021
@3j14
Copy link
Contributor Author

3j14 commented Apr 30, 2021

What irony... Now pylint complains that we should use a with statement:

src/markdown_katex/wrapper.py:207:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
15
src/markdown_katex/wrapper.py:296:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)
16

Copy link
Contributor Author

@3j14 3j14 left a comment

Choose a reason for hiding this comment

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

You would also have to put this comment in the _get_cmd_help_text function. I added the same try: ... finally block there as well

@mbarkhau mbarkhau merged commit d6143ed into mbarkhau:master Apr 30, 2021
@3j14
Copy link
Contributor Author

3j14 commented Apr 30, 2021

Thank you for merging this so quickly 👍🏽

@3j14 3j14 deleted the fix-unclosed-file branch April 30, 2021 15:44
@mbarkhau
Copy link
Owner

Published with v202104.1030

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

Successfully merging this pull request may close these issues.

2 participants