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

Fix FreeType SDL_RWops leak #577

Merged
merged 3 commits into from Oct 23, 2018

Conversation

Projects
None yet
2 participants
@dlon
Copy link
Member

dlon commented Oct 23, 2018

Fixes #572. Not sure whether the first or second commit is cleaner. Added 2nd as the user may not expect pygame to close a stream that they passed to it? At least in CPython, files normally close when the ref count hits 0, so either works.

@illume

This comment has been minimized.

Copy link
Member

illume commented Oct 23, 2018

Is adding a unit test for this possible?

I guess your example in the issue could be used? It could check to see if the file is closed after the font destructor is called?

Something like this maybe?

with open('file.ttf', 'rb') as f:
    font = Font(f)
    del font
    assert f.closed
@illume

This comment has been minimized.

Copy link
Member

illume commented Oct 23, 2018

I guess your second commit would be better? Because it cleans up all the resources, whereas closing the file will just make sure the file is closed but will leave the objects around?

@dlon

This comment has been minimized.

Copy link
Member Author

dlon commented Oct 23, 2018

The first does clean up everything and close the handle (SDL will call ..._close() in rwobject.c, I believe). The second lets Python close the file instead, which I think makes sense since the object is created in Python

But this test might fail with PyPy

@dlon dlon force-pushed the dlon:freetype-rwops-leak branch from 8b85f6e to bd7db5e Oct 23, 2018

@dlon dlon force-pushed the dlon:freetype-rwops-leak branch from bd7db5e to f460ddc Oct 23, 2018

@illume

This comment has been minimized.

Copy link
Member

illume commented Oct 23, 2018

@dlon thanks. So, do you think this is ok to merge now?

I guess that test will fail with pypy.

@dlon

This comment has been minimized.

Copy link
Member Author

dlon commented Oct 23, 2018

Yes

@illume illume merged commit 42778d0 into pygame:master Oct 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dlon dlon deleted the dlon:freetype-rwops-leak branch Nov 7, 2018

@dlon dlon referenced this pull request Mar 23, 2019

Closed

1.9.5 release notes. #561

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.