Fixed several reference cycles to prevent memory leaks. #2967
Conversation
…test for detect memory leaks after application closing.
|
|
||
| def get_gc_count(self): | ||
| last_collected = 0 | ||
| while True: |
There was a problem hiding this comment.
Is this actually guaranteed to converge? The possibility here for an infinite while loop concerns me.
There was a problem hiding this comment.
I don't know :) May be it possible in some cases with threads that works simultaneously with this test. Then GC will collects different count of objects in each step of cycle.
pyramid/config/views.py
Outdated
| @@ -933,7 +933,7 @@ def register(permission=permission, renderer=renderer): | |||
| if not exception_only and isexc: | |||
| derived_view = runtime_exc_view(derived_view, derived_exc_view) | |||
|
|
|||
| derived_view.__discriminator__ = lambda *arg: discriminator | |||
| derived_view.__discriminator__ = lambda *arg: view_intr.discriminator | |||
There was a problem hiding this comment.
I'm not sure I see the cycle here, can you explain?
There was a problem hiding this comment.
This reference cycle prevents the removal of instance of Сonfigurator after a WSGI application has created.
If skip some steps then cycle looks like that:
discriminator (always instance of Deferred in this place) => discrim_func => Configurator (as self in closure) => Registry => Introspector => default_exceptionrespone_view => __discrimantor__ => lambda => discriminator.
My fix removes the instance of Deferred from closure. Instead it, closure contains now instance of Introspectable with undeferred value of discriminator.
For details, see the attached image with graph of backrefs for instance of Configurator after running this code:
app = Configurator().make_wsgi_app()
gc.collect()
There was a problem hiding this comment.
Would this be solved if we fixed the Deferred to release the closure when it is resolved? Imagine self.func = None in
Line 278 in 7fb8893
There was a problem hiding this comment.
Yes, it was a first solution that I have tried when investigate this reference cycle. It works too.
There was a problem hiding this comment.
I think I'd prefer that solution because it feels less error prone. The current fix is quite subtle.
pyramid/tweens.py
Outdated
| @@ -42,7 +42,7 @@ def excview_tween(request): | |||
| provides = providedBy(exc) | |||
| try: | |||
| response = _call_view( | |||
| registry, | |||
| request.registry, | |||
There was a problem hiding this comment.
Again, I guess the issue here is that the registry eventually retains the excview_tween and the excview_tween retains the registry via the closure, but this is all cleaned up via GC in general and likely occurs in many other parts of code.
What practical problem is this solving that I should accept this change? Is the submitted integration test going to prevent me from writing this code again later?
There was a problem hiding this comment.
Sorry but just to clarify I think the integration test and the weakref fixes on the WeakOrderedSet are good. I'm just nitpicking these other two changes.
There was a problem hiding this comment.
Hmm... Yes, I agree with you - this closure has not matter and do not prevents GC to resolve a cycle "registry-tween-registry" .
I could remove this change, if you think that it is absolutely useless.
|
Thank you @Cykooz. I'm leaving this open for now until we backport these fixes to 1.7 and 1.8 as well. |
…test for detect memory leaks after application closing. Backport Pylons#2967 to the 1.7-branch
…test for detect memory leaks after application closing. Backport Pylons#2967 to 1.8-branch.
…test for detect memory leaks after application closing. Backport Pylons#2967 to the 1.7-branch
…test for detect memory leaks after application closing. Backport Pylons#2967 to 1.8-branch.
…test for detect memory leaks after application closing. Backport Pylons#2967 to the 1.7-branch
I have corrected three places in the code where appear reference cycles due to closure.
Also I have added a simple test for detect memory leaks after application closing.