Andrew/error cancel cleanup

.github/workflows/ruff.yml

@@ -6,6 +6,6 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
-      - uses: chartboost/ruff-action@v1
+      - uses: astral-sh/ruff-action@v3
         with:
-          src: './choreographer'
+          src: 'choreographer'

.pre-commit-config.yaml

@@ -21,7 +21,7 @@ repos:
       - id: add-trailing-comma
   - repo: https://github.com/astral-sh/ruff-pre-commit
     #  Ruff version.
-    rev: v0.8.2
+    rev: v0.12.7
     hooks:
       #  Run the linter.
       - id: ruff

CHANGELOG.txt

@@ -1,3 +1,5 @@
+v1.0.10
+- Add locks and errors for commands called on closed browsers.
 v1.0.9
 - Serializer now accepts unsigned ints
 - Serializer better differentiates between pd/xr

choreographer/_brokers/_async.py

@@ -252,7 +252,7 @@ async def write_json(
                     self._channel.write_json,
                     obj,
                 )
-        except BaseException as e:  # noqa: BLE001
+        except Exception as e:  # noqa: BLE001
             future.set_exception(e)
             del self.futures[key]
             _logger.debug(f"Future for {key} deleted.")

choreographer/browser_async.py

@@ -32,6 +32,8 @@
 
 _logger = logistro.getLogger(__name__)
 
+# Since I added locks to pipes, do we need locks here?
+
 
 class Tab(Target):
     """A wrapper for `Target`, so user can use `Tab`, not `Target`."""
@@ -145,6 +147,8 @@ def run() -> subprocess.Popen[bytes]:
         try:
             _logger.debug("Starting watchdog")
             self._watch_dog_task = asyncio.create_task(self._watchdog())
+            _logger.debug("Opening channel.")
+            self._channel.open()  # should this and below be in a broker run
             _logger.debug("Running read loop")
             self._broker.run_read_loop()
             _logger.debug("Populating Targets")
@@ -173,6 +177,8 @@ def __await__(self) -> Generator[Any, Any, Browser]:
         return self.__aenter__().__await__()
 
     async def _is_closed(self, wait: int | None = 0) -> bool:
+        if not hasattr(self, "subprocess"):
+            return True
         if wait == 0:
             # poll returns None if its open
             _is_open = self.subprocess.poll() is None
@@ -199,6 +205,8 @@ async def _close(self) -> None:
         except ChannelClosedError:
             _logger.debug("Can't send Browser.close on close channel")
         loop = asyncio.get_running_loop()
+
+        # why in another thread?
         await loop.run_in_executor(None, self._channel.close)
 
         if await self._is_closed(wait=3):

choreographer/browser_sync.py

@@ -109,6 +109,7 @@ def open(self) -> None:
         )
         super().__init__("0", self._broker)
         self._add_session(SessionSync("", self._broker))
+        self._channel.open()
 
     def __enter__(self) -> Self:
         """Open browser as context to launch on entry and close on exit."""

choreographer/channels/_interface_type.py

@@ -34,3 +34,9 @@ def read_jsons(self, *, blocking: bool = True) -> Sequence[BrowserResponse]:
 
     def close(self) -> None:
         """Close the channel."""
+
+    def open(self) -> None:
+        """Open the channel."""
+
+    def is_ready(self) -> bool:
+        """Return true if comm channel is active."""

choreographer/channels/pipe.py

@@ -23,6 +23,8 @@
 
 _logger = logistro.getLogger(__name__)
 
+# should be closing my ends from the start?
+
 
 # if we're a pipe we expect these public attributes
 class Pipe:
@@ -57,6 +59,24 @@ def __init__(self) -> None:
 
         # this is just a convenience to prevent multiple shutdowns
         self.shutdown_lock = Lock()  # should be private
