From e675e1aacbcb5b129353334c24d2c4ceda028612 Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Thu, 3 Oct 2024 12:56:57 -0500 Subject: [PATCH 1/7] Fix -connect-driver to wait for ready driver and recreate session There is a race condition in Firefox that sometimes results in a zombie session. When we detect this condition, we throw away the bad session and create another one. --- src/etaoin/api.clj | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/etaoin/api.clj b/src/etaoin/api.clj index add03c4..1a6e47c 100644 --- a/src/etaoin/api.clj +++ b/src/etaoin/api.clj @@ -3465,6 +3465,7 @@ (:post-run-actions driver)) (recur (inc try-num) (:exception res))))))))) + (defn- -connect-driver "Connects to a running Webdriver server. @@ -3546,9 +3547,29 @@ (drv/set-capabilities (:capabilities defaults-global)) (drv/set-capabilities (get-in defaults [type :capabilities])) (drv/set-capabilities capabilities))) - caps (:capabilities driver) - session (create-session driver caps)] - (assoc driver :session session))) + caps (:capabilities driver)] + (loop [n 1] + ;; Wait for driver to be ready before creating the first session. + (wait-predicate + (fn [] (-> (get-status driver) :ready)) + {:timeout 30 + :interval 0.100 + :message "Timeout waiting for WebDriver to become ready after creation."}) + (let [driver (assoc driver :session (create-session driver caps)) + good-session (try (get-window-handle driver) + true + (catch Exception e + (prn e) + false)) + ] + (if good-session + driver + (do + (delete-session driver) + (if (< n 3) ; max attempts = 3 + (recur (inc n)) + (throw+ {:type :etaoin/retries + :message "Could not create a good session after muiltiple retries"})))))))) (defn disconnect-driver "Returns new `driver` after disconnecting from a running WebDriver process. From 4cfcd0a3e32de339f9247672917c12968ecfa5f0 Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Thu, 3 Oct 2024 19:04:13 -0500 Subject: [PATCH 2/7] Fix fake driver to wrap data in :value and add Get Window Handle --- script/fake_driver.clj | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/script/fake_driver.clj b/script/fake_driver.clj index b020dd4..44aa224 100644 --- a/script/fake_driver.clj +++ b/script/fake_driver.clj @@ -27,14 +27,23 @@ (defn make-handler [_opts] (fn handle-request [{:keys [request-method uri] :as _req}] (cond + ;; Get Status + (and (= :get request-method) (= "/status" uri)) + {:status 200 + :headers {"Content-Type" "application/json"} + :body (json/generate-string {:value {:ready true + :message "I'm ready"}})} + ;; Create Session (and (= :post request-method) (= "/session" uri)) {:status 200 :headers {"Content-Type" "application/json"} - :body (json/generate-string {:sessionId (random-uuid)})} - (and (= :get request-method) (= "/status" uri)) + :body (json/generate-string {:value {:sessionId (random-uuid)}})} + ;; Get Window Handle + (and (= :get request-method) (re-matches #"/session/[^/]+/window" uri)) {:status 200 :headers {"Content-Type" "application/json"} - :body (json/generate-string {:ready true})} + :body (json/generate-string {:value (random-uuid)})} + ;; Fake Driver is... well... fake. :else {:status 404}))) From d1ad0eb7706752a4a6aa319583dffaf893af92ca Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Fri, 4 Oct 2024 13:23:11 -0500 Subject: [PATCH 3/7] Fix tests in proc_test.clj --- test/etaoin/unit/proc_test.clj | 42 +++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/test/etaoin/unit/proc_test.clj b/test/etaoin/unit/proc_test.clj index 62f4c8e..21f6e2c 100644 --- a/test/etaoin/unit/proc_test.clj +++ b/test/etaoin/unit/proc_test.clj @@ -105,23 +105,30 @@ (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)))))) + (let [orig-http-request client/http-request] + (with-redefs [client/http-request (fn [{:keys [method uri] :as params}] + ;; allow get status and create session through, fail on everything else + (if (and (= :get method) (str/ends-with? uri "/status")) + (orig-http-request params) + {: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 uri] :as params}] - ;; allow create session through, fail on everything else - (if (and (= :post method) (str/ends-with? uri "/session")) + ;; allow get status and create session through, fail on everything else + (if (or (and (= :get method) (str/ends-with? uri "/status")) + (and (= :post method) (str/ends-with? uri "/session")) + (and (= :get method) (re-find #"/session/[^/]+/window" uri))) (orig-http-request params) (throw (ex-info "read timeout" {}))))] (let [ex (try @@ -130,7 +137,8 @@ (is (= 1 (get-count-firefoxdriver-instances))) (proc/kill (:process driver)) ;; we'll now fail on this call - (e/go driver "https://clojure.org")) + (e/go driver "https://clojure.org") + (is false "Should have thrown an exception.")) (catch Throwable ex {:exception ex})) exd (-> ex :exception ex-data)] @@ -143,8 +151,10 @@ ;; 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 uri] :as params}] - ;; allow create session through, fail on everything else - (if (and (= :post method) (str/ends-with? uri "/session")) + ;; allow get status and create session through, fail on everything else + (if (or (and (= :get method) (str/ends-with? uri "/status")) + (and (= :post method) (str/ends-with? uri "/session")) + (and (= :get method) (re-find #"/session/[^/]+/window" uri))) (orig-http-request params) {:status 418}))] (let [ex (try From ced522d045e5f99694e079f0db00843c3ba66bbf Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Fri, 4 Oct 2024 19:44:22 -0500 Subject: [PATCH 4/7] Fix tests in unit_test.clj --- test/etaoin/unit/unit_test.clj | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/etaoin/unit/unit_test.clj b/test/etaoin/unit/unit_test.clj index 87b2529..6621b86 100644 --- a/test/etaoin/unit/unit_test.clj +++ b/test/etaoin/unit/unit_test.clj @@ -16,7 +16,9 @@ (with-redefs [etaoin.impl.proc/run (fn [_ _] {:some :process}) e/wait-running identity + e/get-status (fn [_] {:ready true :message "I'm ready"}) e/create-session (fn [_ _] "session-key") + e/get-window-handle (fn [_] "ABCDEFG") proc/kill identity e/delete-session identity util/get-free-port (constantly 12345)] @@ -142,7 +144,9 @@ [etaoin.impl.proc/run (fn [_ _] (swap! run-calls inc) {:some :process}) + e/get-status (fn [_] {:ready true :message "I'm ready"}) e/create-session (fn [_ _] "session-key") + e/get-window-handle (fn [_] "ABCDEFG") proc/kill (fn [_] (swap! kill-calls inc)) e/delete-session identity From aa45e55a54ee6b12d97580c7de2ba47a2ba87acf Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Mon, 7 Oct 2024 16:54:47 -0500 Subject: [PATCH 5/7] Update CHANGELOG --- CHANGELOG.adoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 48e54eb..fb8a67d 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -20,6 +20,7 @@ A release with an intentional breaking changes is marked with: == Unreleased * Changes +** {issue}676[#676]: Fix new driver creation so that it sidesteps some underlying Firefox race conditions and improves CI test stability. ({person}dgr[@dgr]) ** {issue}679[#679]: Add `new-window` function that exposes WebDriver's New Window endpoint. ({person}dgr[@dgr]) == v1.1.42 - 2024-09-27 [[v1.1.42]] From 4c6a172cc0f7315b4fa002069a941201b1e93595 Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Tue, 8 Oct 2024 12:57:25 -0500 Subject: [PATCH 6/7] Fix PR comment items --- src/etaoin/api.clj | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/etaoin/api.clj b/src/etaoin/api.clj index 1a6e47c..3ca24f6 100644 --- a/src/etaoin/api.clj +++ b/src/etaoin/api.clj @@ -3547,7 +3547,8 @@ (drv/set-capabilities (:capabilities defaults-global)) (drv/set-capabilities (get-in defaults [type :capabilities])) (drv/set-capabilities capabilities))) - caps (:capabilities driver)] + caps (:capabilities driver) + max-retries 3] (loop [n 1] ;; Wait for driver to be ready before creating the first session. (wait-predicate @@ -3555,21 +3556,33 @@ {:timeout 30 :interval 0.100 :message "Timeout waiting for WebDriver to become ready after creation."}) + ;; The following code works around a bug in + ;; geckodriver/Firefox. In some cases, calling create-session on + ;; a geckodriver will return a "bad" session instance. That + ;; session instance will throw errors for the simplest queries. + ;; The root problem appears to be a race condition somewhere in + ;; the geckodriver/marionette/Firefox pathway as it is + ;; inconsistent and difficult to reproduce. To work around this, + ;; we try to detect a "bad session" and if we have one, we + ;; delete it and create another. Note that the problem is + ;; geckodriver/Firefox-specific, but we do this for all drivers + ;; because it works anyway (non-Firefox drivers just return + ;; a "good session") and it simplifies the logic. (let [driver (assoc driver :session (create-session driver caps)) - good-session (try (get-window-handle driver) - true - (catch Exception e - (prn e) - false)) - ] + [good-session e] (try (get-window-handle driver) + [true nil] + (catch Exception e + [false e]))] (if good-session driver (do (delete-session driver) - (if (< n 3) ; max attempts = 3 - (recur (inc n)) + (if (< n max-retries) ; max attempts = 3 + (do (log/warnf e "Bad session detected. Retrying %d/%d" n max-retries) + (recur (inc n))) (throw+ {:type :etaoin/retries - :message "Could not create a good session after muiltiple retries"})))))))) + :message "Could not create a good session after multiple retries"} + e)))))))) (defn disconnect-driver "Returns new `driver` after disconnecting from a running WebDriver process. From f65f86f3519b6b78a7cad4b231b09e9a711e53ea Mon Sep 17 00:00:00 2001 From: Dave Roberts Date: Tue, 8 Oct 2024 14:01:43 -0500 Subject: [PATCH 7/7] Fix OBOB and make log message and var names more accurate --- src/etaoin/api.clj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/etaoin/api.clj b/src/etaoin/api.clj index 3ca24f6..d3c4f59 100644 --- a/src/etaoin/api.clj +++ b/src/etaoin/api.clj @@ -3548,7 +3548,7 @@ (drv/set-capabilities (get-in defaults [type :capabilities])) (drv/set-capabilities capabilities))) caps (:capabilities driver) - max-retries 3] + max-tries 3] (loop [n 1] ;; Wait for driver to be ready before creating the first session. (wait-predicate @@ -3577,8 +3577,8 @@ driver (do (delete-session driver) - (if (< n max-retries) ; max attempts = 3 - (do (log/warnf e "Bad session detected. Retrying %d/%d" n max-retries) + (if (<= n max-tries) ; max total tries = 3 + (do (log/warnf e "Bad session detected. Try again %d/%d." (inc n) max-tries) (recur (inc n))) (throw+ {:type :etaoin/retries :message "Could not create a good session after multiple retries"}