Merge pull request #91 from plotly/andrew/close_refactor

 ⚠️⚠️⚠️ Andrew/close refactor - DO FIRST!

devtools/browser.py

@@ -204,9 +204,7 @@ async def _open_async(self):
         await self.populate_targets()
         self.future_self.set_result(self)
 
-    # Closers: close() calls sync or async, both call finish_close
-
-    def finish_close(self):
+    def _clean_temp(self):
         clean = False
         try:
             self.temp_dir.cleanup()
@@ -246,93 +244,118 @@ def remove_readonly(func, path, excinfo):
         if self.debug:
             print(f"Tempfile still exists?: {bool(os.path.isfile(str(self.temp_dir.name)))}")
 
-    def sync_process_close(self):
-        self.send_command("Browser.close")
+    async def _is_closed_async(self, wait=0):
+        waiter = self.subprocess.wait()
         try:
-            self.subprocess.wait(3)
-            self.pipe.close()
+            await asyncio.wait_for(waiter, wait)
+            return True
+        except: # noqa
+            return False
+
+    def _is_closed(self, wait=0):
+        if not wait:
+            if not self.subprocess.poll():
+                return False
+            else:
+                return True
+        else:
+            try:
+                self.subprocess.wait(wait)
+                return True
+            except: # noqa
+                return False
+
+    # _sync_close and _async_close are basically the same thing
+
+    def _sync_close(self):
+        if self._is_closed():
+            if self.debug: print("Browser was already closed.", file=sys.stderr)
+            return
+        # check if no sessions or targets
+        self.send_command("Browser.close")
+        if self._is_closed():
+            if self.debug: print("Browser.close method closed browser", file=sys.stderr)
             return
-        except Exception:
-            pass
         self.pipe.close()
+        if self._is_closed(wait = 1):
+            if self.debug: print("pipe.close() (or slow Browser.close) method closed browser", file=sys.stderr)
+            return
+
+        # Start a kill
         if platform.system() == "Windows":
-            if self.subprocess.poll() is None:
+            if not self._is_closed():
                 subprocess.call(
-                    ["taskkill", "/F", "/T", "/PID", str(self.subprocess.pid)]
-                )  # TODO probably needs to be silenced
-                try:
-                    self.subprocess.wait(2)
+                    ["taskkill", "/F", "/T", "/PID", str(self.subprocess.pid)],
+                    stderr=subprocess.DEVNULL,
+                    stdout=subprocess.DEVNULL,
+                )
+                if self._is_closed(wait = 2):
                     return
-                except Exception:
-                    pass
-            else:
+                else:
+                    raise RuntimeError("Couldn't kill browser subprocess")
+        else:
+            self.subprocess.terminate()
+            if self._is_closed():
+                if self.debug: print("terminate() closed the browser", file=sys.stderr)
                 return
-        self.subprocess.terminate()
-        try:
-            self.subprocess.wait(2)
-            return
-        except Exception:
-            pass
-        self.subprocess.kill()
 
+            self.subprocess.kill()
+            if self._is_closed():
+                if self.debug: print("kill() closed the browser", file=sys.stderr)
+        return
 
-    async def async_process_close(self):
-        await self.send_command("Browser.close")
-        waiter = self.subprocess.wait()
-        try:
-            await asyncio.wait_for(waiter, 3)
-            self.finish_close()
-            self.pipe.close()
+
+    async def _async_close(self):
+        if await self._is_closed_async():
+            if self.debug: print("Browser was already closed.", file=sys.stderr)
+            return
+        # TODO: Above doesn't work with closed tabs for some reason
+        # TODO: check if tabs?
+        # TODO: track tabs?
+        await asyncio.wait([self.send_command("Browser.close")], timeout=1)
+        if await self._is_closed_async():
+            if self.debug: print("Browser.close method closed browser", file=sys.stderr)
             return
-        except Exception:
-            pass
         self.pipe.close()
+        if await self._is_closed_async(wait=1):
+            if self.debug: print("pipe.close() method closed browser", file=sys.stderr)
+            return
+
+        # Start a kill
         if platform.system() == "Windows":
-            waiter = self.subprocess.wait()
-            try:
-                await asyncio.wait_for(waiter, 1)
-                self.finish_close()
-                return
-            except Exception:
-                pass
-            # need try
-            subprocess.call(
-                ["taskkill", "/F", "/T", "/PID", str(self.subprocess.pid)]
-            )  # TODO probably needs to be silenced
-            waiter = self.subprocess.wait()
-            try:
-                await asyncio.wait_for(waiter, 2)
-                self.finish_close()
+            if not await self._is_closed_async():
+                subprocess.call(
+                    ["taskkill", "/F", "/T", "/PID", str(self.subprocess.pid)],
+                    stderr=subprocess.DEVNULL,
+                    stdout=subprocess.DEVNULL,
+                )
+                if await self._is_closed_async(wait = 2):
+                    return
+                else:
+                    raise RuntimeError("Couldn't kill browser subprocess")
+        else:
+            self.subprocess.terminate()
+            if await self._is_closed_async():
+                if self.debug: print("terminate() closed the browser", file=sys.stderr)
                 return
-            except Exception:
-                pass
-        self.subprocess.terminate()
-        waiter = self.subprocess.wait()
-        try:
-            await asyncio.wait_for(waiter, 2)
-            self.finish_close()
-            return
-        except Exception:
-            pass
-        self.subprocess.kill()
+
+            self.subprocess.kill()
+            if await self._is_closed_async():
+                if self.debug: print("kill() closed the browser", file=sys.stderr)
+        return
+
 
     def close(self):
         if self.loop:
