Rework when_changed attribute to use weakrefs

Some fairly major changes to ensure that the Pin.when_changed property
doesn't keep references to the objects owning the callbacks that are
assigned. This is vaguely tricky given that ordinary weakref's can't be
used with bound methods (which are ephemeral), so I've back-ported
weakref.WeakMethod from Py3.4.

This solves a whole pile of things like Button instances not
disappearing when they're deleted, and makes composite devices
containing Buttons much easier to construct as we don't need to worry
about partially constructed things not getting deleted.

gpiozero/boards.py

@@ -211,24 +211,20 @@ class LEDCollection(CompositeOutputDevice):
         initial_value = kwargs.pop('initial_value', False)
         order = kwargs.pop('_order', None)
         LEDClass = PWMLED if pwm else LED
-        try:
+        super(LEDCollection, self).__init__(
-            super(LEDCollection, self).__init__(
+            *(
-                *(
+                pin_or_collection
-                    pin_or_collection
+                if isinstance(pin_or_collection, LEDCollection) else
-                    if isinstance(pin_or_collection, LEDCollection) else
+                LEDClass(pin_or_collection, active_high, initial_value)
-                    LEDClass(pin_or_collection, active_high, initial_value)
+                for pin_or_collection in args
-                    for pin_or_collection in args
+                ),
-                    ),
+            _order=order,
-                _order=order,
+            **{
-                **{
+                name: pin_or_collection
-                    name: pin_or_collection
+                if isinstance(pin_or_collection, LEDCollection) else
-                    if isinstance(pin_or_collection, LEDCollection) else
+                LEDClass(pin_or_collection, active_high, initial_value)
-                    LEDClass(pin_or_collection, active_high, initial_value)
+                for name, pin_or_collection in kwargs.items()
-                    for name, pin_or_collection in kwargs.items()
+                })
-                    })
-        except:
-            self.close()
-            raise
         leds = []
         for item in self:
             if isinstance(item, LEDCollection):

gpiozero/compat.py

