-
Notifications
You must be signed in to change notification settings - Fork 387
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(recipe): No more memory leak once TreeCache was closed #524
fix(recipe): No more memory leak once TreeCache was closed #524
Conversation
57b3a22
to
4115dd4
Compare
😸 It is ready for review now. |
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.
Thanks for continuing to maintain this recipe. And I appreciate the detailed commit messages, very helpful.
When I see stuff like cb = None # GC: release last reference before idle
, I wonder why not just do del cb
?
Using del
makes it explicit that you're clearing the memory, so that the annotation # GC: release last reference before idle
becomes superfluous.
Given the attention to detail in your PR's, I suspect I'm missing something... Is there some reason that it's better to set it to None
?
kazoo/recipe/cache.py
Outdated
@@ -57,6 +58,11 @@ def start(self): | |||
After a cache started, all changes of subtree will be synchronized | |||
from the ZooKeeper server. Events will be fired for those activity. | |||
|
|||
Don't forget to call :meth:`close` if it was started and you don't | |||
need it anymore, or you will leak the memory of cached nodes, even you |
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.
typo: even you
--> even if you
(at least, that's what I think you mean)
kazoo/recipe/cache.py
Outdated
Don't forget to call :meth:`close` if it was started and you don't | ||
need it anymore, or you will leak the memory of cached nodes, even you | ||
give up all reference to the :class:`TreeCache` instance. (There are | ||
so many callback has been registered) |
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.
callbacks should be plural: callback has been
--> callbacks that have been
e73d157
to
c2aadd9
Compare
Hi @jeffwidman, Thanks for you review. The commits have been revised now. |
log.warning("Exception in worker greenlet") | ||
log.exception(exc) | ||
finally: | ||
del func # release before possible idle |
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.
Given the attention to detail in your PR's, I suspect I'm missing something... Is there some reason that it's better to set it to None?
I had set the func
to None
for preventing from NameError
if an exception was raised earlier than func
was assigned. But it is confusing and del func
is more explicit that it, I agree with you.
I use the nested try-except
like the implementation of SequentialThreadingHandler
now. The NameError
will never occur here.
ab3f49f
to
3426dfb
Compare
I will keep the branch up-to-date for latest master. The new pushed commits are just for rebasing. |
3426dfb
to
1cdeafc
Compare
1cdeafc
to
a4c566c
Compare
Hi @tonyseek, thanks for your PR. I will try to give it a review during the week. |
kazoo/tests/test_cache.py
Outdated
while not self.client.handler.completion_queue.empty(): | ||
self.client.handler.sleep_func(0.1) | ||
else: | ||
# Let hub of gevent/eventlet be in a short-time loop |
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 check here that the queue is empty?
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.
It seems that switching to loop hub via sleep_func
once is stable enough for waiting the async results to be finished when we are using eventlet and gevent. They will switch back only when IO event happened. But this conclusion doesn't not play well with threading handlers because the native threads have unpredictable schedule opportunity. That's why I wrote the special branch to wait completion_queue
for the threading handler.
It is possible to check completion_queue
for eventlet too, but not for gevent. The gevent uses native AsyncResult
of itself instead of the completion_queue
.
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.
kazoo/tests/test_cache.py
Outdated
continue | ||
except Exception: | ||
logger.warning( | ||
'The handler module %s is imported but could not be used', |
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.
Did you notice cases where this can happen?
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.
In my cases it only happen when the gevent installation is broken. I should delete it and make it fail fast.
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.
b9c7948
to
d07a9b0
Compare
kazoo/tests/test_cache.py
Outdated
from kazoo.exceptions import KazooException | ||
from kazoo.recipe.cache import TreeCache, TreeNode, TreeEvent | ||
|
||
|
||
class KazooTreeCacheTests(KazooTestCase): | ||
logger = logging.getLogger(__name__) |
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.
Seems like you can remove this object now since it is not used anymore
kazoo/tests/test_cache.py
Outdated
@@ -1,15 +1,48 @@ | |||
import gc | |||
import importlib | |||
import logging |
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 import is now useless
Thanks again for this great PR, it is impressive. I just added 2 small comments... I think we will be ready to merge this PR asap. |
d07a9b0
to
4a26c99
Compare
@StephenSorriaux My pleasure. And thank you for your review. The unused logger has been removed in 4a26c99 now. |
@tonyseek merged to master. Thanks again for your hard work. |
Great job pushing this through @StephenSorriaux / @tonyseek |
The current implementation of TreeCache recipe could not be closed completely because some unexpected references:
TreeNode
instancesThis pull request fixes those memory leak problems, and verifies that by using
objgraph
in test.We could guarantee all closed
TreeCache
will be eventually released now. (not immediately since the cycle reference betweenTreeNode
andTreeCache
must be resolved by mark-swap GC of Python)