Repository: ManiacalLabs/BiblioPixel (python)

Issue:
Builder.close() doesn't stop the drivers.
For example, when I run a 12k project from `bp`, I see this at the end:

    INFO - driver - Closing connection to: /dev/cu.usbmodem1403261
    INFO - driver - Closing connection to: /dev/cu.usbmodem2084771
    INFO - driver - Closing connection to: /dev/cu.usbmodem2084491

This never appears if I use a project builder.  

Omitting the shutdown has negative results - if I run it this way a few times, eventually the 12k gets into a state where I have to power cycle it and detach the USB to get it to work again.

Pull request:
Fix #1104
This took way more work than it should but it was very interesting.

In case it comes up again:

I managed to fix _most of_ the problem immediately in the first tiny commit, but at least on my machine, it could still be that there was a long delay before `__del__` was called on Projects and Builders - and that meant that if you create a new Project or Builder, the new driver would fail to open because it was held by the old one.

Before Python 3.4, if you had a circular reference in your tree, and at least one of the items had a `__del__`, the memory is never garbage collected.  That's no longer true, but it seems that in that case, the `__del__` doesn't go off until you either do a bunch of work, or call the garbage collector by hand.

I reduced the circular references to nothing by refactoring in some cases, and in a couple of cases using weak references.   Now the garbage collection goes off instantaneously.


Patch:
diff --git a/bibliopixel/animation/animation.py b/bibliopixel/animation/animation.py
index 0371686..d2f4fe4 100644
--- a/bibliopixel/animation/animation.py
+++ b/bibliopixel/animation/animation.py
@@ -52,14 +52,12 @@ class Animation(object):
         self.on_completion = None
         self.state = runner.STATE.ready
         self.preclear = preclear
-        self.project = None
         self.runner = None
         self.time = time.time
         self.preframe_callbacks = []
 
     def set_project(self, project):
-        self.project = project
-        self.time = project.time
+        self.time = project.clock.time
         self.runner.set_project(project)
         self.threading.set_project(project)
 
diff --git a/bibliopixel/animation/animation_threading.py b/bibliopixel/animation/animation_threading.py
index ce69b4d..ff90efc 100644
--- a/bibliopixel/animation/animation_threading.py
+++ b/bibliopixel/animation/animation_threading.py
@@ -17,8 +17,7 @@ class AnimationThreading:
         self.sleep = time.sleep
 
     def set_project(self, project):
-        self.project = project
-        self.sleep = project.sleep
+        self.sleep = project.clock.sleep
 
     def join(self, timeout=None):
         self.thread and self.thread.join(timeout)
diff --git a/bibliopixel/animation/movie_writer.py b/bibliopixel/animation/movie_writer.py
index ff746ab..0bb33f9 100644
--- a/bibliopixel/animation/movie_writer.py
+++ b/bibliopixel/animation/movie_writer.py
@@ -46,10 +46,6 @@ class MovieWriter(wrapper.Wrapper):
         if self.movie_writer.step(self.cur_step):
             self.completed = True
 
-    def set_project(self, project):
-        super().set_project(project)
-        self.movie_writer.set_project(project)
-
     def cleanup(self, clean_layout=True):
         super().cleanup(clean_layout)
         self.movie_writer.write()
diff --git a/bibliopixel/animation/runner.py b/bibliopixel/animation/runner.py
index 3ce11ef..6c221fa 100644
--- a/bibliopixel/animation/runner.py
+++ b/bibliopixel/animation/runner.py
@@ -73,9 +73,9 @@ class Runner(object):
         self.time = time.time
 
     def set_project(self, project):
-        self.time = project.time
         if self.flat_out:
             project.flat_out()
+        self.time = project.clock.time
 
     @property
     def fps(self):
diff --git a/bibliopixel/builder/builder.py b/bibliopixel/builder/builder.py
index 493daf8..298c560 100644
--- a/bibliopixel/builder/builder.py
+++ b/bibliopixel/builder/builder.py
@@ -5,6 +5,7 @@ from .. import colors
 from .. commands import animations
 from .. drivers.SimPixel import driver as simpixel_driver
 from .. project import project
