Skip to content
Snippets Groups Projects
Unverified Commit 296e8561 authored by Cam Saul's avatar Cam Saul
Browse files

Extra tests to make sure JDBC queries release resources when done

parent 7093ecb1
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/driver/sql_jdbc/execute.clj
+ 26
− 12
View file @ 296e8561
(ns metabase.driver.sql-jdbc.execute
"Code related to actually running a SQL query against a JDBC database (including setting the session timezone when
appropriate), and for properly encoding/decoding types going in and out of the database."
......@@ -175,17 +174,27 @@
(.closeOnCompletion stmt))
;; Need to run the query in another thread so that this thread can cancel it if need be
(try
(let [query-future (future (jdbc/query conn (into [stmt] params) opts))]
;; This thread is interruptable because it's awaiting the other thread (the one actually running the
;; query). Interrupting this thread means that the client has disconnected (or we're shutting down) and so
;; we can give up on the query running in the future
@query-future)
;; This thread is interruptable because it's awaiting the other thread (the one actually running the query).
;; Interrupting this thread means that the client has disconnected (or we're shutting down) and so we can give
;; up on the query running in the future
(let [query-future (future
(try
(jdbc/query conn (into [stmt] params) opts)
(catch Throwable e
e)))
result @query-future]
(if (instance? Throwable result)
(throw result)
result))
(catch InterruptedException e
(log/warn (tru "Client closed connection, canceling query"))
;; This is what does the real work of canceling the query. We aren't checking the result of
;; `query-future` but this will cause an exception to be thrown, saying the query has been cancelled.
(.cancel stmt)
(throw e))))))
(throw e))
(catch Exception e
(.cancel stmt)
e)))))
(defn- run-query
"Run the query itself."
......@@ -205,10 +214,12 @@
;;; -------------------------- Running queries: exception handling & disabling auto-commit ---------------------------
(defn- exception->nice-error-message ^String [^SQLException e]
(or (->> (.getMessage e) ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
(re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless
second) ; so just return the part of the exception that is relevant
(.getMessage e)))
;; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
;; the user already knows the SQL, and error code is meaningless
;; so just return the part of the exception that is relevant
(->> (.getMessage e)
(re-find #"^(.*);")
second))
(defn do-with-try-catch
"Tries to run the function `f`, catching and printing exception chains if SQLException is thrown,
......@@ -219,7 +230,10 @@
(f)
(catch SQLException e
(log/error (jdbc/print-sql-exception-chain e))
(throw (Exception. (exception->nice-error-message e) e)))))
(throw
(if-let [nice-error-message (exception->nice-error-message e)]
(Exception. nice-error-message e)
e)))))
(defn- do-with-auto-commit-disabled
"Disable auto-commit for this transaction, and make the transaction `rollback-only`, which means when the
......
test/metabase/driver/sql_jdbc/execute_test.clj
+ 131
− 5
View file @ 296e8561
(ns metabase.driver.sql-jdbc.execute-test
(:require [clojure.java.jdbc :as jdbc]
(:require [clojure.core.async :as a]
[clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[expectations :refer [expect]]
[metabase
[query-processor :as qp]
[util :as u]]
[metabase.driver.sql-jdbc-test :as sql-jdbc-test]
[metabase.query-processor :as qp]
[metabase.test.data :as data]
[metabase.test.data.datasets :as datasets])
(:import java.sql.PreparedStatement))
[metabase.test
[data :as data]
[util :as tu]]
[metabase.test.data.datasets :as datasets]
[metabase.test.util.log :as tu.log])
(:import [java.sql PreparedStatement ResultSet]))
(defn- do-with-max-rows [f]
;; force loading of the test data before swapping out `jdbc/query`, otherwise things might not sync correctly
......@@ -59,3 +67,121 @@
:type :query
:query {:source-table (data/id :venues)}})
:constraints {:max-results 15}})))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Query Cancelation & Resource Closing Tests |
;;; +----------------------------------------------------------------------------------------------------------------+
(defrecord ^:private FakePreparedStatement [called-cancel? called-close? fake-resultset]
java.sql.PreparedStatement
(closeOnCompletion [_]) ; no-op
(cancel [_] (some-> called-cancel? (deliver true)))
(close [_] (some-> called-close? (deliver true)))
(executeQuery [_] fake-resultset))
(defrecord ^:private FakeResultSet [called-close?]
ResultSet
(close [_] (some-> called-close? (deliver true)))
(next [_] false)
(getMetaData [_]
(reify java.sql.ResultSetMetaData
(getColumnCount [_] 0))))
(defn- fake-prepare-statement
"Returns `fake-value` whenenver the `sql` parameter returns a truthy value when passed to `use-fake-value?`."
[& {:keys [use-fake-value? faked-value]
:or {use-fake-value? (constantly false)}}]
(let [original-prepare-statement jdbc/prepare-statement]
(fn [connection sql options]
(if (use-fake-value? sql)
faked-value
(original-prepare-statement connection sql options)))))
(defn- fake-query
"Function to replace the `clojure.java.jdbc/query` function. Will invoke `on-fake-prepared-statement` when passed an
instance of `FakePreparedStatement`."
[& {:keys [on-fake-prepared-statement]}]
(let [original-query jdbc/query]
(fn
([conn stmt+params]
(conn stmt+params))
([conn stmt+params opts]
(if (instance? FakePreparedStatement (first stmt+params))
(when on-fake-prepared-statement (on-fake-prepared-statement))
(original-query conn stmt+params opts))))))
;; make sure queries properly clean up after themselves and close result sets and prepared statements when finished
(expect
{:prepared-statement-closed? true, :result-set-closed? true}
(let [closed-prepared-statement? (promise)
closed-result-set? (promise)
fake-resultset (->FakeResultSet closed-result-set?)
fake-prepared-statement (->FakePreparedStatement nil closed-prepared-statement? fake-resultset)
placeholder-query "SELECT 1;"]
(with-redefs [jdbc/prepare-statement (fake-prepare-statement
:use-fake-value? #(str/includes? % placeholder-query)
:faked-value fake-prepared-statement)]
(let [{{:keys [rows]} :data, :as results} (qp/process-query
{:database (data/id)
:type :native
:native {:query placeholder-query}})]
(when-not rows
(throw (ex-info "Query failed to run!" results)))
;; make sure results are fully realized!
(mapv vec rows))
{:prepared-statement-closed? (deref closed-prepared-statement? 1000 false)
:result-set-closed? (deref closed-result-set? 1000 false)})))
;; make sure a prepared statement is canceled & closed when the async QP channel is closed
(expect
{:prepared-statement-canceled? true, :prepared-statement-closed? true}
(let [started? (promise)
canceled-prepared-statement? (promise)
closed-prepared-statement? (promise)
fake-prepared-statement (->FakePreparedStatement canceled-prepared-statement? closed-prepared-statement? nil)
placeholder-query "SLEEP PLACEHOLDER"]
(with-redefs [jdbc/prepare-statement (fake-prepare-statement
:use-fake-value? #(str/includes? % placeholder-query)
:faked-value fake-prepared-statement)
jdbc/query (fake-query
:on-fake-prepared-statement
(fn [& _]
(deliver started? true)
(Thread/sleep 2000)
(->FakeResultSet nil)))]
(let [chan (qp/process-query
{:database (data/id)
:type :native
:native {:query placeholder-query}
:async? true})]
(u/deref-with-timeout started? 1000)
(a/close! chan)
{:prepared-statement-canceled? (deref canceled-prepared-statement? 3000 false)
:prepared-statement-closed? (deref closed-prepared-statement? 1000 false)}))))
;; The test below sort of tests the same thing (I think). But it is written in a much more confusing manner. If you
;; dare, try to work out why happens by reading source `call-with-paused-query`. - Cam
(expect
::tu/success
;; this might dump messages about the connection being closed; we don't need to worry about that
(tu.log/suppress-output
(tu/call-with-paused-query
(fn [query-thunk called-query? called-cancel? pause-query]
(let [ ;; This fake prepared statement is cancelable like a prepared statement, but will allow us to tell the
;; difference between our Prepared statement and the real thing
fake-prep-stmt (->FakePreparedStatement called-cancel? nil nil)]
(future
(try
(with-redefs [jdbc/prepare-statement (fake-prepare-statement
:use-fake-value? (fn [sql] (re-find #"checkins" (str/lower-case sql)))
:faked-value fake-prep-stmt)
jdbc/query (fake-query
:on-fake-prepared-statement
(fn []
(deliver called-query? true)
@pause-query))]
(query-thunk))
(catch Throwable e
(throw e)))))))))
test/metabase/query_processor_test/query_cancellation_test.clj deleted 100644 → 0
+ 0
− 65
View file @ 7093ecb1
(ns metabase.query-processor-test.query-cancellation-test
"TODO - This is sql-jdbc specific, so it should go in a sql-jdbc test namespace."
(:require [clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[expectations :refer [expect]]
[metabase.test.util :as tu]
[metabase.test.util.log :as tu.log]))
(defrecord ^:private FakePreparedStatement [called-cancel?]
java.sql.PreparedStatement
(closeOnCompletion [_]) ; no-op
(cancel [_] (deliver called-cancel? true))
(close [_] true))
(defn- make-fake-prep-stmt
"Returns `fake-value` whenenver the `sql` parameter returns a truthy value when passed to `use-fake-value?`."
[orig-make-prep-stmt & {:keys [use-fake-value? faked-value]
:or {use-fake-value? (constantly false)}}]
(fn [connection sql options]
(if (use-fake-value? sql)
faked-value
(orig-make-prep-stmt connection sql options))))
(defn- fake-query
"Function to replace the `clojure.java.jdbc/query` function. Will invoke `on-fake-prepared-statement` when passed an
instance of `FakePreparedStatement`."
{:style/indent 1}
[orig-jdbc-query & {:keys [on-fake-prepared-statement]}]
(fn
([conn stmt+params]
(orig-jdbc-query conn stmt+params))
([conn stmt+params opts]
(if (instance? FakePreparedStatement (first stmt+params))
(when on-fake-prepared-statement (on-fake-prepared-statement))
(orig-jdbc-query conn stmt+params opts)))))
(expect
::tu/success
;; this might dump messages about the connection being closed; we don't need to worry about that
(tu.log/suppress-output
(tu/call-with-paused-query
(fn [query-thunk called-query? called-cancel? pause-query]
(let [ ;; This fake prepared statement is cancelable like a prepared statement, but will allow us to tell the
;; difference between our Prepared statement and the real thing
fake-prep-stmt (->FakePreparedStatement called-cancel?)
;; Much of the underlying plumbing of MB requires a working jdbc/query and jdbc/prepared-statement (such
;; as queryies for the application database). Let binding the original versions of the functions allows
;; us to delegate to them when it's not the query we're trying to test
orig-jdbc-query jdbc/query
orig-prep-stmt jdbc/prepare-statement]
(future
(try
(with-redefs [jdbc/prepare-statement (make-fake-prep-stmt
orig-prep-stmt
:use-fake-value? (fn [sql] (re-find #"checkins" (str/lower-case sql)))
:faked-value fake-prep-stmt)
jdbc/query (fake-query orig-jdbc-query
:on-fake-prepared-statement
(fn []
(deliver called-query? true)
@pause-query))]
(query-thunk))
(catch Throwable e
(throw e)))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment