Repository: zoofIO/flexx
Pull request #202 (2016-09-11 23:15:12)

Improvements to avoid memory leaks
This makes the handler store references of the objects of bound methods using a weak ref. That way, a handler can never be the thing that is holding back object deletion. In effect it is generally no longer necessary to disconnect connections; thinks should degenerate by themselves.

This is nice, since a typo in a disconnect call (as was present in the colabpainting example) could easily cause memory leaks (which is, I think, why the AWS server was broken so quickly).

Also allowed using decorators using `some_object.connect()`. In some examples, we can now write `relay.connect(...)`. By writing it in that way, the object is aware of the handler, and will dispose it when it is itself disposed, which means we don't get these 'cannot send commands, app is closed' warnings.



Patch:
diff --git a/flexx/app/session.py b/flexx/app/session.py
index 357b8f3..7d1d13f 100644
--- a/flexx/app/session.py
+++ b/flexx/app/session.py
@@ -242,6 +242,7 @@ class Session(SessionAssets):
         self._runtime = None  # init web runtime, will be set when used
         self._ws = None  # init websocket, will be set when a connection is made
         self._model = None  # Model instance, can be None if app_name is __default__
+        self._closing = False
         
         # While the client is not connected, we keep a queue of
         # commands, which are send to the client as soon as it connects
@@ -306,11 +307,15 @@ class Session(SessionAssets):
         """ Close the runtime, if possible
         """
         # todo: close via JS
-        if self._runtime:
-            self._runtime.close()
-        if self._model:
-            self._model.dispose()
-            self._model = None  # break circular reference
+        self._closing = True  # suppress warnings for session being closed.
+        try:
+            if self._runtime:
+                self._runtime.close()
+            if self._model:
+                self._model.dispose()
+                self._model = None  # break circular reference
+        finally:
+            self._closing = False
     
     @property
     def status(self):
@@ -334,6 +339,8 @@ class Session(SessionAssets):
             self._ws.command(command)
         elif self.status == self.STATUS.PENDING:
             self._pending_commands.append(command)
+        elif self._closing:
+            pass
         else:
             #raise RuntimeError('Cannot send commands; app is closed')
             logger.warn('Cannot send commands; app is closed')
diff --git a/flexx/app/tornadoserver.py b/flexx/app/tornadoserver.py
index 211336a..82888db 100644
--- a/flexx/app/tornadoserver.py
+++ b/flexx/app/tornadoserver.py
@@ -258,9 +258,9 @@ class MainHandler(tornado.web.RequestHandler):
                                                       correct_app_name != app_name):
             self.redirect('/%s/%s' % (correct_app_name, file_name))
         
-        if app_name == '__index__':
+        elif app_name == '__index__':
             # Show plain index page
