Make default stderr sys.stderr, not passthrough

.github/workflows/test.yml

@@ -83,13 +83,13 @@ jobs:
       - name: Test Process Control
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rFe --capture=fd tests/test_process.py
-        timeout-minutes: 3
+        timeout-minutes: 4
       - name: Test The Rest DEBUG
         if: runner.debug
         run:  pytest -W error -vvv -rA --capture=tee-sys --ignore=tests/test_process.py
         timeout-minutes: 5
       - name: Test The Rest
         if: ${{ ! runner.debug }}
         run:  pytest -W error -n auto -v -rfE --ignore=tests/test_process.py
-        timeout-minutes: 2
+        timeout-minutes: 3
 

README.md

@@ -148,3 +148,5 @@ We will document these as the API stabilizes.
 [devtools-ref]: https://chromedevtools.github.io/devtools-protocol/
 [kaleido]: https://pypi.org/project/kaleido/
 [puppeteer]: https://pptr.dev/
+
+

choreographer/browser.py

@@ -2,6 +2,7 @@
 import os
 from pathlib import Path
 import sys
+import io
 import subprocess
 import time
 import tempfile
@@ -55,33 +56,65 @@ def __init__(
         loop=None,
         executor=None,
         debug=False,
-        debug_browser=None,
+        debug_browser=False,
         **kwargs
     ):
         # Configuration
         self.enable_gpu = kwargs.pop("enable_gpu", False)
         self.enable_sandbox = kwargs.pop("enable_sandbox", False)
+        self._tmpdir_path = kwargs.pop("tmp_path", None)
         if len(kwargs):
             raise ValueError(f"Unknown keyword arguments: {kwargs}")
         self.headless = headless
         self.debug = debug
         self.loop_hack = False # subprocess needs weird stuff w/ SelectorEventLoop
 
         # Set up stderr
-        if not debug_browser:  # false o None
+        if debug_browser is False:  # false o None
             stderr = subprocess.DEVNULL
         elif debug_browser is True:
-            stderr = None
+            stderr = sys.stderr
         else:
             stderr = debug_browser
+
+        if ( stderr
+            and stderr not in ( subprocess.PIPE, subprocess.STDOUT, subprocess.DEVNULL )
+            and not isinstance(stderr, int) ):
+            try: stderr.fileno()
+            except io.UnsupportedOperation:
+                warnings.warn("A value has been passed to debug_browser which is not compatible with python. The default value if deug_browser is True is whatever the value of sys.stderr is. sys.stderr may be many things but debug_browser must be a value Popen accepts for stderr, or True.")
+
+
+
         self._stderr = stderr
+
         if debug:
             print(f"STDERR: {stderr}", file=sys.stderr)
 
-        # Set up temp dir
-        if platform.system() == "Linux":
+
+        # Set up process env
+        new_env = os.environ.copy()
+
+        if not path: # use argument first
+            path = os.environ.get("BROWSER_PATH", None)
+        if not path:
+            path = default_path
+        if path:
+            new_env["BROWSER_PATH"] = str(path)
+        else:
+            raise BrowserFailedError(
+                "Could not find an acceptable browser. Please set environmental variable BROWSER_PATH or pass `path=/path/to/browser` into the Browser() constructor."
+            )
+
+        if self._tmpdir_path:
+            temp_args = dict(dir=self._tmpdir_path)
+        elif "snap" in path:
+            self._tmpdir_path = Path.home()
+            if self.debug:
+                print("Snap detected, moving tmp directory to home", file=sys.stderr)
             temp_args = dict(prefix=".choreographer-", dir=Path.home())
         else:
+            self._snap = False
             temp_args = {}
         if platform.system() != "Windows":
             self.temp_dir = tempfile.TemporaryDirectory(**temp_args)
@@ -97,27 +130,16 @@ def __init__(
                 )
             else:
                 self.temp_dir = tempfile.TemporaryDirectory(**temp_args)
-
-        # Set up process env
-        new_env = os.environ.copy()
-
-        if not path: # use argument first
-            path = os.environ.get("BROWSER_PATH", None)
-        if not path:
-            path = default_path
-        if path:
-            new_env["BROWSER_PATH"] = str(path)
-        else:
-            raise BrowserFailedError(
-                "Could not find an acceptable browser. Please set environmental variable BROWSER_PATH or pass `path=/path/to/browser` into the Browser() constructor."
-            )
+        self._temp_dir_name = self.temp_dir.name
+        if self.debug:
+            print(f"TEMP DIR NAME: {self._temp_dir_name}", file=sys.stderr)
 
         if self.enable_gpu:
             new_env["GPU_ENABLED"] = "true"
         if self.enable_sandbox:
             new_env["SANDBOX_ENABLED"] = "true"
 
-        new_env["USER_DATA_DIR"] = str(self.temp_dir.name)
+        new_env["USER_DATA_DIR"] = str(self._temp_dir_name)
 
         if headless:
             new_env["HEADLESS"] = "--headless"  # unset if false
@@ -203,11 +225,22 @@ def _open(self):
 
 
     async def _watchdog(self):