@@ -9,6 +9,7 @@ from __future__ import (
 str = type('')
 
 import cmath
+import weakref
 import collections
 import operator
 import functools
@@ -81,3 +82,59 @@ class frozendict(collections.Mapping):
             hashes = map(hash, self.items())
             self.__hash = functools.reduce(operator.xor, hashes, 0)
         return self.__hash
+
+
+# Backported from py3.4
+class WeakMethod(weakref.ref):
+    """
+    A custom `weakref.ref` subclass which simulates a weak reference to
+    a bound method, working around the lifetime problem of bound methods.
+    """
+
+    __slots__ = "_func_ref", "_meth_type", "_alive", "__weakref__"
+
+    def __new__(cls, meth, callback=None):
+        try:
+            obj = meth.__self__
+            func = meth.__func__
+        except AttributeError:
+            raise TypeError("argument should be a bound method, not {0}"
+                            .format(type(meth))) from None
+        def _cb(arg):
+            # The self-weakref trick is needed to avoid creating a reference
+            # cycle.
+            self = self_wr()
+            if self._alive:
+                self._alive = False
+                if callback is not None:
+                    callback(self)
+        self = weakref.ref.__new__(cls, obj, _cb)
+        self._func_ref = weakref.ref(func, _cb)
+        self._meth_type = type(meth)
+        self._alive = True
+        self_wr = weakref.ref(self)
+        return self
+
+    def __call__(self):
+        obj = super(WeakMethod, self).__call__()
+        func = self._func_ref()
+        if obj is None or func is None:
+            return None
+        return self._meth_type(func, obj)
+
+    def __eq__(self, other):
+        if isinstance(other, WeakMethod):
+            if not self._alive or not other._alive:
+                return self is other
+            return weakref.ref.__eq__(self, other) and self._func_ref == other._func_ref
+        return False
+
+    def __ne__(self, other):
+        if isinstance(other, WeakMethod):
+            if not self._alive or not other._alive:
+                return self is not other
+            return weakref.ref.__ne__(self, other) or self._func_ref != other._func_ref
+        return True
+
+    __hash__ = weakref.ref.__hash__
+

gpiozero/mixins.py

@@ -438,23 +438,28 @@ class HoldThread(GPIOThread):
     device is active.
     """
     def __init__(self, parent):
-        super(HoldThread, self).__init__(target=self.held, args=(parent,))
+        super(HoldThread, self).__init__(
+            target=self.held, args=(weakref.proxy(parent),))
         self.holding = Event()
         self.start()
 
     def held(self, parent):
-        while not self.stopping.is_set():
+        try:
-            if self.holding.wait(0.1):
+            while not self.stopping.is_set():
-                self.holding.clear()
+                if self.holding.wait(0.1):
-                while not (
+                    self.holding.clear()
-                        self.stopping.is_set() or
+                    while not (
-                        parent._inactive_event.wait(parent.hold_time)
+                            self.stopping.is_set() or
-                        ):
+                            parent._inactive_event.wait(parent.hold_time)
-                    if parent._held_from is None:
+                            ):
-                        parent._held_from = time()
+                        if parent._held_from is None:
-                    parent._fire_held()
+                            parent._held_from = time()
-                    if not parent.hold_repeat:
+                        parent._fire_held()
-                        break
+                        if not parent.hold_repeat:
+                            break
+        except ReferenceError:
+            # Parent is dead; time to die!
+            pass
 
 
 class GPIOQueue(GPIOThread):

gpiozero/pins/pi.py

@@ -7,7 +7,12 @@ from __future__ import (
 str = type('')
 
 import io
-import weakref
+from threading import RLock
+from weakref import ref, proxy
+try:
+    from weakref import WeakMethod
+except ImportError:
+    from .compat import WeakMethod
 import warnings
 
 try:
@@ -186,7 +191,9 @@ class PiPin(Pin):
     """
     def __init__(self, factory, number):
         super(PiPin, self).__init__()
-        self._factory = weakref.proxy(factory)
+        self._factory = proxy(factory)
+        self._when_changed_lock = RLock()
+        self._when_changed = None
         self._number = number
         try:
             factory.pi_info.physical_pin(self.address[-1])
@@ -206,3 +213,39 @@ class PiPin(Pin):
     def _get_address(self):
         return self.factory.address + ('GPIO%d' % self.number,)
 
+    def _call_when_changed(self):
+        method = self.when_changed()
+        if method is None:
+            self.when_changed = None
+        else:
+            method()
+
+    def _get_when_changed(self):
+        return self._when_changed
+
+    def _set_when_changed(self, value):
+        # Have to take care, if value is either a closure or a bound method,
+        # not to keep a strong reference to the containing object
+        with self._when_changed_lock:
+            if self._when_changed is None and value is not None:
+                if isinstance(value, MethodType):
+                    self._when_changed = WeakMethod(value)
+                else:
+                    self._when_changed = ref(value)
+                self._enable_event_detect()
+            elif self._when_changed is not None and value is None:
+                self._disable_event_detect()
+                self._when_changed = None
+            elif value is None:
+                self._when_changed = None
+            elif isinstance(value, MethodType):
+                self._when_changed = WeakMethod(value)
+            else:
+                self._when_changed = ref(value)
+
+    def _enable_event_detect(self):
+        raise NotImplementedError
+
+    def _disable_event_detect(self):
+        raise NotImplementedError
+

gpiozero/pins/pigpiod.py

@@ -6,9 +6,15 @@ from __future__ import (
     )
 str = type('')
 
-import weakref
-import pigpio
 import os
+from weakref import proxy
+from threading import RLock
+try:
+    from weakref import WeakMethod
+except ImportError:
+    from .compat import WeakMethod
+
+import pigpio
 
 from . import SPI
 from .pi import PiPin, PiFactory
@@ -164,6 +170,7 @@ class PiGPIOPin(PiPin):
         self._pull = 'up' if factory.pi_info.pulled_up(self.address[-1]) else 'floating'
         self._pwm = False
         self._bounce = None
+        self._when_changed_lock = RLock()
         self._when_changed = None
         self._callback = None
         self._edges = pigpio.EITHER_EDGE
@@ -269,26 +276,24 @@ class PiGPIOPin(PiPin):
         finally:
             self.when_changed = f
 
-    def _get_when_changed(self):
+    def _call_when_changed(self, gpio, level, tick):
-        if self._callback is None:
+        super(PiGPIOPin, self)._call_when_changed()
-            return None
-        return self._callback.callb.func
 
-    def _set_when_changed(self, value):
+    def _enable_event_detect(self):
+        self._callback = self.factory.connection.callback(
+                self.number, self._edges, self._call_when_changed)
+
+    def _disable_event_detect(self):
         if self._callback is not None:
             self._callback.cancel()
             self._callback = None
-        if value is not None:
-            self._callback = self.factory.connection.callback(
-                    self.number, self._edges,
-                    lambda gpio, level, tick: value())
 
 
 class PiGPIOHardwareSPI(SPI, Device):
     def __init__(self, factory, port, device):
         self._port = port
         self._device = device
-        self._factory = weakref.proxy(factory)
+        self._factory = proxy(factory)
         super(PiGPIOHardwareSPI, self).__init__()
         self._reserve_pins(*(
             factory.address + ('GPIO%d' % pin,)
@@ -382,7 +387,7 @@ class PiGPIOSoftwareSPI(SPI, Device):
         self._clock_pin = clock_pin
         self._mosi_pin = mosi_pin
         self._miso_pin = miso_pin
-        self._factory = weakref.proxy(factory)
+        self._factory = proxy(factory)
         super(PiGPIOSoftwareSPI, self).__init__()
         self._reserve_pins(
             factory.pin_address(clock_pin),

gpiozero/pins/rpigpio.py

@@ -7,6 +7,14 @@ from __future__ import (
 str = type('')
 
 import warnings
+from types import MethodType
+from threading import RLock
+from weakref import ref
+try:
+    from weakref import WeakMethod
+except ImportError:
+    from .compat import WeakMethod
+
 from RPi import GPIO
 
 from .local import LocalPiFactory, LocalPiPin
@@ -89,6 +97,7 @@ class RPiGPIOPin(LocalPiPin):
         self._frequency = None
         self._duty_cycle = None
         self._bounce = -666
+        self._when_changed_lock = RLock()
         self._when_changed = None
         self._edges = GPIO.BOTH
         GPIO.setup(self.number, GPIO.IN, self.GPIO_PULL_UPS[self._pull])
@@ -202,19 +211,15 @@ class RPiGPIOPin(LocalPiPin):
         finally:
             self.when_changed = f
 
-    def _get_when_changed(self):
+    def _call_when_changed(self, channel):
-        return self._when_changed
+        super(RPiGPIOPin, self)._call_when_changed()
 
-    def _set_when_changed(self, value):
+    def _enable_event_detect(self):
-        if self._when_changed is None and value is not None:
+        GPIO.add_event_detect(
-            self._when_changed = value
+            self.number, self._edges,
-            GPIO.add_event_detect(
+            callback=self._call_when_changed,
-                self.number, self._edges,
+            bouncetime=self._bounce)
-                callback=lambda channel: self._when_changed(),
+
-                bouncetime=self._bounce)
+    def _disable_event_detect(self):
-        elif self._when_changed is not None and value is None:
+        GPIO.remove_event_detect(self.number)
-            GPIO.remove_event_detect(self.number)
-            self._when_changed = None
-        else:
-            self._when_changed = value
 

gpiozero/pins/rpio.py

@@ -8,6 +8,12 @@ str = type('')
 
 
 import warnings
+from threading import RLock
+try:
+    from weakref import WeakMethod
+except ImportError:
+    from .compat import WeakMethod
+
 import RPIO
 import RPIO.PWM
 from RPIO.Exceptions import InvalidChannelException
@@ -83,6 +89,7 @@ class RPIOPin(LocalPiPin):
         self._pwm = False
         self._duty_cycle = None
         self._bounce = None
+        self._when_changed_lock = RLock()
         self._when_changed = None
         self._edges = 'both'
         try:
@@ -191,25 +198,20 @@ class RPIOPin(LocalPiPin):
         finally:
             self.when_changed = f
 
-    def _get_when_changed(self):
+    def _call_when_changed(self, channel, value):
-        return self._when_changed
+        super(RPIOPin, self)._call_when_changed()
 
-    def _set_when_changed(self, value):
+    def _enable_event_detect(self):
-        if self._when_changed is None and value is not None:
+        RPIO.add_interrupt_callback(
-            self._when_changed = value
+            self.number, self._call_when_changed, self._edges,
-            RPIO.add_interrupt_callback(
+            self.GPIO_PULL_UPS[self._pull], self._bounce)
-                self.number,
+
-                lambda channel, value: self._when_changed(),
+    def _disable_event_detect(self):
-                self._edges, self.GPIO_PULL_UPS[self._pull], self._bounce)
+        try:
-        elif self._when_changed is not None and value is None:
+            RPIO.del_interrupt_callback(self.number)
-            try:
+        except KeyError:
-                RPIO.del_interrupt_callback(self.number)
+            # Ignore this exception which occurs during shutdown; this
-            except KeyError:
+            # simply means RPIO's built-in cleanup has already run and
-                # Ignore this exception which occurs during shutdown; this
+            # removed the handler
-                # simply means RPIO's built-in cleanup has already run and
+            pass
-                # removed the handler
-                pass
-            self._when_changed = None
-        else:
-            self._when_changed = value