Merge pull request #267 from plotly/andrew/further-thread-extraction

Move all process functions to new threadpool executor.

Author: ayjayt

CHANGELOG.txt

@@ -1,3 +1,10 @@
+v1.2.1
+- Use custom threadpool for functions that could be running during shutdown:
+  Python's stdlib threadpool isn't available during interpreter shutdown, nor
+  `atexit`- so they cannot be started or shutdown during `atexit`, or relied
+  upon at all. We use a custom threadpool during `Browser.close()` so that it
+  can be leveraged during atexit, and now we use it during `Browser.open()`
+  since certain use patterns have that function running during shutdown.
 - Remove site directory
 - Improve error messaging
 - Organize functions internally

ROADMAP.md

@@ -3,11 +3,10 @@
 - [ ] Working on better diagnostic information
   - [ ] Explain to user when their system has security restrictions
 - [ ] Eliminate synchronous API: it's unused, hard to maintain, and nearly
-worthless
+      worthless
 - [ ] Diagnose function should collect a JSON and then print that
 - [ ] Allow user to build and send their own JSONS
   - [ ] Get serialization out of the lock
-- [ ]
 - [ ] Support Firefox
 - [ ] Support LadyBird (!)
 - [ ] Test against multiple docker containers

src/choreographer/browser_async.py

@@ -46,6 +46,9 @@ async def close(self) -> None:
 class Browser(Target):
     """`Browser` is the async implementation of `Browser`."""
 
+    subprocess: subprocess.Popen[bytes] | subprocess.Popen[str]
+    """A reference to the `Popen` object."""
+
     tabs: MutableMapping[str, Tab]
     """A mapping by target_id of all the targets which are open tabs."""
     targets: MutableMapping[str, Target]
@@ -96,7 +99,7 @@ def __init__(
         """
         _logger.debug("Attempting to open new browser.")
 
-        self._check_closed_executor = _manual_thread_pool.ManualThreadExecutor(
+        self._process_executor = _manual_thread_pool.ManualThreadExecutor(
             max_workers=3,
             name="checking_close",
         )
@@ -144,7 +147,10 @@ def run() -> subprocess.Popen[bytes] | subprocess.Popen[str]:  # depends on args
 
         _logger.debug("Trying to open browser.")
         loop = asyncio.get_running_loop()
-        self.subprocess = await loop.run_in_executor(None, run)
+        self.subprocess = await loop.run_in_executor(
+            self._process_executor,
+            run,
+        )
 
         super().__init__("0", self._broker)
         self._add_session(Session("", self._broker))
@@ -198,7 +204,7 @@ async def _is_closed(self, wait: int | None = 0) -> bool:
                 loop = asyncio.get_running_loop()
 
                 await loop.run_in_executor(
-                    self._check_closed_executor,
+                    self._process_executor,
                     self.subprocess.wait,
                     wait,
                 )
@@ -250,6 +256,8 @@ async def close(self) -> None:
             self._watch_dog_task.cancel()
         if not self._release_lock():
             return
+        # it can never be mid open here, because all of these must
+        # run on the same thread. Do not push open or close to threads.
         try:
             _logger.debug("Starting browser close methods.")
             await self._close()
@@ -267,7 +275,7 @@ async def close(self) -> None:
         _logger.debug("Browser channel closed.")
         self._browser_impl.clean()  # os blocky/hangy across networks
         _logger.debug("Browser implementation cleaned up.")
-        self._check_closed_executor.shutdown(wait=False, cancel_futures=True)
+        self._process_executor.shutdown(wait=False, cancel_futures=True)
 
     async def __aexit__(
         self,