+        self._watchdog_healthy = True
         if self.debug: print("Starting watchdog", file=sys.stderr)
         await self.subprocess.wait()
+        if self.lock.locked(): return # it was locked and closed
+        self._watchdog_healthy = False
         if self.debug:
             print("Browser is being closed because chrom* closed", file=sys.stderr)
         await self.close()
+        await asyncio.sleep(1)
+        with warnings.catch_warnings():
+            # we'll ignore warnings here because
+            # if the user sloppy-closes the browsers
+            # they may leave processes up still trying to create temporary files
+            warnings.filterwarnings("ignore", category=TempDirWarning)
+            self._retry_delete_manual(self._temp_dir_name, delete=True)
+
 
 
     async def _open_async(self):
@@ -242,7 +275,9 @@ async def _open_async(self):
 
     def _retry_delete_manual(self, path, delete=False):
         if not os.path.exists(path):
-            return 0, 0, [(path, FileNotFoundError("Supplied path doesn't exist"))]
+            if self.debug:
+                print("No retry delete manual necessary, path doesn't exist", file=sys.stderr)
+            return 0, 0, []
         n_dirs = 0
         n_files = 0
         errors = []
@@ -252,16 +287,22 @@ def _retry_delete_manual(self, path, delete=False):
             if delete:
                 for f in files:
                     fp = os.path.join(root, f)
+                    if self.debug:
+                        print(f"Removing file: {fp}", file=sys.stderr)
                     try:
                         os.chmod(fp, stat.S_IWUSR)
                         os.remove(fp)
+                        if self.debug: print("Success", file=sys.stderr)
                     except BaseException as e:
                         errors.append((fp, e))
                 for d in dirs:
                     fp = os.path.join(root, d)
+                    if self.debug:
+                        print(f"Removing dir: {fp}", file=sys.stderr)
                     try:
                         os.chmod(fp, stat.S_IWUSR)
                         os.rmdir(fp)
+                        if self.debug: print("Success", file=sys.stderr)
                     except BaseException as e:
                         errors.append((fp, e))
             # clean up directory
@@ -278,46 +319,41 @@ def _retry_delete_manual(self, path, delete=False):
         return n_dirs, n_files, errors
 
     def _clean_temp(self):
-        name = self.temp_dir.name
+        name = self._temp_dir_name
         clean = False
         try:
+            # no faith in this python implementation, always fails with windows
+            # very unstable recently as well, lots new arguments in tempfile package
             self.temp_dir.cleanup()
             clean=True
         except BaseException as e:
-            if platform.system() == "Windows":
-                if self.debug:
-                    print(f"TempDirWarning: {str(e)}", file=sys.stderr)
-            else:
-                warnings.warn(str(e), TempDirWarning)
+            if self.debug:
+                print(f"First tempdir deletion failed: TempDirWarning: {str(e)}", file=sys.stderr)
 
-        # windows+old vers doesn't like python's default cleanup
 
         def remove_readonly(func, path, excinfo):
             os.chmod(path, stat.S_IWUSR)
             func(path)
 
         try:
             if with_onexc:
-                shutil.rmtree(self.temp_dir.name, onexc=remove_readonly)
+                shutil.rmtree(self._temp_dir_name, onexc=remove_readonly)
                 clean=True
             else:
-                shutil.rmtree(self.temp_dir.name, onerror=remove_readonly)
+                shutil.rmtree(self._temp_dir_name, onerror=remove_readonly)
                 clean=True
             del self.temp_dir
         except FileNotFoundError:
             pass # it worked!
         except BaseException as e:
+            if self.debug:
+                print(f"Second tmpdir deletion failed (shutil.rmtree): {str(e)}", file=sys.stderr)
             if not clean:
-                if platform.system() == "Windows":
-                    def extra_clean():
-                        time.sleep(5)
-                        self._retry_delete_manual(name, delete=True)
-                    t = Thread(target=extra_clean)
-                    t.run()
-                else:
-                    warnings.warn(
-                            f"The temporary directory could not be deleted, execution will continue. {type(e)}: {e}", TempDirWarning
-                    )
+                def extra_clean():
+                    time.sleep(2)
+                    self._retry_delete_manual(name, delete=True)
+                t = Thread(target=extra_clean)
+                t.run()
         if self.debug:
             print(f"Tempfile still exists?: {bool(os.path.exists(str(name)))}", file=sys.stderr)
 

choreographer/chrome_wrapper.py

@@ -1,23 +1,22 @@
 import os
 
 # importing modules has side effects, so we do this before imports
-# chromium reads on 3, writes on 4
 # linter complains.
 env = None
 
+
+# chromium reads on 3, writes on 4
 # really this means windows only, but a more readable platform.system()
-# needs to come later for the above reasons
-# windows will import, not run as separate process
 if __name__ == "__main__":
     os.dup2(0, 3)  # make our stdin their input
     os.dup2(1, 4)  # make our stdout their output
 
 
-
 import subprocess  # noqa
 import signal  # noqa
 import platform  # noqa
 import asyncio #noqa
