Repository: theolind/pymysensors

Issue:
Main program does not exit cleanly
[branch master - version 0.14.0 - Using serial gateway - NO asyncio]
After calling the SerialGateway.stop() method the program does not return to console but seems to be looping in a still alive thread ( probably the persistence thread).

************************************************************************************
```py
MYSGW_Serial_Port = '/dev/ttyMSGW'
....
GATEWAY = mysensors.SerialGateway(
MYSGW_Serial_Port, event_callback=event, persistence=True,
persistence_file='./mysensors.json', protocol_version='2.0', baud=115200,
timeout=1.0, reconnect_timeout=10.0)
GATEWAY.start_persistence()
GATEWAY.start()
....
....
GATEWAY.stop() #-> main thread does not go past this point
exit(0)
```
*************************************************************************************

Discussion:

Patch:
diff --git a/mysensors/__init__.py b/mysensors/__init__.py
index f57486b..784f988 100644
--- a/mysensors/__init__.py
+++ b/mysensors/__init__.py
@@ -49,7 +49,7 @@ class Gateway(object):
         self.metric = True  # if true - use metric, if false - use imperial
         if persistence:
             self.persistence = Persistence(
-                self.sensors, persistence_file, persistence_scheduler)
+                self.sensors, persistence_scheduler, persistence_file)
         else:
             self.persistence = None
         self.protocol_version = safe_is_version(protocol_version)
@@ -351,7 +351,8 @@ class ThreadingGateway(Gateway):
 
     def __init__(self, *args, **kwargs):
         """Set up gateway instance."""
-        super().__init__(*args, **kwargs)
+        super().__init__(
+            *args, persistence_scheduler=self._create_scheduler, **kwargs)
         self.lock = threading.Lock()
         self._stop_event = threading.Event()
         self._cancel_save = None
@@ -373,12 +374,22 @@ class ThreadingGateway(Gateway):
                 continue
             time.sleep(0.02)
 
+    def _create_scheduler(self, save_sensors):
+        """Return function to schedule saving sensors."""
+        def schedule_save():
+            """Save sensors and schedule a new save."""
+            save_sensors()
+            scheduler = threading.Timer(10.0, schedule_save)
+            scheduler.start()
+            self._cancel_save = scheduler.cancel
+        return schedule_save
+
     def start_persistence(self):
         """Load persistence file and schedule saving of persistence file."""
         if not self.persistence:
             return
         self.persistence.safe_load_sensors()
-        self._cancel_save = self.persistence.schedule_save_sensors()
+        self.persistence.schedule_save_sensors()
 
     def stop(self):
         """Stop the background thread."""
@@ -494,7 +505,7 @@ class BaseAsyncGateway(BaseTransportGateway):
         """Return function to schedule saving sensors."""
         @asyncio.coroutine
         def schedule_save():
-            """Return a function to cancel the schedule."""
+            """Save sensors and schedule a new save."""
             yield from self.loop.run_in_executor(None, save_sensors)
             callback = partial(
                 ensure_future, schedule_save(), loop=self.loop)
diff --git a/mysensors/persistence.py b/mysensors/persistence.py
index 5dd4b57..efb2e6c 100644
--- a/mysensors/persistence.py
+++ b/mysensors/persistence.py
@@ -3,35 +3,21 @@ import json
 import logging
 import os
 import pickle
-import threading
 
 from .sensor import ChildSensor, Sensor
 
 _LOGGER = logging.getLogger(__name__)
 
 
-def create_scheduler(save_sensors):
-    """Return function to schedule saving sensors."""
-    def schedule_save():
-        """Return a function to cancel the schedule."""
-        save_sensors()
-        scheduler = threading.Timer(10.0, schedule_save)
-        scheduler.start()
-        return scheduler.cancel
-    return schedule_save
-
-
 class Persistence(object):
     """Organize persistence file saving and loading."""
 
     def __init__(
-            self, sensors, persistence_file='mysensors.pickle',
-            schedule_factory=None):
+            self, sensors, schedule_factory,
+            persistence_file='mysensors.pickle'):
         """Set up Persistence instance."""
         self.persistence_file = persistence_file
         self.persistence_bak = '{}.bak'.format(self.persistence_file)