-            all_apps = ['<li><a href="%s">%s</a></li>' % (name, name) for name in 
+            all_apps = ['<li><a href="%s/">%s</a></li>' % (name, name) for name in 
                         manager.get_app_names()]
             the_list = '<ul>%s</ul>' % ''.join(all_apps) if all_apps else 'no apps'
             self.write('Index of available apps: ' + the_list)
diff --git a/flexx/event/_handler.py b/flexx/event/_handler.py
index dc6d494..e686afb 100644
--- a/flexx/event/_handler.py
+++ b/flexx/event/_handler.py
@@ -2,20 +2,30 @@
 Implementation of handler class and corresponding descriptor.
 """
 
-import inspect
 import weakref
+import inspect
 
 from ._dict import Dict
 from ._loop import loop
 from . import logger
 
 
+console = window = None
+
+
 def this_is_js():
     return False
 
-console = window = None
 
+def looks_like_method(func):
+    if hasattr(func, '__func__'):
+        return False  # this is a bound method
+    try:
+        return inspect.getargspec(func)[0][0] in ('self', 'this')
+    except (TypeError, IndexError):
+        return False
 
+        
 # Decorator to wrap a function in a Handler object
 def connect(*connection_strings):
     """ Decorator to turn a method of HasEvents into an event
@@ -53,6 +63,9 @@ def connect(*connection_strings):
     def _connect(func):
         if not callable(func):
             raise TypeError('connect() decorator requires a callable.')
+        if not looks_like_method(func):
+            raise TypeError('connect() decorator requires a method '
+                            '(first arg must be self).')
         return HandlerDescriptor(func, connection_strings)
     
     if func is not None:
@@ -63,12 +76,19 @@ def connect(*connection_strings):
 
 class HandlerDescriptor:
     """ Class descriptor for handlers.
+    
+    Arguments:
+        func (callable): function that handles the events.
+        connection_strings (list): the strings that represent the connections.
+        ob (HasEvents, optional): the HasEvents object to use a a basis for the
+            connection. A weak reference to this object is stored.
     """
     
-    def __init__(self, func, connection_strings):
+    def __init__(self, func, connection_strings, ob=None):
         assert callable(func)  # HandlerDescriptor is not instantiated directly
         self._func = func
         self._name = func.__name__  # updated by HasEvents meta class
+        self._ob = None if ob is None else weakref.ref(ob)
         self._connection_strings = connection_strings
         self.__doc__ = '*%s*: %s' % ('event handler', func.__doc__ or self._name)
     
@@ -90,7 +110,8 @@ class HandlerDescriptor:
         try:
             handler = getattr(instance, private_name)
         except AttributeError:
-            handler = Handler(self._func, self._connection_strings, instance)
+            handler = Handler((self._func, instance), self._connection_strings,
+                              instance if self._ob is None else self._ob())
             setattr(instance, private_name, handler)
         
         # Make the handler use *our* func one time. In most situations
@@ -116,39 +137,40 @@ class Handler:
         func (callable): function that handles the events.
         connection_strings (list): the strings that represent the connections.
         ob (HasEvents): the HasEvents object to use a a basis for the
-            connection. A weak reference to this object is stored. It
-            is passed a a first argument to the function in case its
-            first arg is self.
+            connection. A weak reference to this object is stored.
     """
     
     _count = 0
     
     def __init__(self, func, connection_strings, ob):
-        # Check and set func
-        assert callable(func)
-        self._func = func
-        self._func_once = func
-        self._name = func.__name__
         Handler._count += 1
         self._id = 'h%i' % Handler._count  # to ensure a consistent event order
         
-        # Set docstring; this appears correct in sphinx docs
-        self.__doc__ = '*%s*: %s' % ('event handler', func.__doc__ or self._name)
-        
-        # Store object using a weakref
-        self._ob = weakref.ref(ob)
+        # Store objects using a weakref.
+        # - ob1 is the HasEvents object of which the connect() method was called
+        #   to create the handler. Connection strings are relative to this object.
+        # - ob2 is the object to be passed to func (if it is a method). Is often
+        #   the same as ob1, but not per see. Can be None.
+        self._ob1 = weakref.ref(ob)
         
-        # Get whether function is a method
-        try:
-            self._func_is_method = inspect.getargspec(func)[0][0] in ('self', 'this')
-        except (TypeError, IndexError):
-            self._func_is_method = False
+        # Get unbounded version of bound methods. 
+        self._ob2 = None  # if None, its regarded a regular function
+        if isinstance(func, tuple):
+            self._ob2 = weakref.ref(func[1])
+            func = func[0]
         if getattr(func, '__self__', None) is not None:
-            self._func_is_method = False  # already bound
+            self._ob2 = weakref.ref(func.__self__)
+            func = func.__func__
+        
+        # Store func, name, and docstring (e.g. for sphinx docs)
+        assert callable(func)
+        self._func = func
+        self._func_once = func
+        self._name = func.__name__
+        self.__doc__ = '*%s*: %s' % ('event handler', func.__doc__ or self._name)
         
         self._init(connection_strings)
     
-    
     def _init(self, connection_strings):
         """ Init of this handler that is compatible with PyScript.
         """
@@ -197,8 +219,13 @@ class Handler:
         """ Call the handler function.
         """
         func = self._func_once
-        if self._func_is_method and self._ob is not None:
-            res = func(self._ob(), *events)
+        if self._ob2 is not None:
+            if self._ob2() is not None:
+                res = func(self._ob2(), *events)
+            else:
+                # We detected that the object that wants the events no longer exist
+                self.dispose()
+                return
         else:
             res = func(*events)
         self._func_once = self._func
@@ -310,7 +337,7 @@ class Handler:
         path = connection.fullname.replace('.*', '*').split('.')[:-1]
         
         # Obtain root object and setup connections
-        ob = self._ob()
+        ob = self._ob1()
         if ob is not None:
             self._seek_event_object(index, path, ob)
         
diff --git a/flexx/event/_hasevents.py b/flexx/event/_hasevents.py
index f4a373d..c2ddab3 100644
--- a/flexx/event/_hasevents.py
+++ b/flexx/event/_hasevents.py
@@ -6,7 +6,7 @@ generated and handled. It is the object that keeps track of handlers.
 import sys
 
 from ._dict import Dict
-from ._handler import HandlerDescriptor, Handler
+from ._handler import HandlerDescriptor, Handler, looks_like_method
 from ._emitters import BaseEmitter, Property
 from ._loop import loop
 from . import logger
@@ -414,8 +414,11 @@ class HasEvents(with_metaclass(HasEventsMeta, object)):
         
         def _connect(func):
             if not callable(func):
-                raise TypeError('connect() decotator requires a callable.')
-            return Handler(func, connection_strings, self)
+                raise TypeError('connect() decorator requires a callable.')
+            if looks_like_method(func):
+                return HandlerDescriptor(func, connection_strings, self)
+            else:
+                return Handler(func, connection_strings, self)
         
         if func is not None:
             return _connect(func)
diff --git a/flexx/event/_js.py b/flexx/event/_js.py
index 4ed5a8d..e8fe103 100644
--- a/flexx/event/_js.py
+++ b/flexx/event/_js.py
@@ -151,7 +151,7 @@ class HasEventsJS:
         HasEvents.prototype._HANDLER_COUNT += 1
         handler._name = name
         handler._id = 'h' + str(HasEvents.prototype._HANDLER_COUNT)
-        handler._ob = lambda : that  # no weakref in JS
+        handler._ob1 = lambda : that  # no weakref in JS
         handler._init(connection_strings, self)
         
         return handler
diff --git a/flexx/ui/examples/chatroom.py b/flexx/ui/examples/chatroom.py
index 9765906..1782ced 100644
--- a/flexx/ui/examples/chatroom.py
+++ b/flexx/ui/examples/chatroom.py
@@ -26,6 +26,10 @@ class MessageBox(ui.Label):
     """
 
 
+# Create global relay
+relay = Relay()
+
+
 class ChatRoom(ui.Widget):
     """ Despite the name, this represents one connection to the chat room.
     """
@@ -43,19 +47,15 @@ class ChatRoom(ui.Widget):
                     self.ok = ui.Button(text='Send')
             ui.Widget(flex=1)
         
-        # Pipe messages send by the relay into this app
-        relay.connect(self._push_info, 'new_message:' + self.id)
-        
         self._update_participants()
     
+    @relay.connect('new_message')  # note that we connect to relay
     def _push_info(self, *events):
-        if self.session.status:
-            for ev in events:
-                self.emit('new_message', ev)
+        for ev in events:
+            self.emit('new_message', ev)
     
     def _update_participants(self):
         if not self.session.status:
-            relay.disconnect('new_message:' + self.id)
             return  # and dont't invoke a new call
         proxies = app.manager.get_connections(self.__class__.__name__)
         names = [p.app.name.text for p in proxies]
@@ -80,8 +80,6 @@ class ChatRoom(ui.Widget):
             self.messages.text += ''.join([ev.msg for ev in events])
 
 
-# Create global relay
-relay = Relay()
 
 if __name__ == '__main__':
     app.serve(ChatRoom)
diff --git a/flexx/ui/examples/colab_painting.py b/flexx/ui/examples/colab_painting.py
index b5976a0..b7e656c 100644
--- a/flexx/ui/examples/colab_painting.py
+++ b/flexx/ui/examples/colab_painting.py
@@ -58,10 +58,6 @@ class ColabPainting(ui.Widget):
                 ui.Widget(flex=1)
             ui.Widget(flex=1)
         
-        # Connect events
-        self.connect(self._this_user_adds_paint, 'canvas.mouse_down')
-        relay.connect(self._any_user_adds_paint, 'global_paint:' + self.id)
-        
         # Start people-count-updater
         self._update_participants()
     
@@ -70,21 +66,23 @@ class ColabPainting(ui.Widget):
         """ The selected color for the current session. """
         return str(color)
     
+    @event.connect('canvas.mouse_down')
     def _this_user_adds_paint(self, *events):
         """ Detect mouse down, emit global paint event via the relay. """
         for ev in events:
             relay.global_paint(ev.pos, self.color)
     
+    @relay.connect('global_paint')  # note that we connect to relay here
     def _any_user_adds_paint(self, *events):
         """ Receive global paint event from the relay, emit local paint event. """
-        if self.session.status:
-            for ev in events:
-                self.emit('paint', ev)
+        # if not self.session.status:
+        #     return  I think this is not required anymore. Worst case we get a warning
+        for ev in events:
+            self.emit('paint', ev)
     
     def _update_participants(self):
         """ Keep track of the number of participants. """
         if not self.session.status:
-            relay.disconnect('new_dot:' + self.id)  # clean up
             return  # and dont't invoke a new call
         proxies = app.manager.get_connections(self.__class__.__name__)
         n = len(proxies)
diff --git a/flexx/ui/examples/monitor.py b/flexx/ui/examples/monitor.py
index 48f5e52..430bf32 100644
--- a/flexx/ui/examples/monitor.py
+++ b/flexx/ui/examples/monitor.py
@@ -42,6 +42,10 @@ class Relay(event.HasEvents):
         app.call_later(1, self.refresh)
 
 
+# Create global relay
+relay = Relay()
+
+
 class Monitor(ui.Widget):
     
     def init(self):
@@ -64,10 +68,8 @@ class Monitor(ui.Widget):
                                               ylabel='Mem usage (%)',
                                               sync_props=False)
                 ui.Widget(flex=1)