+        self._open_lock = Lock()  # should be private
+
+    def is_ready(self) -> bool:
+        """Return true if pipe open."""
+        return not self.shutdown_lock.locked() and self._open_lock.locked()
+
+    def open(self) -> None:
+        """
+        Open the channel.
+
+        In a sense, __init__ creates the pipe. The OS opens it.
+        Here we're just marking it open for use, that said.
+
+        We only use locks here for indications, we never actually lock,
+        because the broker is in charge of all async/parallel stuff.
+        """
+        if not self._open_lock.acquire(blocking=False):
+            raise RuntimeError("Cannot open same pipe twice.")
 
     def write_json(self, obj: Mapping[str, Any]) -> None:
         """
@@ -66,8 +86,11 @@ def write_json(self, obj: Mapping[str, Any]) -> None:
             obj: any python object that serializes to json.
 
         """
-        if self.shutdown_lock.locked():
-            raise ChannelClosedError
+        if not self.is_ready():
+            raise ChannelClosedError(
+                "The communication channel was either never "
+                "opened or closed. Was .open() or .close() called?",
+            )
         encoded_message = wire.serialize(obj) + b"\0"
         _logger.debug(
             f"Writing message {encoded_message[:15]!r}...{encoded_message[-15:]!r}, "
@@ -102,8 +125,11 @@ def read_jsons(  # noqa: PLR0912, PLR0915, C901 branches, complexity
             A list of jsons.
 
         """
-        if self.shutdown_lock.locked():
-            raise ChannelClosedError
+        if not self.is_ready():
+            raise ChannelClosedError(
+                "The communication channel was either never "
+                "opened or closed. Was .open() or .close() called?",
+            )
         if not _with_block and not blocking:
             warnings.warn(  # noqa: B028
                 "Windows python version < 3.12 does not support non-blocking",
@@ -173,20 +199,20 @@ def _unblock_fd(self, fd: int) -> None:
         try:
             if _with_block:
                 os.set_blocking(fd, False)
-        except BaseException:  # noqa: BLE001, S110 OS errors are not consistent, catch blind + pass
+        except Exception:  # noqa: BLE001, S110 OS errors are not consistent, catch blind + pass
             pass
 
     def _close_fd(self, fd: int) -> None:
         try:
             os.close(fd)
-        except BaseException:  # noqa: BLE001, S110 OS errors are not consistent, catch blind + pass
+        except Exception:  # noqa: BLE001, S110 OS errors are not consistent, catch blind + pass
             pass
 
     def _fake_bye(self) -> None:
         self._unblock_fd(self._write_from_browser)
         try:
             os.write(self._write_from_browser, b"{bye}\n")
-        except BaseException:  # noqa: BLE001, S110 OS errors are not consistent, catch blind + pass
+        except Exception:  # noqa: BLE001, S110 OS errors are not consistent, catch blind + pass
             pass
 
     def close(self) -> None:

choreographer/cli/_cli_utils_no_qa.py

@@ -10,6 +10,7 @@
 # so lets give diagnose a separate file
 
 # ruff: noqa: PLR0915, C901, S603, BLE001, S607, PERF203, TRY002, T201, PLR0912, SLF001
+# ruff: noqa: PLC0415
 # ruff: noqa: F401, ERA001 # temporary, sync
 
 # in order, exceptions are:
@@ -66,12 +67,12 @@ def diagnose() -> None:
     try:
         print("PIP:".center(25, "*"))
         print(subprocess.check_output([sys.executable, "-m", "pip", "freeze"]).decode())
-    except BaseException as e:
+    except Exception as e:
         print(f"Error w/ pip: {e}")
     try:
         print("UV:".center(25, "*"))
         print(subprocess.check_output(["uv", "pip", "freeze"]).decode())
-    except BaseException as e:
+    except Exception as e:
         print(f"Error w/ uv: {e}")
     try:
         print("GIT:".center(25, "*"))
@@ -80,7 +81,7 @@ def diagnose() -> None:
                 ["git", "describe", "--tags", "--long", "--always"],
             ).decode(),
         )
-    except BaseException as e:
+    except Exception as e:
         print(f"Error w/ git: {e}")
     finally:
         print(sys.version)
@@ -94,7 +95,7 @@ def diagnose() -> None:
             # browser.open()
             # time.sleep(3)
             # browser.close()
-        except BaseException as e:
+        except Exception as e:
             fail.append(("Sync test headless", e))
         finally:
             print("Done with sync test headless".center(50, "*"))
@@ -107,7 +108,7 @@ async def test_headless() -> None:
         try:
             print("Async Test Headless".center(50, "*"))
             asyncio.run(test_headless())
-        except BaseException as e:
+        except Exception as e:
             fail.append(("Async test headless", e))
         finally:
             print("Done with async test headless".center(50, "*"))
@@ -125,8 +126,8 @@ async def test_headless() -> None:
                     exception[1],
                     exception[1].__traceback__,
                 )
-            except BaseException:
+            except Exception:
                 print("Couldn't print traceback for:")
                 print(str(exception))
-        raise BaseException("There was an exception, see above.")
+        raise Exception("There was an exception, see above.")
     print("Thank you! Please share these results with us!")

choreographer/utils/_tmpfile.py

@@ -107,7 +107,7 @@ def _delete_manually(  # noqa: C901, PLR0912
                         fp.chmod(stat.S_IWUSR)
                         fp.unlink(missing_ok=True)
                         _logger.debug2("Deleted")
-                    except BaseException as e:  # noqa: BLE001 yes catch and report
+                    except Exception as e:  # noqa: BLE001 yes catch and report
                         errors.append((fp, e))
                 for d in dirs:
                     fp = Path(root) / d
@@ -116,15 +116,15 @@ def _delete_manually(  # noqa: C901, PLR0912
                         fp.chmod(stat.S_IWUSR)
                         fp.rmdir()
                         _logger.debug2("Deleted")
-                    except BaseException as e:  # noqa: BLE001 yes catch and report
+                    except Exception as e:  # noqa: BLE001 yes catch and report
                         errors.append((fp, e))
 
             # clean up directory
         if not check_only:
             try:
                 self.path.chmod(stat.S_IWUSR)
                 self.path.rmdir()
-            except BaseException as e:  # noqa: BLE001 yes catch and report
+            except Exception as e:  # noqa: BLE001 yes catch and report
                 errors.append((self.path, e))
 
         if check_only:
@@ -154,7 +154,7 @@ def clean(self) -> None:  # noqa: C901
                 self.temp_dir.cleanup()
             self.exists = False
             _logger.info("TemporaryDirectory.cleanup() worked.")
-        except BaseException as e:  # noqa: BLE001 we try many ways to clean, this is the first one
+        except Exception as e:  # noqa: BLE001 we try many ways to clean, this is the first one
             _logger.info(f"TemporaryDirectory.cleanup() failed. Error {e}")
 
         # bad typing but tough
@@ -183,7 +183,7 @@ def remove_readonly(
             if hasattr(self, "temp_dir"):
                 del self.temp_dir
             _logger.info("shutil.rmtree worked.")
-        except BaseException as e:  # noqa: BLE001
+        except Exception as e:  # noqa: BLE001
             _logger.debug("Error during tmp file removal.", exc_info=e)
             self._delete_manually(check_only=True)
             if not self.exists:

choreographer/utils/_which.py

@@ -25,8 +25,8 @@ def _is_exe(path: str | Path) -> bool:
 
 def _which_from_windows_reg() -> str | None:
     try:
-        import re
-        import winreg
+        import re  # noqa: PLC0415 specific to win
+        import winreg  # noqa: PLC0415 specific to win
 
         command = winreg.QueryValueEx(  # type: ignore [attr-defined]
             winreg.OpenKey(  # type: ignore [attr-defined]
@@ -38,7 +38,7 @@ def _which_from_windows_reg() -> str | None:
             "",
         )[0]
         exe = re.search('"(.*?)"', command).group(1)  # type: ignore [union-attr]
-    except BaseException:  # noqa: BLE001 don't care why, best effort search
+    except Exception:  # noqa: BLE001 don't care why, best effort search
         return None
 
     return exe

pyproject.toml

@@ -64,6 +64,9 @@ dev = [
 #mkquixote = { path = "../mkquixote", editable = true }
 #logistro = { path = "../logistro", editable = true }
 
+[tool.ruff]
+src = ["choreographer"]
+
 [tool.ruff.lint]
 select = ["ALL"]
 ignore = [