Issue 408 fix, Memory leak was generated from Event #524

kashirin-alex · 2018-09-12T00:14:26Z

#408, fix for MemLeak on event exceptions …
(good with PyPy, while issues with Python 2/3)

changes event.py:

self._result and self._exc should not be class's static attributes (the Values were passing between the different Event instances)
self._exc should not store exception trace, instead if exception occurred self._exc=True (references kept them self, causing GC not to collect, Event has _exc and _exc has trace frames object in touched)
self._exc do not need to store exception trace, the same exception is called with the schedule_call_global for self._waiters timers
a wait on a triggered event is returning _results (without throw) None is exception/timeout

changes greenthread.py: (adjustments)

use of super for init inherited class

#408, Test for fix for MemLeak on event exceptions …

added test_no_mem_leaks to event_test.py
added tox.ini dependencies objgraph==3.4.0

changes event.py: - self._result and self._exc should not be class's static attributes - self._exc should not store exception trace, instead if exception occurred True - self._exc do not need to store exception trace, the same exception is called with the schedule_call_global for self._waiters timers - no need to throw self._exc on wait, it is thrown on the switch changes greenthread.py: (adjustments) - use of super for init inherited class - main method, try send, except send_exception and raise, and finally _resolve_links Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

added test_no_mem_leaks to event_test.py added tox.ini dependencies objgraph==3.4.0 Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

changes greenthread.py: (adjustments) - use of super for init inherited class Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

changes event.py: self._result and self._exc should not be class's static attributes (the Values were passing between the different Event instances) self._exc is set back to None on last waiter has it's results (references kept them self, causing GC not to collect, Event has _exc and _exc has trace frames object in touched) Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

- added test_no_mem_leaks to event_test.py - added tox.ini dependencies objgraph==3.4.0 Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

changes greenthread.py: (adjustments) - use of super for init inherited class - main method, _resolve_links finally Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

changes event.py: - self._result and self._exc should not be class's static attributes Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

…into 408-event-memleak Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com> # Conflicts: # eventlet/event.py # eventlet/greenthread.py # tests/event_test.py

changes event.py: - self._exc should not store exception trace, instead if exception occurred self._exc=True (references kept them self, causing GC not to collect, Event has _exc and _exc has trace frames object in touched) - self._exc do not need to store exception trace, the same exception is called with the schedule_call_global for self._waiters timers - a wait on a triggered event is returning _results (without throw) None is exception/timeout Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

changes event.py: - self._exc should not store exception trace, instead if exception occurred self._exc=True (references kept them self, causing GC not to collect, Event has _exc and _exc has trace frames object in touched) - self._exc do not need to store exception trace, the same exception is called with the schedule_call_global for self._waiters timers - a wait on a triggered event with sellf._exc will throw current (GreenletExit) Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

changes event.py: - self._exc should not store exception trace, instead if exception occurred self._exc=True (references kept them self, causing GC not to collect, Event has _exc and _exc has trace frames object in touched) Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

kashirin-alex · 2018-09-12T02:39:34Z

Sorry, Unable to find the Fix.
big issues are seen without the throw for a wait() with a results been an exception.

Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

…r test use Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

jstasiak

It's redundant to list the changes being made in commit messages.

Instead an explanation of why things are being done is needed. After this pull request is merged the repository needs to contain a helpful commit message explaining that.

Additionally, if you don't have a solution ready please consider testing it locally as creating a bunch of commits that fix each other floods inboxes of people watching the repository.

jstasiak · 2018-09-12T09:59:46Z

eventlet/event.py


    def __init__(self):
+        self._result = None
+        self._exc = None


This seems unrelated to the issue at hand.

At this point it is the only thing that can cause a one not garbage collected object.

jstasiak · 2018-09-12T10:00:01Z

eventlet/greenthread.py