-        if schedule_factory is None:
-            schedule_factory = create_scheduler
         self.schedule_save_sensors = schedule_factory(self.save_sensors)
         self._sensors = sensors
         self.need_save = True


Test patch:
diff --git a/tests/test_gateway_mqtt.py b/tests/test_gateway_mqtt.py
index cba4270..60700fa 100644
--- a/tests/test_gateway_mqtt.py
+++ b/tests/test_gateway_mqtt.py
@@ -143,11 +143,10 @@ def test_subscribe_error(gateway, add_sensor, mock_sub, caplog):
 def test_start_stop_gateway(
         mock_save, mock_load, gateway, add_sensor, mock_pub, mock_sub):
     """Test start and stop of MQTT gateway."""
-    gateway.persistence = Persistence(gateway.sensors)
-    mock_cancel_save = mock.MagicMock()
+    mock_schedule_factory = mock.MagicMock()
     mock_schedule_save = mock.MagicMock()
-    mock_schedule_save.return_value = mock_cancel_save
-    gateway.persistence.schedule_save_sensors = mock_schedule_save
+    mock_schedule_factory.return_value = mock_schedule_save
+    gateway.persistence = Persistence(gateway.sensors, mock_schedule_factory)
     sensor = add_sensor(1)
     sensor.add_child_sensor(1, gateway.const.Presentation.S_HUM)
     sensor.children[1].values[gateway.const.SetReq.V_HUM] = '20'
@@ -173,7 +172,6 @@ def test_start_stop_gateway(
     assert mock_pub.call_count == 2
     assert mock_pub.mock_calls == calls
     gateway.stop()
-    assert mock_cancel_save.call_count == 1
     assert mock_save.call_count == 1
 
 
@@ -185,7 +183,7 @@ def test_mqtt_load_persistence(gateway, add_sensor, mock_sub, tmpdir):
 
     persistence_file = tmpdir.join('file.json')
     gateway.persistence = Persistence(
-        gateway.sensors, persistence_file.strpath)
+        gateway.sensors, mock.MagicMock(), persistence_file.strpath)
     gateway.persistence.save_sensors()
     del gateway.sensors[1]
     assert 1 not in gateway.sensors
diff --git a/tests/test_mysensors.py b/tests/test_mysensors.py
index 403ce9d..3a6c9c8 100644
--- a/tests/test_mysensors.py
+++ b/tests/test_mysensors.py
@@ -613,6 +613,28 @@ def test_gateway_low_protocol():
     assert gateway.protocol_version == '1.4'
 
 
+@mock.patch('mysensors.persistence.Persistence.save_sensors')
+@mock.patch('mysensors.threading.Timer')
+def test_threading_persistence(mock_timer_class, mock_save_sensors):
+    """Test schedule persistence on threading gateway."""
+    mock_timer_1 = mock.MagicMock()
+    mock_timer_2 = mock.MagicMock()
+    mock_timer_class.side_effect = [mock_timer_1, mock_timer_2]
+    gateway = ThreadingGateway(persistence=True)
+    gateway.persistence.schedule_save_sensors()
+    assert mock_save_sensors.call_count == 1
+    assert mock_timer_class.call_count == 1
+    assert mock_timer_1.start.call_count == 1
+    gateway.persistence.schedule_save_sensors()
+    assert mock_save_sensors.call_count == 2
+    assert mock_timer_class.call_count == 2
+    assert mock_timer_1.start.call_count == 1
+    assert mock_timer_2.start.call_count == 1
+    gateway.stop()
+    assert mock_timer_2.cancel.call_count == 1
+    assert mock_save_sensors.call_count == 3
+
+
 def test_update_fw():
     """Test calling fw_update with bad path."""
     gateway = ThreadingGateway()