+import sys #noqa
 
 system = platform.system()
 if system == "Windows":
@@ -26,7 +25,7 @@
     os.set_inheritable(4, True)
     os.set_inheritable(3, True)
 
-def open_browser(to_chromium, from_chromium, stderr=None, env=None, loop=None, loop_hack=False):
+def open_browser(to_chromium, from_chromium, stderr=sys.stderr, env=None, loop=None, loop_hack=False):
     path = env.get("BROWSER_PATH")
     if not path:
         raise RuntimeError("No browser path was passed to run")

tests/conftest.py

@@ -1,4 +1,5 @@
 import asyncio
+import os
 
 import pytest
 import pytest_asyncio
@@ -44,22 +45,29 @@ async def browser(request):
     # this needs also to be set by command line TODO
     headless = request.config.getoption("--headless")
     debug = request.config.get_verbosity() > 2
+    debug_browser = None if debug else False
     browser = await choreo.Browser(
-        headless=headless, debug=debug, debug_browser=debug
+        headless=headless, debug=debug, debug_browser=debug_browser
     )
+    temp_dir = browser._temp_dir_name
     yield browser
     try:
         await browser.close()
     except choreo.browser.BrowserClosedError:
         pass
+    if os.path.exists(temp_dir):
+        raise RuntimeError(f"Temporary directory not deleted successfully: {temp_dir}")
 
 
 
 @pytest_asyncio.fixture(scope="function", loop_scope="function")
 async def browser_verbose():
     browser = await choreo.Browser(debug=True, debug_browser=True)
+    temp_dir = browser._temp_dir_name
     yield browser
     await browser.close()
+    if os.path.exists(temp_dir):
+        raise RuntimeError(f"Temporary directory not deleted successfully: {temp_dir}")
 
 # add a 2 second timeout if there is a browser fixture
 # we do timeouts manually in test_process because it

tests/test_process.py

@@ -14,10 +14,11 @@ async def test_context(capteesys, headless, debug, debug_browser, sandbox, gpu):
     async with choreo.Browser(
         headless=headless,
         debug=debug,
-        debug_browser=debug_browser,
+        debug_browser=None if debug_browser else False,
         enable_sandbox=sandbox,
         enable_gpu=gpu
     ) as browser, timeout(pytest.default_timeout):
+        temp_dir = browser._temp_dir_name
         response = await browser.send_command(command="Target.getTargets")
         assert "result" in response and "targetInfos" in response["result"]
         assert (len(response["result"]["targetInfos"]) != 0) != headless
@@ -29,16 +30,18 @@ async def test_context(capteesys, headless, debug, debug_browser, sandbox, gpu):
     assert capteesys.readouterr().out == "\n", "stdout should be silent!"
     # let asyncio do some cleaning up if it wants, may prevent warnings
     await asyncio.sleep(0)
+    assert not os.path.exists(temp_dir)
 
 @pytest.mark.asyncio(loop_scope="function")
 async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gpu):
     browser = await choreo.Browser(
         headless=headless,
         debug=debug,
-        debug_browser=debug_browser,
+        debug_browser=None if debug_browser else False,
         enable_sandbox=sandbox,
         enable_gpu=gpu
     )
+    temp_dir = browser._temp_dir_name
     try:
         async with timeout(pytest.default_timeout):
             response = await browser.send_command(command="Target.getTargets")
@@ -49,18 +52,20 @@ async def test_no_context(capteesys, headless, debug, debug_browser, sandbox, gp
                 assert len(browser.get_tab().sessions) == 1
     finally:
         await browser.close()
-        print("") # this make sure that capturing is working
-        assert capteesys.readouterr().out == "\n", "stdout should be silent!"
-        await asyncio.sleep(0)
+    print("") # this make sure that capturing is working
+    assert capteesys.readouterr().out == "\n", "stdout should be silent!"
+    await asyncio.sleep(0)
+    assert not os.path.exists(temp_dir)
 
 # we're basically just harrassing choreographer with a kill in this test to see if it behaves well
 @pytest.mark.asyncio(loop_scope="function")
 async def test_watchdog(capteesys, headless, debug, debug_browser):
     browser = await choreo.Browser(
         headless=headless,
         debug=debug,
-        debug_browser=debug_browser,
+        debug_browser=None if debug_browser else False,
     )
+    #temp_dir = browser._temp_dir_name
     #async with timeout(pytest.default_timeout):
 
     if platform.system() == "Windows":
@@ -71,9 +76,11 @@ async def test_watchdog(capteesys, headless, debug, debug_browser):
         )
     else:
         os.kill(browser.subprocess.pid, signal.SIGKILL)
-    await asyncio.sleep(1)
+    await asyncio.sleep(1.5)
     with pytest.raises((choreo.browser.PipeClosedError, choreo.browser.BrowserClosedError)):
         await browser.send_command(command="Target.getTargets") # could check for error, for close
     # interestingly, try/except doesn't work here? maybe problem with pytest
 
     await browser.close()
+    await asyncio.sleep(0)
+    # assert not os.path.exists(temp_dir) # since we slopily kill the browser, we have no idea whats going to happen