fix(api): do not cache removed modules in thread_manager

Closes #5359

api/src/opentrons/drivers/temp_deck/driver.py

@@ -210,17 +210,25 @@ def update_temperature(self, default=None) -> str:
             updated_temperature = default or self._temperature.copy()
             self._temperature.update(updated_temperature)
         else:
-            # comment
-            try:
-                self._update_thread = Thread(
-                    target=self._recursive_update_temperature,
-                    args=[DEFAULT_COMMAND_RETRIES],
-                    name='Tempdeck recursive update temperature')
-                self._update_thread.start()
-            except (TempDeckError, SerialException, SerialNoResponse) as e:
-                return str(e)
+            def _update():
+                try:
+                    self._recursive_update_temperature(retries=DEFAULT_COMMAND_RETRIES)
+                except (OSError, TempDeckError, SerialException, SerialNoResponse):
+                    log.exception("UPDATE FAILED")
+
+            self._update_thread = Thread(
+                target=_update(),
+                name='Tempdeck recursive update temperature')
+            self._update_thread.start()
         return ''
 
+    def _update_temperature(self):
+        self._update_thread = Thread(
+            target=self._recursive_update_temperature,
+            args=[DEFAULT_COMMAND_RETRIES],
+            name='Tempdeck recursive update temperature')
+        self._update_thread.start()
+
     @property
     def target(self) -> Optional[int]:
         return self._temperature.get('target')

api/src/opentrons/drivers/thermocycler/driver.py

@@ -331,10 +331,12 @@ def send(self, command, callback):
             self._send_write_fd.write(b'c')
 
     def close(self):
+        log.debug("Halting TCPoller")
         self._halt_write_fd.write(b'q')
 
     def __del__(self):
         """ Clean up thread fifos"""
+        log.debug("Cleaning up thread fifos in TCPoller.")
         try:
             os.unlink(self._send_path)
         except NameError:
@@ -376,9 +378,10 @@ async def connect(self, port: str) -> 'Thermocycler':
         return self
 
     def disconnect(self) -> 'Thermocycler':
-        if self.is_connected():
+        if self.is_connected() or self._poller:
             self._poller.close()  # type: ignore
             self._poller.join()  # type: ignore
+            log.debug("TC poller stopped.")
         self._poller = None
         return self
 
@@ -627,6 +630,7 @@ async def enter_programming_mode(self):
 
     def __del__(self):
         try:
-            self._poller.close()  # type: ignore
+            if self._poller:
+                self._poller.close()  # type: ignore
         except Exception:
-            log.exception('Exception while cleaning up Thermocycler:')
+            log.exception('Exception while cleaning up Thermocycler')

api/src/opentrons/hardware_control/api.py

@@ -1733,7 +1733,7 @@ def _unregister_modules(self,
         for removed_mod in removed_modules:
             self._log.info(f"Module {removed_mod.name()} detached"
                            f" from port {removed_mod.port}")
-            del removed_mod
+            removed_mod.cleanup()
 
     async def register_modules(
             self,

api/src/opentrons/hardware_control/modules/magdeck.py

@@ -209,7 +209,7 @@ def _disconnect(self):
         if self._driver:
             self._driver.disconnect(port=self._port)
 
-    def __del__(self):
+    def cleanup(self) -> None:
         self._disconnect()
 
     async def prep_for_update(self) -> str:

api/src/opentrons/hardware_control/modules/mod_abc.py

@@ -162,3 +162,11 @@ def name(cls) -> str:
     def bootloader(cls) -> UploadFunction:
         """ Method used to upload file to this module's bootloader. """
         pass
+
+    def cleanup(self) -> None:
+        """ Clean up the module instance.
+
+        Clean up, i.e. stop pollers, disconnect serial, etc in preparation for
+        object destruction.
+        """
+        pass

api/src/opentrons/hardware_control/modules/tempdeck.py

@@ -242,8 +242,9 @@ async def _connect(self):
         self._poller = Poller(self._driver)
         self._poller.start()
 
-    def __del__(self):
-        if hasattr(self, '_poller') and self._poller:
+    def cleanup(self) -> None:
+        if self._poller:
+            log.debug("Stopping tempdeck poller.")
             self._poller.stop()
 
     async def prep_for_update(self) -> str:

api/src/opentrons/hardware_control/modules/thermocycler.py

@@ -347,3 +347,6 @@ async def prep_for_update(self):
         new_port = await update.find_bootloader_port()
 
         return new_port or self.port
+
+    def cleanup(self) -> None:
+        self._driver.disconnect()

api/src/opentrons/hardware_control/thread_manager.py

@@ -4,7 +4,7 @@
 import logging
 import asyncio
 import functools
-from typing import Generic, TypeVar, Any, Optional
+from typing import Generic, TypeVar, Any, Optional, Dict
 from .adapters import SynchronousAdapter
 from .modules.mod_abc import AbstractModule
 
@@ -108,7 +108,7 @@ def __init__(self, builder, *args, **kwargs):
         self._sync_managed_obj: Optional[SynchronousAdapter] = None
         is_running = threading.Event()
         self._is_running = is_running
-
+        self._cached_modules: Dict[AbstractModule, CallBridger[AbstractModule]] = {}
         # TODO: remove this if we switch to python 3.8
         # https://docs.python.org/3/library/asyncio-subprocess.html#subprocess-and-threads  # noqa
         # On windows, the event loop and system interface is different and
@@ -175,10 +175,9 @@ def clean_up(self):
             loop.call_soon_threadsafe(loop.stop)
         except Exception:
             pass
-        object.__getattribute__(self, 'wrap_module').cache_clear()
+        self._cached_modules = {}
         object.__getattribute__(self, '_thread').join()
 
-    @functools.lru_cache(8)
     def wrap_module(
             self, module: AbstractModule) -> CallBridger[AbstractModule]:
         return CallBridger(module, object.__getattribute__(self, '_loop'))
@@ -193,7 +192,16 @@ def __getattribute__(self, attr_name):
             wrap = object.__getattribute__(self, 'wrap_module')
             managed = object.__getattribute__(self, 'managed_obj')
             attr = getattr(managed, attr_name)
-            return [wrap(mod) for mod in attr]
+            cached_mods = object.__getattribute__(self, '_cached_modules')
+
+            # Update self._cached_modules to delete all removed modules' entries and add
+            # newly attached modules. Removing references to stale instances
+            # is necessary to allow the garbage collector to delete those objects from
+            # memory and cleanly stop all the threads associated with them.
+            cached_mods = {
+                    module: cached_mods.get(module, wrap(module)) for module in attr}
+            object.__setattr__(self, '_cached_modules', cached_mods)
+            return cached_mods.values()
         elif attr_name == 'clean_up':
             # the wrapped object probably has this attr as well as us, and we
             # want to call both, with the wrapped one first