-            if not len(self.tabs):
+            async def close_task():
+                await self._async_close()
                 self.pipe.close()
-                self.finish_close()
-                future = self.loop.create_future()
-                future.set_result(None)
-                return future
-            else:
-                return asyncio.create_task(self.async_process_close())
-
+                self._clean_temp() # can we make async
+            return asyncio.create_task(close_task())
         else:
-            if self.subprocess.poll() is None:
-                self.sync_process_close()
-                # I'd say race condition but the user needs to take care of it
-            self.finish_close()
-    # These are effectively stubs to allow use with with
+            self._sync_close()
+            self.pipe.close()
+            self._clean_temp()
 
     def __enter__(self):
         return self

devtools/chrome_wrapper.py

@@ -31,7 +31,7 @@
 elif system == "Linux":
     default_path = "/usr/bin/google-chrome-stable"
 else: # assume mac, or system == "Darwin"
-    default_path = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" 
+    default_path = "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome"
 
 def open_browser(to_chromium, from_chromium, stderr=None, env=None, loop=None, loop_hack=False):
     path = env.get("BROWSER_PATH", default_path)
@@ -62,12 +62,11 @@ def open_browser(to_chromium, from_chromium, stderr=None, env=None, loop=None, l
             f"--remote-debugging-io-pipes={str(to_chromium_handle)},{str(from_chromium_handle)}"
         ]
         if platform.system() == "Windows":
-            win_only = {"creationflags": subprocess.CREATE_NEW_PROCESS_GROUP}
+            win_only = {"creationflags": subprocess.CREATE_NEW_PROCESS_GROUP, "close_fds":False}
     if not loop:
         return subprocess.Popen(
             cli,
             stderr=stderr,
-            close_fds=False, # TODO sh/could be true?
             pass_fds=(to_chromium, from_chromium) if system != "Windows" else None,
             **win_only,
         )
@@ -76,7 +75,6 @@ def run():
             return subprocess.Popen(
                 cli,
                 stderr=stderr,
-                close_fds=False, # TODO sh/could be true?
                 pass_fds=(to_chromium, from_chromium) if system != "Windows" else None,
                 **win_only,
             )
@@ -86,7 +84,6 @@ def run():
                 cli[0],
                 *cli[1:],
                 stderr=stderr,
-                close_fds=False, # TODO: sh/could be true?
                 pass_fds=(to_chromium, from_chromium) if system != "Windows" else None,
                 **win_only)
 

devtools/pipe.py

@@ -54,7 +54,10 @@ def read_jsons(self, blocking=True, debug=None):
         if debug:
             print(f"read_jsons ({'blocking' if blocking else 'not blocking'}):", file=sys.stderr)
         jsons = []
-        if with_block: os.set_blocking(self.read_from_chromium, blocking)
+        try:
+            if with_block: os.set_blocking(self.read_from_chromium, blocking)
+        except OSError as e:
+            raise PipeClosedError() from e
         try:
             raw_buffer = os.read(
                 self.read_from_chromium, 10000
@@ -71,6 +74,14 @@ def read_jsons(self, blocking=True, debug=None):
             if debug:
                 print("read_jsons: BlockingIOError caught.", file=sys.stderr)
             return jsons
+        except OSError as e:
+            if debug:
+                print(f"caught OSError in read() {str(e)}", file=sys.stderr)
+            if not raw_buffer:
+                raise PipeClosedError()
+            # TODO this could be hard to test as it is a real OS corner case
+            # but possibly raw_buffer is partial
+            # and we don't check for partials
         decoded_buffer = raw_buffer.decode("utf-8")
         for raw_message in decoded_buffer.split("\0"):
             if raw_message:
@@ -81,14 +92,36 @@ def read_jsons(self, blocking=True, debug=None):
                     print(f"read_jsons: {jsons[-1]}", file=sys.stderr)
         return jsons
 
+    def _unblock_fd(self, fd):
+        try:
+            if with_block: os.set_blocking(fd, False)
+        except BaseException as e:
+            if self.debug:
+                print(f"Expected error unblocking {str(fd)}: {str(e)}", file=sys.stderr)
+
+    def _close_fd(self, fd):
+        try:
+            os.close(fd)
+        except BaseException as e:
+            if self.debug:
+                print(f"Expected error closing {str(fd)}: {str(e)}", file=sys.stderr)
+
+    def _fake_bye(self):
+        self._unblock_fd(self.write_from_chromium)
+        try:
+            os.write(self.write_from_chromium, b'{bye}\n')
+        except BaseException as e:
+            if self.debug:
+                print(f"Caught expected error in self-wrte bye: {str(e)}", file=sys.stderr)
+
     def close(self):
         if platform.system() == "Windows":
-            try:
-                if with_block: os.set_blocking(self.write_from_chromium, False)
-                os.write(self.write_from_chromium, b'{bye}\n')
-            except Exception:
-                pass
-        os.close(self.write_to_chromium)
-        os.close(self.read_from_chromium)
-        os.close(self.write_from_chromium)
-        os.close(self.read_to_chromium)
+            self._fake_bye()
+        self._unblock_fd(self.write_from_chromium)
+        self._unblock_fd(self.read_from_chromium)
+        self._unblock_fd(self.write_to_chromium)
+        self._unblock_fd(self.read_to_chromium)
+        self._close_fd(self.write_to_chromium)
+        self._close_fd(self.read_from_chromium)
+        self._close_fd(self.write_from_chromium)
+        self._close_fd(self.read_to_chromium)