-        
-        # Relay global info into this app
-        relay.connect(self._push_info, 'system_info:' + self.id)
     
+    @relay.connect('system_info')  # note that we connect to relay
     def _push_info(self, *events):
         if not self.session.status:
             return relay.disconnect('system_info:' + self.id)
@@ -118,10 +120,6 @@ class Monitor(ui.Widget):
             self.mem_plot.ydata = usage
 
 
-# Create global relay
-relay = Relay()
-
-
 if __name__ == '__main__':
     app.serve(Monitor)
     # m = app.launch(Monitor)  # for use during development


Test patch:
diff --git a/flexx/event/tests/test_handlers.py b/flexx/event/tests/test_handlers.py
index be17057..60fa5cc 100644
--- a/flexx/event/tests/test_handlers.py
+++ b/flexx/event/tests/test_handlers.py
@@ -177,13 +177,19 @@ def test_func_handlers_nodecorator_reverse_connect_order():
 
 def test_func_handlers_with_method_decorator():
 
+    with raises(TypeError):  # error because needs self as first arg
+        @event.connect('x')
+        def foo(*events):
+            pass
+    
+    # We can fool Flexx by putting self as a first arg. No way for Flexx to see
+    # that this is not a method.
     @event.connect('x')
-    def foo(*events):
+    def foo(self, *events):
         pass
     
     assert isinstance(foo, event._handler.HandlerDescriptor)
     assert repr(foo)