@@ -167,7 +167,7 @@ class GreenThread(greenlet.greenlet):
    """

    def __init__(self, parent):
-        greenlet.greenlet.__init__(self, self.main, parent)
+        super(GreenThread, self).__init__(self.main, parent)


Also seems unrelated to the issue at hand

jstasiak · 2018-09-12T10:00:45Z

tests/event_test.py

+
+    import objgraph
+    import gc
+    import sys


Those can be imported at the module level just fine, why import here?

jstasiak · 2018-09-12T10:01:33Z

tests/event_test.py

+    while gc.collect():
+        pass
+
+    eventlet.hubs.get_hub().set_timer_exceptions(True)


We have to make sure this is executed even if this test fails earlier on.

jstasiak · 2018-09-12T10:02:58Z

tox.ini

@@ -67,6 +67,7 @@ deps =
    py{27,34}-{selects,poll,epolls}: pyopenssl==17.3.0
    setuptools==38.5.1
    {selects,poll,epolls}: pyzmq==17.0.0
+    objgraph==3.4.0


This is an unnecessarily heavy dependency for what's being done here, we can count the amount of objects of a specific type just fine without it.

Will be great to know which method can replace the objgraph.count('eventlet.greenthread.GreenThread')

>>> import gc >>> import objgraph >>> >>> class X: 2 pass >>> >>> objgraph.count('X') 0 >>> sum(1 for o in gc.get_objects() if isinstance(o, X)) 0 >>> x = X() >>> objgraph.count('X') 1 >>> sum(1 for o in gc.get_objects() if isinstance(o, X)) 1

OK, thanks,
I'll keep it updated with a solution to overcome/change the use of self._exc (if found of-course)

it might should be a method of the tests/init.py, and it has already gc imported

def check_ref_count(Instance, expected=0): c = sum(1 for o in gc.get_objects() if isinstance(o, Instance)) assert c == expected, \ "%s ref expected: %d counted: %d " % (Instance, expected, c)

or it can be better with a with statement

def check_ref_count(Instance, expected=0): try: pre_c = sum(1 for o in gc.get_objects() if isinstance(o, Instance)) finally: c = sum(1 for o in gc.get_objects() if isinstance(o, Instance)) assert c-pre_c == expected, \ "%s ref pre-counted: %d expected: %d \ counted: %d " % (Instance, pre_c, expected, c)

and work with:

with check_ref_count(Instance): #code

tests/event_test.py

kashirin-alex · 2018-09-12T10:39:18Z

Yes, the follow-up commits came out of nowhere, as there was more to the issue and initial checks were not sufficient although no issues have been seen for a while now (different type of occurrences/usages)
At this point, I don't yet know how to fix this one:

Instance need to preserve state, while it would include instances that need to be deleted/collected.
One thing would change the issue, it is wait to always return results (None for exception), and a call on wait after send been issued, it is equivalent to the result with exception that is None.

kashirin-alex added 11 commits September 12, 2018 02:55

eventlet#408, Test for fix for MemLeak on event exceptions

53f00cb

added test_no_mem_leaks to event_test.py added tox.ini dependencies objgraph==3.4.0 Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

eventlet#408, fix for MemLeak on event exceptions

4e293eb

changes greenthread.py: (adjustments) - use of super for init inherited class Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

Test for fix for MemLeak on event exceptions …

99350ca

- added test_no_mem_leaks to event_test.py - added tox.ini dependencies objgraph==3.4.0 Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

eventlet#408, fix for MemLeak on event exceptions

9dce8c2

changes greenthread.py: (adjustments) - use of super for init inherited class - main method, _resolve_links finally Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

eventlet#408, fix for MemLeak on event exceptions

454a897

changes event.py: - self._result and self._exc should not be class's static attributes Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

Merge branch 'issue-408' of https://github.com/kashirin-alex/eventlet …

0291f64

…into 408-event-memleak Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com> # Conflicts: # eventlet/event.py # eventlet/greenthread.py # tests/event_test.py

kashirin-alex added 7 commits September 12, 2018 06:42

event_test, clear threads not del

1d4cc2a

Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

event_test, version by sys.version_info.major and count obj refs prio…

98e07fd

…r test use Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

event_test, version by sys.version_info.major and count obj refs prio…

e0c12b6

…r test use Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

event_test, version by sys.version_info.major and count obj refs prio…

a2c8e6d

…r test use Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

event_test, version by sys.version_info.major and count obj refs prio…

b985825

…r test use Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

event_test, some counting info with assert

a55b2e9

Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

event_test, some counting info with assert

0ee4c90

Signed-off-by: Kashirin Alex <kashirin.alex@gmail.com>

jstasiak requested changes Sep 12, 2018

View reviewed changes

itamarst added bug important labels Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 408 fix, Memory leak was generated from Event #524

Issue 408 fix, Memory leak was generated from Event #524

kashirin-alex commented Sep 12, 2018 •

edited

kashirin-alex commented Sep 12, 2018 •

edited

jstasiak left a comment

jstasiak Sep 12, 2018

kashirin-alex Sep 12, 2018

jstasiak Sep 12, 2018

jstasiak Sep 12, 2018

jstasiak Sep 12, 2018

jstasiak Sep 12, 2018

kashirin-alex Sep 12, 2018

jstasiak Sep 12, 2018

kashirin-alex Sep 12, 2018

kashirin-alex Sep 12, 2018

kashirin-alex Sep 12, 2018

kashirin-alex commented Sep 12, 2018

Issue 408 fix, Memory leak was generated from Event #524

Are you sure you want to change the base?

Issue 408 fix, Memory leak was generated from Event #524

Conversation

kashirin-alex commented Sep 12, 2018 • edited

kashirin-alex commented Sep 12, 2018 • edited

jstasiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashirin-alex commented Sep 12, 2018

kashirin-alex commented Sep 12, 2018 •

edited

kashirin-alex commented Sep 12, 2018 •

edited