From 5b63645c895827ab45cc162051f9fef152e11e02 Mon Sep 17 00:00:00 2001 From: lread Date: Mon, 4 Jul 2022 11:48:19 -0400 Subject: [PATCH] Include driver process liveness in some exceptions When an http request, for example, times out one might wonder if the WebDriver process is still running. We now use slingshot throw+ as we do for http status errors. This throw+ still includes the :driver :process but checks if it is alive and realizes its exit value in the thrown map if dead. Also: if an exception occurs when booting a driver, any launched WebDriver process is now cleaned up. Also: reworked some proc tests to always attempt cleanup even when exceptions occur. Closes #469 --- src/etaoin/api.clj | 41 ++++--- src/etaoin/impl/client.cljc | 58 +++++++--- test/etaoin/unit/proc_test.clj | 188 +++++++++++++++++++++++++-------- 3 files changed, 211 insertions(+), 76 deletions(-) diff --git a/src/etaoin/api.clj b/src/etaoin/api.clj index fff791a7..acbaa06d 100644 --- a/src/etaoin/api.clj +++ b/src/etaoin/api.clj @@ -3621,12 +3621,23 @@ (proc/kill (:process driver)) (dissoc driver :process :args :env :capabilities)) +(defn quit + "Have `driver` close the current session, then, if Etaoin launched it, kill the WebDriver process." + [driver] + (let [process (:process driver)] + (try + (disconnect-driver driver) + (finally + (when process + (stop-driver driver)))))) + (defn boot-driver "Launch and return a driver of `type` (e.g. `:chrome`, `:firefox` `:safari` `:edge` `:phantom`) with `opts` options. - creates a driver - - launches a WebDriver process (or connects to an existing running process if `:host` is specified) + - launches a WebDriver process (or connects to an existing running process if `:host` + or `:webdriver-url` is specified) - creates a session for driver Defaults taken from [[defaults-global]] then [[defaults]] for `type`: @@ -3641,22 +3652,18 @@ (dissoc (type defaults) :capabilities)) ;; if host, we are launching webdriver, default port is random (not host) (assoc :port (util/get-free-port))) - opts (merge default-opts opts)] - (cond-> type - :always (-create-driver opts) - (and (not host) - (not webdriver-url)) (-run-driver opts) - :always (-connect-driver opts))))) - -(defn quit - "Have `driver` close the current session, then, if Etaoin launched it, kill the WebDriver process." - [driver] - (let [process (:process driver)] - (try - (disconnect-driver driver) - (finally - (when process - (stop-driver driver)))))) + opts (merge default-opts opts) + driver (cond-> (-create-driver type opts) + (and (not host) (not webdriver-url)) (-run-driver opts))] + (try + (-connect-driver driver opts) + (catch Throwable ex + (when (:process driver) + (try + (quit driver) + ;; silently ignore failure to quit driver on cleanup + (catch Throwable _ex))) + (throw ex)))))) (def ^{:arglists '([] [opts])} firefox "Launch and return a Firefox driver. diff --git a/src/etaoin/impl/client.cljc b/src/etaoin/impl/client.cljc index 0f819447..8700f00d 100644 --- a/src/etaoin/impl/client.cljc +++ b/src/etaoin/impl/client.cljc @@ -3,6 +3,7 @@ [cheshire.core :as json] [clojure.string :as str] [clojure.tools.logging :as log] + [etaoin.impl.proc :as proc] [etaoin.impl.util :as util] #?(:bb [clj-http.lite.client :as client] :clj [clj-http.client :as client]) @@ -13,9 +14,9 @@ ;; (def default-timeout - "HTTP timeout in seconds. The current value may seems to high, + "HTTP timeout in seconds. The current value may seem high, but according to my experience with SPA application full of React - modules even 20 seconds could not be enough for a driver to process + modules even 20 seconds can insufficient time for a driver to process your request." 60) @@ -70,6 +71,23 @@ (parse-json body) body)) +(defn- realized-driver + "Realize process liveness (or actually deadness, if dead)" + [{:keys [process] :as driver}] + (try + (if (and process (not (proc/alive? process))) + (assoc driver :process (proc/result process)) + driver) + (catch Throwable ex + ;; if, by chance, something goes wrong while trying to realize process liveness + (assoc driver :process-liveness-ex ex)))) + + +(defn http-request + "an isolated http-request to support mocking" + [params] + (client/request params)) + ;; ;; client ;; @@ -98,25 +116,31 @@ (-> method name str/upper-case) path (-> payload (or ""))) - resp (client/request params) - body #?(:bb (-> resp :body parse-json) - :clj (:body resp)) - error (delay {:type :etaoin/http-error - :status (:status resp) - :driver driver - :response (error-response body) + error (delay {:type :etaoin/http-ex + :driver (realized-driver driver) :webdriver-url webdriver-url :host host :port port :method method :path path - :payload payload})] - (cond - (-> resp :status (not= 200)) - (throw+ @error) + :payload payload}) + resp (try (http-request params) + (catch Throwable ex + {:exception ex}))] + (if (:exception resp) + (throw+ @error (:exception resp)) + (let [body #?(:bb (some-> resp :body parse-json) + :clj (:body resp)) + error (delay (assoc @error + :type :etaoin/http-error + :status (:status resp) + :response (error-response body)))] + (cond + (-> resp :status (not= 200)) + (throw+ @error) - (-> body :status (or 0) (> 0)) - (throw+ @error) + (-> body :status (or 0) (> 0)) + (throw+ @error) - :else - body))) + :else + body))))) diff --git a/test/etaoin/unit/proc_test.clj b/test/etaoin/unit/proc_test.clj index 0d05ee44..b52f421a 100644 --- a/test/etaoin/unit/proc_test.clj +++ b/test/etaoin/unit/proc_test.clj @@ -5,17 +5,18 @@ [clojure.string :as str] [clojure.test :refer [deftest is]] [etaoin.api :as e] + [etaoin.impl.client :as client] [etaoin.impl.proc :as proc] [etaoin.test-report])) -(defn get-count-chromedriver-instances - [] +(defn get-count-driver-instances + [drivername] (if proc/windows? - (let [instance-report (-> (shell/sh "powershell" "-command" "(Get-Process chromedriver -ErrorAction SilentlyContinue).Path") + (let [instance-report (-> (shell/sh "powershell" "-command" (format "(Get-Process %s -ErrorAction SilentlyContinue).Path" drivername)) :out str/split-lines)] ;; more flakiness diagnosis - (println "windows chromedriver instance report:" instance-report) + (println "windows" drivername "instance report:" instance-report) (println "windows full list of running processes:") ;; use Get-CimInstance, because Get-Process, does not have commandline available (pprint/pprint (-> (shell/sh "powershell" "-command" "Get-CimInstance Win32_Process | select name, commandline") @@ -23,55 +24,158 @@ str/split-lines)) (->> instance-report (remove #(str/includes? % "\\scoop\\shims\\")) ;; for the scoop users, exclude the shim process - (filter #(str/includes? % "chromedriver")) + (filter #(str/includes? % drivername)) count)) (->> (shell/sh "sh" "-c" "ps aux") :out str/split-lines - (filter #(str/includes? % "chromedriver")) + (filter #(str/includes? % drivername)) count))) -(deftest test-process-forking-port-specified +(defn get-count-chromedriver-instances [] + (get-count-driver-instances "chromedriver")) + +(defn get-count-firefoxdriver-instances [] + (get-count-driver-instances "geckodriver")) + +(deftest test-process-forking-port-specified-is-in-use (let [port 9997 - process (proc/run ["chromedriver" (format "--port=%d" port)]) - _ (e/wait-running {:port port :host "localhost"})] - (is (= 1 (get-count-chromedriver-instances))) - (is (thrown-with-msg? - clojure.lang.ExceptionInfo - #"already in use" - (e/chrome {:port port}))) - (proc/kill process))) + process (proc/run ["chromedriver" (format "--port=%d" port)])] + (try + (e/wait-running {:port port :host "localhost"}) + (is (= 1 (get-count-chromedriver-instances))) + (is (thrown-with-msg? + clojure.lang.ExceptionInfo + #"already in use" + (e/chrome {:port port}))) + (finally + (proc/kill process))))) -(deftest test-process-forking-port-random +(deftest test-process-forking-port-not-specified-so-random-port-is-picked (let [port 9998 - process (proc/run ["chromedriver" (format "--port=%d" port)]) - _ (e/wait-running {:port port :host "localhost"})] - (e/with-chrome {:args ["--no-sandbox"]} driver - ;; added to diagnose flakyness on windows on CI - (println "automatically chosen port->" (:port driver)) - ;; added to diagnose flakyness on windows on CI - (e/wait-running driver) - (is (= 2 (get-count-chromedriver-instances)))) - (proc/kill process))) + process (proc/run ["chromedriver" (format "--port=%d" port)])] + (try + (e/wait-running {:port port :host "localhost"}) + (e/with-chrome {:args ["--no-sandbox"]} driver + ;; added to diagnose flakyness on windows on CI + (println "automatically chosen port->" (:port driver)) + ;; added to diagnose flakyness on windows on CI + (e/wait-running driver) + (is (= 2 (get-count-chromedriver-instances)))) + (finally + (proc/kill process))))) -(deftest test-process-forking-connect-existing-host +(deftest test-process-forking-connect-existing-webdriver-host (let [port 9999 - process (proc/run ["chromedriver" (format "--port=%d" port)]) - _ (e/wait-running {:port port :host "localhost"}) - ;; should connect, not launch - driver (e/chrome {:host "localhost" :port port :args ["--no-sandbox"]})] - (is (= 1 (get-count-chromedriver-instances))) - (e/quit driver) - (proc/kill process))) + process (proc/run ["chromedriver" (format "--port=%d" port)])] + (try + (e/wait-running {:port port :host "localhost"}) + ;; should connect, not launch + (let [driver (e/chrome {:host "localhost" :port port :args ["--no-sandbox"]})] + (is (= 1 (get-count-chromedriver-instances))) + (e/quit driver)) + (finally + (proc/kill process))))) (deftest test-process-forking-connect-existing-webdriver-url (let [port 9999 - process (proc/run ["chromedriver" (format "--port=%d" port)]) - ;; normally would not call wait-running for a remote service, we are simulating here and want - ;; to make sure the process we launched is up and running - _ (e/wait-running {:port port :host "localhost"}) - ;; should connect, not launch - driver (e/chrome {:webdriver-url (format "http://localhost:%d" port) :args ["--no-sandbox"]})] - (is (= 1 (get-count-chromedriver-instances))) - (e/quit driver) - (proc/kill process))) + process (proc/run ["chromedriver" (format "--port=%d" port)])] + (try + (e/wait-running {:port port :host "localhost"}) + (let [;; should connect, not launch + driver (e/chrome {:webdriver-url (format "http://localhost:%d" port) :args ["--no-sandbox"]})] + + + (is (= 1 (get-count-chromedriver-instances))) + (e/quit driver)) + (finally + (proc/kill process))))) + +(deftest http-exception-on-create-proc-alive + ;; when etaoin tries to create a session we simulate a read timeout + (with-redefs [client/http-request (fn [_] (throw (ex-info "read timeout" {})))] + (let [ex (try + (e/with-firefox _driver + (is false "should not reach here")) + (catch Throwable ex + {:exception ex})) + exd (-> ex :exception ex-data)] + (is (= :etaoin/http-ex (:type exd))) + (is (= nil (-> exd :driver :process :exit))) + (is (= 0 (get-count-firefoxdriver-instances)))))) + +(deftest http-error-on-create-proc-alive + ;; when etaoin tries to create a session return an http error + (with-redefs [client/http-request (fn [_] {:status 400})] + (let [ex (try + (e/with-firefox _driver + (is false "should not reach here")) + (catch Throwable ex + {:exception ex})) + exd (-> ex :exception ex-data)] + (is (= :etaoin/http-error (:type exd))) + (is (= 400 (:status exd))) + (is (= nil (-> exd :driver :process :exit))) + (is (= 0 (get-count-firefoxdriver-instances)))))) + +(deftest http-exception-after-create-proc-now-dead + (let [orig-http-request client/http-request] + (with-redefs [client/http-request (fn [{:keys [method url] :as params}] + ;; allow create session through, fail on everything else + (if (and (= :post method) (str/ends-with? url "/session")) + (orig-http-request params) + (throw (ex-info "read timeout" {}))))] + (let [ex (try + ;; go headless to avoid that dangling open web browser + (e/with-firefox-headless driver + (is (= 1 (get-count-firefoxdriver-instances))) + (proc/kill (:process driver)) + ;; we'll now fail on this call + (e/go driver "https://clojure.org")) + (catch Throwable ex + {:exception ex})) + exd (-> ex :exception ex-data)] + (is (= :etaoin/http-ex (:type exd))) + (is (= 143 (-> exd :driver :process :exit))) + (is (= 0 (get-count-firefoxdriver-instances))))))) + +(deftest http-error-after-create-proc-now-dead + ;; unlikely, we know we just talked to the driver because it returned an http error, but for completeness + (let [orig-http-request client/http-request] + (with-redefs [client/http-request (fn [{:keys [method url] :as params}] + ;; allow create session through, fail on everything else + (if (and (= :post method) (str/ends-with? url "/session")) + (orig-http-request params) + {:status 418}))] + (let [ex (try + ;; go headless to avoid that dangling open web browser + (e/with-firefox-headless driver + (is (= 1 (get-count-firefoxdriver-instances))) + (proc/kill (:process driver)) + ;; we'll now fail on this call + (e/go driver "https://clojure.org")) + (catch Throwable ex + {:exception ex})) + exd (-> ex :exception ex-data)] + (is (= :etaoin/http-error (:type exd))) + (is (= 418 (:status exd))) + (is (= 143 (-> exd :driver :process :exit))) + (is (= 0 (get-count-firefoxdriver-instances))))))) + +(deftest test-cleanup-connect-existing-on-create-error + (let [port 9999 + process (proc/run ["chromedriver" (format "--port=%d" port)])] + (try + (e/wait-running {:port port :host "localhost"}) + (with-redefs [client/http-request (fn [_] (throw (ex-info "read timeout" {})))] + ;; attempt connect, not launch + (let [ex (try + (e/with-chrome {:webdriver-url (format "http://localhost:%d" port) :args ["--no-sandbox"]} driver + (is false "should not get here")) + (catch Throwable ex + {:exception ex})) + exd (-> ex :exception ex-data)] + (is (= :etaoin/http-ex (:type exd))) + (is (= 1 (get-count-chromedriver-instances))))) + (finally + (proc/kill process)))))