diff --git a/.github/workflows/ruff.yml b/.github/workflows/ruff.yml index 3c58e4a2..5cd857d3 100644 --- a/.github/workflows/ruff.yml +++ b/.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' diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9bf923b4..67024898 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/CHANGELOG.txt b/CHANGELOG.txt index cd9b4b4a..56e8916b 100644 --- a/CHANGELOG.txt +++ b/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 diff --git a/choreographer/_brokers/_async.py b/choreographer/_brokers/_async.py index 65d92b73..49c8bfe0 100644 --- a/choreographer/_brokers/_async.py +++ b/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.") diff --git a/choreographer/browser_async.py b/choreographer/browser_async.py index 09ff4683..7f92a341 100644 --- a/choreographer/browser_async.py +++ b/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): diff --git a/choreographer/browser_sync.py b/choreographer/browser_sync.py index dfb37d5b..d04873d2 100644 --- a/choreographer/browser_sync.py +++ b/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.""" diff --git a/choreographer/channels/_interface_type.py b/choreographer/channels/_interface_type.py index 8879db47..10854fbc 100644 --- a/choreographer/channels/_interface_type.py +++ b/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.""" diff --git a/choreographer/channels/pipe.py b/choreographer/channels/pipe.py index a7ca6d07..5326bfba 100644 --- a/choreographer/channels/pipe.py +++ b/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: diff --git a/choreographer/cli/_cli_utils_no_qa.py b/choreographer/cli/_cli_utils_no_qa.py index 42917f1d..4df8b501 100644 --- a/choreographer/cli/_cli_utils_no_qa.py +++ b/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!") diff --git a/choreographer/utils/_tmpfile.py b/choreographer/utils/_tmpfile.py index a84439a3..f27928ad 100644 --- a/choreographer/utils/_tmpfile.py +++ b/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,7 +116,7 @@ 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 @@ -124,7 +124,7 @@ def _delete_manually( # noqa: C901, PLR0912 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: diff --git a/choreographer/utils/_which.py b/choreographer/utils/_which.py index 4510dd48..c2349fae 100644 --- a/choreographer/utils/_which.py +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index 141aa44b..24b1f32f 100644 --- a/pyproject.toml +++ b/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 = [