diff --git a/tests/test_persistence.py b/tests/test_persistence.py
index fdf5464..c5d8896 100644
--- a/tests/test_persistence.py
+++ b/tests/test_persistence.py
@@ -45,7 +45,7 @@ def test_persistence(gateway, add_sensor, filename, tmpdir):
 
     persistence_file = tmpdir.join(filename)
     gateway.persistence = Persistence(
-        gateway.sensors, persistence_file.strpath)
+        gateway.sensors, mock.MagicMock(), persistence_file.strpath)
     gateway.persistence.save_sensors()
     del gateway.sensors[1]
     assert 1 not in gateway.sensors
@@ -75,7 +75,7 @@ def test_bad_file_name(gateway, add_sensor, tmpdir):
     add_sensor(1)
     persistence_file = tmpdir.join('file.bad')
     gateway.persistence = Persistence(
-        gateway.sensors, persistence_file.strpath)
+        gateway.sensors, mock.MagicMock(), persistence_file.strpath)
     with pytest.raises(Exception):
         gateway.persistence.save_sensors()
 
@@ -85,7 +85,7 @@ def test_json_no_files(gateway, tmpdir):
     assert not gateway.sensors
     persistence_file = tmpdir.join('file.json')
     gateway.persistence = Persistence(
-        gateway.sensors, persistence_file.strpath)
+        gateway.sensors, mock.MagicMock(), persistence_file.strpath)
     gateway.persistence.safe_load_sensors()
     assert not gateway.sensors
 
@@ -97,7 +97,7 @@ def test_empty_files(gateway, filename, tmpdir):
     assert not gateway.sensors
     persistence_file = tmpdir.join(filename)
     gateway.persistence = Persistence(
-        gateway.sensors, persistence_file.strpath)
+        gateway.sensors, mock.MagicMock(), persistence_file.strpath)
     persistence = gateway.persistence
     persistence_file.write('')
     with open(persistence.persistence_bak, 'w') as file_handle:
@@ -112,7 +112,8 @@ def test_json_empty_file_good_bak(gateway, add_sensor, tmpdir):
     assert 1 in gateway.sensors
     persistence_file = tmpdir.join('file.json')
     orig_file_name = persistence_file.strpath
-    gateway.persistence = Persistence(gateway.sensors, orig_file_name)
+    gateway.persistence = Persistence(
+        gateway.sensors, mock.MagicMock(), orig_file_name)
     gateway.persistence.save_sensors()
     del gateway.sensors[1]
     assert 1 not in gateway.sensors
@@ -160,7 +161,7 @@ def test_persistence_upgrade(
     assert 'description' not in sensor.children[0].__dict__
     persistence_file = tmpdir.join(filename)
     gateway.persistence = Persistence(
-        gateway.sensors, persistence_file.strpath)
+        gateway.sensors, mock.MagicMock(), persistence_file.strpath)
     gateway.persistence.save_sensors()
     del gateway.sensors[1]
     assert 1 not in gateway.sensors
@@ -175,16 +176,21 @@ def test_persistence_upgrade(
     assert gateway.sensors[1].children[0].type == sensor.children[0].type
 
 
-@mock.patch('mysensors.persistence.threading.Timer')
 @mock.patch('mysensors.persistence.Persistence.save_sensors')
-def test_schedule_save_sensors(mock_save, mock_timer_class, gateway):
+def test_schedule_save_sensors(mock_save, gateway):
     """Test schedule save sensors."""
-    mock_timer = mock.MagicMock()
-    mock_timer_class.return_value = mock_timer
-    gateway.persistence = Persistence(gateway.sensors)
+    mock_schedule_save = mock.MagicMock()
+    mock_schedule_factory = mock.MagicMock()
+    mock_schedule_factory.return_value = mock_schedule_save
+
+    gateway.persistence = Persistence(gateway.sensors, mock_schedule_factory)
+
+    assert mock_schedule_factory.call_count == 1
+    assert mock_schedule_factory.call_args == mock.call(mock_save)
+
     gateway.persistence.schedule_save_sensors()
-    assert mock_save.call_count == 1
-    assert mock_timer.start.call_count == 1
+
+    assert mock_schedule_save.call_count == 1
 
 
 class MySensorsJSONEncoderTestUpgrade(MySensorsJSONEncoder):