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: prevent memory leak in _redlock_release of SimpleMemoryBackend #807

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

moshego189
Copy link
Contributor

@moshego189 moshego189 commented Jan 21, 2024

What do these changes do?

The changes refactor the _redlock_release method in the SimpleMemoryBackend class. Instead of directly removing items from the _cache dictionary using the pop method, the method now invokes the __delete method. This ensures that when an item is released from the cache, associated timer handles in the _handlers dictionary are also properly cancelled, thereby preventing potential memory leaks.

Are there changes in behavior for the user?

There are no direct changes in behavior for the end user. This update enhances the internal memory management of the cache system, contributing to improved stability and efficiency without altering the functional interface.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

…eMemoryBackend

This commit addresses a memory leak issue in the SimpleMemoryBackend class of our Python cache library. Previously, the _redlock_release method directly removed items from the _cache dictionary using the pop method. However, this approach did not account for the cleanup of associated timer handles in the _handlers dictionary.

The fix refactors the _redlock_release method to call the __delete method instead of directly popping the key from the _cache. The __delete method handles both the removal of the cache item and the cancellation of any associated timer handle in the _handlers dictionary. This change ensures that when a cache item is released using the _redlock_release method, all related resources are properly cleaned up, preventing potential memory leaks.

This change contributes to the overall stability and efficiency of the cache mechanism by ensuring resources are correctly managed and released.
@Dreamsorcerer
Copy link
Member

Could we have a test that validates the handler is no longer present after release (and ideally that the callback has been cancelled)?

@moshego189
Copy link
Contributor Author

Could we have a test that validates the handler is no longer present after release (and ideally that the callback has been cancelled)?

Done!

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fe28c47) 99.76% compared to head (20a2347) 99.76%.
Report is 9 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #807   +/-   ##
=======================================
  Coverage   99.76%   99.76%           
=======================================
  Files          36       36           
  Lines        3794     3797    +3     
=======================================
+ Hits         3785     3788    +3     
  Misses          9        9           
Flag Coverage Δ
unit 99.76% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
aiocache/backends/memory.py 100.00% <100.00%> (ø)
tests/ut/backends/test_memory.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c57a3e9...20a2347. Read the comment docs.

@Dreamsorcerer Dreamsorcerer merged commit f4dfe7c into aio-libs:master Jan 22, 2024
15 checks passed
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