+from .. util import log
 
 
 class Builder(SavedDescription):
@@ -15,7 +16,6 @@ class Builder(SavedDescription):
     COLORS = colors.COLORS
 
     def __init__(self, project_file='', desc=None, threaded=None, **kwds):
-
         self.project = None
         self.pixel = Pixels(self)
         self._runner = Runner(self)
@@ -29,18 +29,24 @@ class Builder(SavedDescription):
         """Creates and starts the project."""
         if threaded is not None:
             self.threaded = threaded
+        run = {'run': {'threaded': False}}
+        self.project = project.project(
+            self.desc, run, root_file=self.project_file)
+        self._run = self.project.run
         self._runner.start(self.threaded)
 
     def stop(self=None):
         """Stop the builder if it's running."""
         if not self:
-            self = getattr(Runner.instance(), 'builder', None)
+            instance = getattr(Runner.instance(), 'builder', None)
+            self = instance and instance()
             if not self:
                 return
 
         self._runner.stop()
         if self.project:
             self.project.stop()
+            self.project = None
 
     def clear(self):
         """Stop the project if it's running and clear the project description"""
@@ -94,10 +100,3 @@ class Builder(SavedDescription):
 
     def __deepcopy__(self, _):
         return self.__copy__()
-
-    def _run(self):
-        """Called from the Runner class"""
-        run = {'run': {'threaded': False}}
-        self.project = project.project(
-            self.desc, run, root_file=self.project_file)
-        self.project.start()
diff --git a/bibliopixel/builder/pixels.py b/bibliopixel/builder/pixels.py
index 9679dcb..5c2104f 100644
--- a/bibliopixel/builder/pixels.py
+++ b/bibliopixel/builder/pixels.py
@@ -1,3 +1,4 @@
+import weakref
 from .. colors import make
 from .. util import log
 
@@ -8,7 +9,7 @@ class Pixels:
     better error reporting.
     """
     def __init__(self, builder):
-        self.builder = builder
+        self.builder = weakref.ref(builder)
 
     def __getitem__(self, index):
         """
@@ -34,9 +35,11 @@ class Pixels:
 
     @property
     def layout(self):
-        if not self.builder.project:
+        b = self.builder()
+
+        if not (b and b.project):
             raise ValueError('Cannot get layout before Builder has started')
-        return self.builder.project.layout
+        return b.project.layout
 
     @property
     def shape(self):
diff --git a/bibliopixel/builder/runner.py b/bibliopixel/builder/runner.py
index 37937b1..d8eada5 100644
--- a/bibliopixel/builder/runner.py
+++ b/bibliopixel/builder/runner.py
@@ -10,7 +10,7 @@ class Runner:
     def __init__(self, builder):
         self.thread = None
         self.is_running = False
-        self.builder = builder
+        self.builder = weakref.ref(builder)
 
     def start(self, threaded):
         """Creates and starts the project."""
@@ -52,11 +52,11 @@ class Runner:
 
     def _target(self):
         try:
-            log.info('Running Builder')
-            self.builder._run()
+            self.builder()._run()
         except:
             if self.thread:
                 log.error(traceback.format_exc())
             raise
         finally:
-            self.builder.stop()
+            b = self.builder()
+            b and b.stop()
diff --git a/bibliopixel/control/action.py b/bibliopixel/control/action.py
index d94068b..11d447b 100644
--- a/bibliopixel/control/action.py
+++ b/bibliopixel/control/action.py
@@ -43,13 +43,6 @@ class Action(Receiver):
             return cls(**action)
         return cls(*action)
 
-    if deprecated.allowed():
-        set_root = set_project
-
-        @property
-        def root(self):
-            return self.project
-
 
 class ActionList(Receiver):
     """A list of Actions."""
