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

BUG: Fix memory leak in CRS.list_authority #1178

Merged
merged 1 commit into from Nov 13, 2022

Conversation

skogler
Copy link
Contributor

@skogler skogler commented Nov 11, 2022

List elements were not properly disposed of when iterating the results of proj_identify. This fixes this and simplifies the cleanup logic.

  • Tests added (not sure how to test this automatically)
  • Fully documented, including history.rst for all changes and api/*.rst for new API

You can reproduce this issue with the following snippet (look at the memory usage):

from pyproj import CRS

for i in range(10000):
    CRS.from_epsg(3857).to_epsg()

@skogler skogler force-pushed the fix-memory-leak-in-to-authority branch from 2be4e1d to fd1c090 Compare November 11, 2022 12:50
pyproj/_crs.pyx Outdated Show resolved Hide resolved
pyproj/_crs.pyx Outdated Show resolved Hide resolved
pyproj/_crs.pyx Outdated Show resolved Hide resolved
@skogler skogler force-pushed the fix-memory-leak-in-to-authority branch from fd1c090 to c4b9ea2 Compare November 11, 2022 14:58
@snowman2 snowman2 changed the title Fix memory leak in CRS.to_authority/CRS.list_authority BUG: Fix memory leak in CRS.to_authority/CRS.list_authority Nov 11, 2022
@snowman2 snowman2 added the bug label Nov 11, 2022
@snowman2 snowman2 added this to the 3.4.1 milestone Nov 11, 2022
@snowman2
Copy link
Member

Thanks @skogler 👍

@skogler
Copy link
Contributor Author

skogler commented Nov 11, 2022

Happy to help, thanks for the free software :)

@snowman2 snowman2 changed the title BUG: Fix memory leak in CRS.to_authority/CRS.list_authority BUG: Fix memory leak in CRS.list_authority Nov 11, 2022
@skogler
Copy link
Contributor Author

skogler commented Nov 11, 2022

@snowman2 I searched for other usages of proj_list_get and found one here: https://github.com/pyproj4/pyproj/blob/main/pyproj/_transformer.pyx#L204

What do you think about that one? It seems to me that https://github.com/pyproj4/pyproj/blob/main/pyproj/_transformer.pyx#L116 should probably call proj_destroy on all the child transformer objects?

@snowman2
Copy link
Member

What do you think about that one?

I believe that should be fine. The _Transformer or CoordinateOperation object handles de-allocation of the objects.

@skogler
Copy link
Contributor Author

skogler commented Nov 11, 2022

Ah, I see, because Base is doing, it, you are right!

@snowman2
Copy link
Member

Mind rebasing from main? I added a fix (#1179) that should allow the CICD tests to run to completion now.

List elements were not properly disposed of when iterating the results of proj_identify. This fixes this and simplifies the cleanup logic.
@skogler skogler force-pushed the fix-memory-leak-in-to-authority branch from c4b9ea2 to 01b793b Compare November 12, 2022 22:28
@skogler
Copy link
Contributor Author

skogler commented Nov 12, 2022

Sure, done.

@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Merging #1178 (07bf470) into main (07bf470) will not change coverage.
The diff coverage is n/a.

❗ Current head 07bf470 differs from pull request most recent head 01b793b. Consider uploading reports for the commit 01b793b to get more accurate results

@@           Coverage Diff           @@
##             main    #1178   +/-   ##
=======================================
  Coverage   96.19%   96.19%           
=======================================
  Files          18       18           
  Lines        1707     1707           
=======================================
  Hits         1642     1642           
  Misses         65       65           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@snowman2 snowman2 merged commit 74cd9e7 into pyproj4:main Nov 13, 2022
@snowman2
Copy link
Member

Thanks @skogler 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants