Fix race conditions when loading drivers (#16750)

src/metabase/driver/impl.clj

@@ -5,7 +5,8 @@
             [metabase.plugins.classloader :as classloader]
             [metabase.util :as u]
             [metabase.util.i18n :refer [trs tru]]
-            [schema.core :as s]))
+            [schema.core :as s])
+  (:import java.util.concurrent.locks.ReentrantReadWriteLock))
 
 ;;; --------------------------------------------------- Hierarchy ----------------------------------------------------
 
@@ -13,10 +14,37 @@
   hierarchy
   (make-hierarchy))
 
+
+(defonce ^{:doc "To find out whether a driver has been registered, we need to wait until any current driver-loading
+  operations have finished. Otherwise we can get a \"false positive\" -- see #13114.
+
+  To see whether a driver is registered, we only need to obtain a *read lock* -- multiple threads can have these at
+  once, and they only block if a write lock is held or if a thread is waiting for one (see dox
+  for [[ReentrantReadWriteLock]] for more details.)
+
+  If we're currently in the process of loading a driver namespace, obtain the *write lock* which will prevent other
+ threads from obtaining read locks until it finishes."} ^:private ^ReentrantReadWriteLock load-driver-lock
+  (ReentrantReadWriteLock.))
+
+(defmacro ^:private with-load-driver-read-lock [& body]
+  `(try
+     (.. load-driver-lock readLock lock)
+     ~@body
+     (finally
+       (.. load-driver-lock readLock unlock))))
+
+(defmacro ^:private with-load-driver-write-lock [& body]
+  `(try
+     (.. load-driver-lock writeLock lock)
+     ~@body
+     (finally
+       (.. load-driver-lock writeLock unlock))))
+
 (defn registered?
   "Is `driver` a valid registered driver?"
   [driver]
-  (isa? hierarchy (keyword driver) :metabase.driver/driver))
+  (with-load-driver-read-lock
+    (isa? hierarchy (keyword driver) :metabase.driver/driver)))
 
 (defn concrete?
   "Is `driver` registered, and non-abstract?"
@@ -60,14 +88,17 @@
   [driver]
   (when-not *compile-files*
     (when-not (registered? driver)
-      (u/profile (trs "Load driver {0}" driver)
-        (require-driver-ns driver)
-        ;; ok, hopefully it was registered now. If not, try again, but reload the entire driver namespace
+      (with-load-driver-write-lock
+        ;; driver may have become registered while we were waiting for the lock, check again to be sure
         (when-not (registered? driver)
-          (require-driver-ns driver :reload)
-          ;; if *still* not registered, throw an Exception
-          (when-not (registered? driver)
-            (throw (Exception. (tru "Driver not registered after loading: {0}" driver)))))))))
+          (u/profile (trs "Load driver {0}" driver)
+            (require-driver-ns driver)
+            ;; ok, hopefully it was registered now. If not, try again, but reload the entire driver namespace
+            (when-not (registered? driver)
+              (require-driver-ns driver :reload)
+              ;; if *still* not registered, throw an Exception
+              (when-not (registered? driver)
+                (throw (Exception. (tru "Driver not registered after loading: {0}" driver)))))))))))
 
 
 ;;; -------------------------------------------------- Registration --------------------------------------------------

test/metabase/driver/impl_test.clj

 (ns metabase.driver.impl-test
-  (:require [clojure.test :refer :all]
-            [metabase.driver.impl :as impl]))
+  (:require [clojure.core.async :as a]
+            [clojure.test :refer :all]
+            [metabase.driver :as driver]
+            [metabase.driver.impl :as impl]
+            [metabase.test.util.async :as tu.async]))
 
 (deftest driver->expected-namespace-test
   (testing "expected namespace for a non-namespaced driver should be `metabase.driver.<driver>`"
@@ -9,3 +12,24 @@
   (testing "for a namespaced driver it should be the namespace of the keyword"
     (is (= 'metabase.driver.impl-test
            (#'impl/driver->expected-namespace ::toucans)))))
+
+(deftest load-driver-namespace-race-condition-test
+  (testing "Make sure we don't report a driver as being registered if its namespace is in the process of being loaded (#13114)"
+    (alter-var-root #'impl/hierarchy underive ::race-condition-test :metabase.driver/driver)
+    ;; basic idea for this test is simulate loading a driver namespace on a different thread and have it register
+    ;; itself immediately. Then on another thread we should call `the-initialized-driver`, but it shouldn't return
+    ;; until the namespace has completed loading.
+    (tu.async/with-open-channels [started-loading-chan (a/promise-chan)]
+      (let [finished-loading (atom false)]
+        (with-redefs [impl/require-driver-ns (fn [_]
+                                               (driver/register! ::race-condition-test)
+                                               (a/>!! started-loading-chan :start)
+                                               (Thread/sleep 100)
+                                               (reset! finished-loading true))]
+          ;; fire off a separate thread that will start loading the driver
+          (future (driver/the-initialized-driver ::race-condition-test))
+          (tu.async/wait-for-result started-loading-chan 500)
+          (is (= ::race-condition-test
+                 (driver/the-initialized-driver ::race-condition-test)))
+          (is (= true
+                 @finished-loading)))))))