diff --git a/bibliopixel/control/control.py b/bibliopixel/control/control.py
index f8e4f60..cfc5c52 100644
--- a/bibliopixel/control/control.py
+++ b/bibliopixel/control/control.py
@@ -31,7 +31,6 @@ class Control(runnable.Runnable):
         self.routing = Routing(routing or {}, default or {}, python_path)
 
     def set_project(self, project):
-        self.project = project
         self.pre_routing.set_project(project)
         self.routing.set_project(project)
 
@@ -89,13 +88,6 @@ class Control(runnable.Runnable):
     def __bool__(self):
         return bool(self.routing or self.pre_routing)
 
-    if deprecated.allowed():
-        set_root = set_project
-
-        @property
-        def root(self):
-            return self.project
-
 
 class ControlLoop:
     """Mixin class for looping controls"""
diff --git a/bibliopixel/control/editor.py b/bibliopixel/control/editor.py
index 0d442ce..32ff00b 100644
--- a/bibliopixel/control/editor.py
+++ b/bibliopixel/control/editor.py
@@ -1,3 +1,4 @@
+import weakref
 from . address import number, Address
 from . receiver import Receiver
 from .. util import deprecated
@@ -19,17 +20,15 @@ class Editor(Receiver):
             self.set_project(project)
 
     def set_project(self, project):
-        self.project = project
+        self.project = weakref.ref(project)
         self.segments = self.address.segments[:]
         self.last_segment = self.address.segments.pop()
-        edit_queue = None
+        self.edit_queue = None
 
         for segment in self.address.segments:
-            edit_queue = getattr(project, 'edit_queue', edit_queue)
+            self.edit_queue = getattr(project, 'edit_queue', self.edit_queue)
             project = segment.get(project)
 
-        self.edit_queue = edit_queue
-
     def receive(self, msg):
         """
         Receives a message, and either sets it immediately, or puts it on the
@@ -54,18 +53,12 @@ class Editor(Receiver):
         return str(self.address)
 
     def _get(self):
-        project = self.project
-        for segment in self.address.segments:
-            project = segment.get(project)
-        return project
+        project = self.project and self.project()
+        if project:
+            for segment in self.address.segments:
+                project = segment.get(project)
+            return project
 
     def _set(self, values):
         args = self.address.assignment + values
         self.last_segment.set(self._get(), *args)
-
-    if deprecated.allowed():
-        set_root = set_project
-
-        @property
-        def root(self):
-            return self.project
diff --git a/bibliopixel/control/routing.py b/bibliopixel/control/routing.py
index 46aa6ea..96bc3b3 100644
--- a/bibliopixel/control/routing.py
+++ b/bibliopixel/control/routing.py
@@ -74,10 +74,3 @@ class Routing(Receiver):
 
     def __str__(self):
         return str(self.routing)
-
-    if deprecated.allowed():
-        set_root = set_project
-
-        @property
-        def root(self):
-            return self.project
diff --git a/bibliopixel/drivers/driver_base.py b/bibliopixel/drivers/driver_base.py
index ae9eef8..92f0534 100644
--- a/bibliopixel/drivers/driver_base.py
+++ b/bibliopixel/drivers/driver_base.py
@@ -66,7 +66,6 @@ class DriverBase(object):
         self.brightness_lock = threading.Lock()
         self._brightness = 255
         self._waiting_brightness = None
-        self.time = time.time
 
     def set_pixel_positions(self, pixel_positions):
         """
@@ -95,8 +94,7 @@ class DriverBase(object):
                 end, len(self._colors)))
 
     def set_project(self, project):
-        self.project = project
-        self.time = project.time
+        self.clock = project.clock
 
     def start(self):
         """
@@ -156,7 +154,7 @@ class DriverBase(object):
         provided as a wrapper for each driver's implementation of
         :py:func:`_compute_packet` and :py:func:`_send_packet`.
         """
-        start = self.time()
+        start = self.clock.time()
 
         with self.brightness_lock:
             # Swap in a new brightness.
@@ -171,7 +169,7 @@ class DriverBase(object):
         self._compute_packet()
         self._send_packet()
 
