-
Notifications
You must be signed in to change notification settings - Fork 140
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 handle in preloadFont #131
Conversation
Thanks for the fix -- the CI has failed, it looks like it broke on python 3.9; are you able to investigate/fix that? |
break | ||
else: | ||
for location in ("./", SHARED_DIRECTORY): | ||
full_name = os.path.join(location, fn) | ||
if os.path.isfile(full_name): | ||
f = open(full_name, 'rb') | ||
font_path = pathlib.Path(full_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI should now succeed.
The fix was replacing the string typed font_path
with pathlib.Path
objects . Line 141 already uses a Path
object, which knows how to open files inside a Python .egg
(where the test is taken fonts from), so we only needed to convert the string path in line 149 to a Path
object so we can use its .open
method in line 154.
Commit 785f90a introduced a regression by not using a context manager when opening the file, which eventually lead to a resource leak because the file handle 'f' never gets closed. This is fixed by passing the font path, instead of the open file handle, and by opening the handle using a 'with' context manager. The issue can be reproduced by running pytest with the '-W error' flag for the following minimal example: def test_file_gets_closed(): Figlet().renderText("") This will succeed now, but fails for v1.0.1.
I took the opportunity to squash my fixup commit 6d408fc, because the CI succeeded. |
I can't believe we missed that in my original check-in... This looks good to me. Perhaps we need an extra test to catch these sorts of issues in the future, though? What do you think @pwaller ? |
Yes, a test would be nice, but I'm not going to hold the fix up to wait for the test. If someone wants to implement a test for leaked file descriptors please do so, I'm afraid I don't have the time to do that myself. |
Released in 1.0.2. Thanks. |
Commit 785f90a introduced a regression by not using a context manager when opening the file, which eventually lead to a resource leak because the file handle
f
never gets closed.This is fixed by passing the font path, instead of the open file handle, and by opening the handle using a
with
context manager.The issue can be reproduced by running pytest with the
-W error
flag for the following minimal example:This will succeed now, but fails for v1.0.1.