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: resolves #1443 - closes image when context cleanup #1623

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

RaphaelVRossi
Copy link
Member

@RaphaelVRossi RaphaelVRossi commented Nov 17, 2023

Resolves #1443

After Thumbor finish the request, a cleanup method is called. But the Pillow Image pointer remained alive.

Testing locally, Thumbor was able to clean remaining memory.

Before changes:

669    491.5 MiB      0.0 MiB           1           self._cleanup()
669    496.7 MiB      0.0 MiB           1           self._cleanup()

After changes:

669    204.6 MiB   -239.7 MiB           1           self._cleanup()
669    263.7 MiB   -228.0 MiB           1           self._cleanup()

memory-profile was used to calculate memory allocation and deallocation.

@coveralls
Copy link

coveralls commented Nov 17, 2023

Pull Request Test Coverage Report for Build 9430683271

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 89.008%

Totals Coverage Status
Change from base Build 9374117208: 0.01%
Covered Lines: 3895
Relevant Lines: 4376

💛 - Coveralls

@RaphaelVRossi
Copy link
Member Author

@scorphus @heynemann What do you think?

Copy link

sonarcloud bot commented Dec 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Comment on lines -680 to -701
self.context.filters_factory = None
self.context.metrics = None
self.context.thread_pool = None
self.context.transformer = None
Copy link
Member

Choose a reason for hiding this comment

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

These lines are being deleted but no new code deals with those attributes. Should this be of any concern?

Comment on lines +78 to +82
if hasattr(self, "modules") and self.modules:
self.modules.cleanup()

self.thread_pool.cleanup()
if hasattr(self, "request") and self.request.engine:
self.request.engine.cleanup()
Copy link
Member

@scorphus scorphus Dec 8, 2023

Choose a reason for hiding this comment

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

Can request ever be None or something else other than expected in a way that line 81 raises something like "AttributeError: 'NoneType' object has no attribute 'engine'"?

self.cleanup()

def cleanup(self):
if hasattr(self, "modules") and self.modules:
Copy link
Member

Choose a reason for hiding this comment

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

This line — and probably line 81 as well, more or less in the same way — could read like the following:

Suggested change
if hasattr(self, "modules") and self.modules:
if getattr(self, "modules", None):

Copy link

github-actions bot commented Feb 7, 2024

This PR is stale because it has been open 60 days with no activity. Remove the stale label or add a comment, or this PR will be closed in 30 days. You can always re-open if you feel this is something we should still keep working on. Tag @heynemann for more information.

@github-actions github-actions bot added the Stale label Feb 7, 2024
@marcelometal marcelometal removed the Stale label Feb 7, 2024
Copy link

sonarcloud bot commented Feb 7, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

github-actions bot commented Apr 7, 2024

This PR is stale because it has been open 60 days with no activity. Remove the stale label or add a comment, or this PR will be closed in 30 days. You can always re-open if you feel this is something we should still keep working on. Tag @heynemann for more information.

Copy link

sonarcloud bot commented Apr 8, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

github-actions bot commented Jun 7, 2024

This PR is stale because it has been open 60 days with no activity. Remove the stale label or add a comment, or this PR will be closed in 30 days. You can always re-open if you feel this is something we should still keep working on. Tag @heynemann for more information.

@github-actions github-actions bot added the Stale label Jun 7, 2024
@marcelometal marcelometal removed the Stale label Jun 8, 2024
Copy link

sonarcloud bot commented Jun 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

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.

Resizing images causes memory leaks
4 participants