-
Notifications
You must be signed in to change notification settings - Fork 53
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 memory leak on exception caching #594
Conversation
@pytest.mark.skipif( | ||
reason="Memory leak is not fixed for PyPy3.9", |
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.
This suggests that it should be fixed at some point, in which case we probably want an xfail instead of a skip.
@@ -222,6 +222,7 @@ async def __call__(self, /, *fn_args: Any, **fn_kwargs: Any) -> _R: | |||
if self.__maxsize is not None and len(self.__cache) > self.__maxsize: | |||
dropped_key, cache_item = self.__cache.popitem(last=False) | |||
cache_item.cancel() | |||
del cache_item |
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.
I'm not clear how this relates to exceptions specifically, but here's my understanding of the issue, which should be documented:
del cache_item | |
# While waiting for future below, we want dropped cache_item to be garbage | |
# collected. Without the del, it is possible to end up with a chain of cache_item | |
# references stopping them from being garbage collected at all. | |
del cache_item |
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.
Should we be doing dropped_key too?
I think #595 seems more sensible. |
What do these changes do?
When exception is "evicted" from cache, if following call raises an exception, both of them will be kept in memory introducing a memory leak.
Are there changes in behavior for the user?
No
Related issue number
199 is probably related
Checklist