-        self.lastUpdate = self.time() - start
+        self.lastUpdate = self.clock.time() - start
 
     def set_brightness(self, brightness):
         """Set the global brightness for this driver's output.
diff --git a/bibliopixel/drivers/dummy.py b/bibliopixel/drivers/dummy.py
index 1223aa7..855da2a 100644
--- a/bibliopixel/drivers/dummy.py
+++ b/bibliopixel/drivers/dummy.py
@@ -18,7 +18,7 @@ class Dummy(DriverBase):
     # Push new data to strand
     def _compute_packet(self):
         if self._delay > 0:
-            self.project.sleep(self._delay)
+            self.clock.sleep(self._delay)
         else:
             pass
 
diff --git a/bibliopixel/drivers/movie_writer.py b/bibliopixel/drivers/movie_writer.py
index f7afdf0..21d9aa0 100644
--- a/bibliopixel/drivers/movie_writer.py
+++ b/bibliopixel/drivers/movie_writer.py
@@ -45,7 +45,9 @@ class MovieWriter(DriverBase):
     def update_colors(self):
         super().update_colors()
         if self.movie_writer.step(self.cur_step):
-            self.project.stop()
+            project = self.movie_writer.project()
+            if project:
+                project.stop()
         else:
             self.cur_step += 1
 
diff --git a/bibliopixel/project/clock.py b/bibliopixel/project/clock.py
new file mode 100644
index 0000000..5715b2e
--- /dev/null
+++ b/bibliopixel/project/clock.py
@@ -0,0 +1,18 @@
+import time
+
+
+class Clock:
+    def __init__(self, is_flat_out=False):
+        self.is_flat_out = is_flat_out
+        self._time = time.time()
+
+    def time(self):
+        return self._time if self.is_flat_out else time.time()
+
+    def sleep(self, delta_time):
+        delta_time = max(delta_time, 0)
+        # TODO: why is delta_time occasionally tiny and negative?
+        if not self.is_flat_out:
+            time.sleep(delta_time)
+        else:
+            self._time += delta_time
diff --git a/bibliopixel/project/project.py b/bibliopixel/project/project.py
index 06f1a8d..b1151b8 100644
--- a/bibliopixel/project/project.py
+++ b/bibliopixel/project/project.py
@@ -1,6 +1,6 @@
 import os, queue, threading, time, weakref
 from . import (
-    attributes, construct, edit_queue, fill, load, merge, recurse)
+    attributes, clock, construct, edit_queue, fill, load, merge, recurse)
 from .. util import exception, log
 
 EDIT_QUEUE_MAXSIZE = 1000
@@ -57,11 +57,12 @@ class Project:
 
         self.animation = create(animation, 'animation')
         self.running = False
-        self.clock = time
+        self.clock = clock.Clock()
 
         eq = edit_queue.EditQueue(maxsize=edit_queue_maxsize)
         self.layout.edit_queue = self.animation.edit_queue = eq
         self.animation.add_preframe_callback(eq.get_and_run_edits)
+        self.cleaned_up = False
 
         # Unfortunately, the whole animation cycle is controlled by methods on
         # the topmost animation - but we need to get called back at a certain
@@ -79,6 +80,12 @@ class Project:
 
         self.animation.set_project(self)
 
+    def __del__(self):
+        try:
+            self.cleanup()
+        except:
+            pass
+
     def start(self):
         with self.LOCK:
             if self.running:
@@ -109,10 +116,12 @@ class Project:
         return True
 
     def cleanup(self):
-        self.animation.cleanup()
-        for c in self.controls:
-            c.cleanup()
-        self.layout.cleanup_drivers()
+        if not self.cleaned_up:
+            self.cleaned_up = True
+            self.animation.cleanup()
+            for c in self.controls:
+                c.cleanup()
+            self.layout.cleanup_drivers()
 
     def join(self, timeout=None):
         self.animation.join(timeout)
@@ -130,15 +139,8 @@ class Project:
             self.cleanup()
         log.debug('Project %s finishes run()', self)
 
-    def time(self):
-        return self.clock.time()
-
-    def sleep(self, delta_time):
-        # TODO: why is delta_time occasionally tiny and negative?
-        return self.clock.sleep(max(delta_time, 0))
-
-    def flat_out(self, time=0):
-        self.clock = FlatOutClock(time)
+    def flat_out(self=0):
+        self.clock.flat_out = True
 
     @staticmethod
     def stop_all():
@@ -173,15 +175,3 @@ def project(*descs, root_file=None):
     project = construct.construct(**desc)
     project.desc = desc
     return project
-
-
-class FlatOutClock:
-    def __init__(self, _time=0):
-        self._time = _time or time.time()
-
-    def time(self):
-        return self._time
-
-    def sleep(self, delta_time):
-        assert delta_time >= -MIN_TIME
-        self._time += delta_time
diff --git a/bibliopixel/util/image/movie_writer.py b/bibliopixel/util/image/movie_writer.py
index fbe7b10..80f7fb8 100644
--- a/bibliopixel/util/image/movie_writer.py
+++ b/bibliopixel/util/image/movie_writer.py
@@ -43,7 +43,7 @@ class MovieWriter:
         self.times = []
 
     def set_project(self, project):
-        self.project = project
+        self.time = project.clock.time
         self.render = renderer.renderer(project.layout, **self.render)
         assert self.render
         self.file_writer = file_writer.file_writer(self)
@@ -54,7 +54,7 @@ class MovieWriter:
             return True
 
         self.frame = cur_step / max(self.divide, 1)
-        self.times.append(self.project.time())
+        self.times.append(self.time())
 
         if self.time:
             elapsed = self.times[-1] - self.times[0]


Test patch:
diff --git a/test/bibliopixel/drivers/dummy_test.py b/test/bibliopixel/drivers/dummy_test.py
index b4e2923..26a8793 100644
--- a/test/bibliopixel/drivers/dummy_test.py
+++ b/test/bibliopixel/drivers/dummy_test.py
@@ -2,10 +2,11 @@ import argparse, time, unittest
 
 from bibliopixel.layout.strip import Strip
 from bibliopixel.drivers.dummy import Dummy
+from bibliopixel.project import clock
 
 
 def clock_only_project():
-    return argparse.Namespace(time=time.time, sleep=time.sleep)
+    return argparse.Namespace(clock=clock.Clock())
 
 
 class DummyTest(unittest.TestCase):
diff --git a/test/bibliopixel/leak_test.py b/test/bibliopixel/leak_test.py
new file mode 100644
index 0000000..2596eb8
--- /dev/null
+++ b/test/bibliopixel/leak_test.py
@@ -0,0 +1,49 @@
+import gc, sys, time, unittest, weakref
+from bibliopixel import builder
+from bibliopixel.project import project
+
+
+class TestLeaks(unittest.TestCase):
+    def test_leaks(self):
+        def _get_items():
+            items, stops = [], []
+            for creator in _CREATORS:
+                item, stop = creator()
+                items.append(item)
+                stops.append(stop)
+            [stop() for stop in stops]
+            return weakref.WeakSet(items)
+
+        items = _get_items()
+        _pause()
+        # If this next line were uncommented, it would work without all the
+        # weakrefs in the code.
+        # gc.collect()
+        self.assertEqual(list(items), [])
+
+
+def _pause():
+    time.sleep(0.1)
+
+
+def _builder_simple():
+    b = builder.Builder(shape=8, driver='dummy')
+    b.start(True)
+    _pause()
+    return b, b.stop
+
+
+def _builder_project():
+    b, stop = _builder_simple()
+    return b.project, stop
+
+
+def _project_simple():
+    desc = {'shape': 8, 'driver': 'dummy', 'run': {'threaded': True}}
+    p = project.project(desc)
+    p.start()
+    _pause()
+    return p, p.stop
+
+
+_CREATORS = _builder_simple, _builder_project, _project_simple