-    # too bad we cannot raise an error when a function in decorated this way
 
 
 def test_method_handler_invoking():
@@ -192,7 +198,8 @@ def test_method_handler_invoking():
     class MyObject(event.HasEvents):
         
         @event.connect('x1', 'x2')
-        def handler(*events):
+        def handler(self, *events):
+            print(events)
             called.append(len(events))
         
         @event.connect('x3')
@@ -258,12 +265,27 @@ def test_method_handler_invoking_other():
     
     h = event.HasEvents()
     
-    with raises(RuntimeError):
-        class Foo(event.HasEvents):
-            
-            @h.connect('x1')
-            def handler(self, *events):
-                self.was_invoked = True
+    # This used not not work
+    # with raises(RuntimeError):
+    #     class Foo(event.HasEvents):
+    #         
+    #         @h.connect('x1')
+    #         def handler(self, *events):
+    #             self.was_invoked = True
+    
+    # But now it does!
+    
+    class Foo(event.HasEvents):
+        
+        @h.connect('x1')
+        def handler(self, *events):
+            self.was_invoked = True
+    
+    foo = Foo()
+    
+    h.emit('x1')
+    event.loop.iter()
+    assert foo.was_invoked
 
 
 def test_connecting():
@@ -377,4 +399,36 @@ def test_dispose2():
     assert handler_ref() is None
 
 
+def test_dispose3():
+    # Test that connecting a "volatile" object to a static object works well
+    # w.r.t. cleanup.
+    
+    relay = event.HasEvents()
+    
+    class Foo:
+        def bar(self, *events):
+            pass
+    
+    foo = Foo()
+    handler = relay.connect(foo.bar, 'xx')
+    
+    handler_ref = weakref.ref(handler)
+    foo_ref = weakref.ref(foo)
+    
+    del foo
+    del handler
+    
+    gc.collect()
+    
+    assert foo_ref() is None
+    assert handler_ref() is not None
+    
+    relay.emit('xx')
+    event.loop.iter()
+    gc.collect()
+    
+    assert foo_ref() is None
+    assert handler_ref() is None
+
+
 run_tests_if_main()