Merge pull request #9915 from metabase/set-context-classloader-when-resolving-drivers

Make sure current thread is using shared classloader when resolving drivers

src/metabase/driver.clj

@@ -142,6 +142,7 @@
     (the-driver :postgres) ; -> :postgres
     (the-driver :baby)     ; -> Exception"
   [driver :- (s/cond-pre s/Str s/Keyword)]
+  (classloader/the-classloader)
   (let [driver (keyword driver)]
     (load-driver-namespace-if-needed driver)
     driver))

src/metabase/plugins/classloader.clj

@@ -18,23 +18,32 @@
   (:import [clojure.lang DynamicClassLoader RT]
            java.net.URL))
 
-(def ^:private shared-context-classloader
-  "The context classloader we'll use for *all threads*, once we figure out what that is. Guaranteed to be an instance of
-  `DynamicClassLoader`."
-  (promise))
-
-;; If the Clojure runtime base loader is already an instance of DynamicClassLoader (e.g. it is something like
-;; `clojure.lang.Compiler/LOADER` we can go ahead and use that in the future. This is usually the case when doing
-;; REPL-based development or running via `lein`; when running from the UberJAR `clojure.lang.Compiler/LOADER` is not
-;; set and thus this will return the current thread's context classloader, which is usually just the System classloader.
-;;
-;; The base loader is what Clojure ultimately uses to loading namespaces with `require` so adding URLs to it is they
-;; way to go, if we can
-(when-not *compile-files*
-  (u/prog1 (RT/baseLoader)
-    (when (instance? DynamicClassLoader <>)
-      (log/debug (trs "Using Clojure base loader as shared context classloader: {0}" <>))
-      (deliver shared-context-classloader <>))))
+(defonce ^:private ^{:doc "The context classloader we'll use for *all threads*, once we figure out what that is.
+  Guaranteed to be an instance of `DynamicClassLoader`."} shared-context-classloader
+  (delay
+   ;; If the Clojure runtime base loader is already an instance of DynamicClassLoader (e.g. it is something like
+   ;; `clojure.lang.Compiler/LOADER` we can go ahead and use that in the future. This is usually the case when doing
+   ;; REPL-based development or running via `lein`; when running from the UberJAR `clojure.lang.Compiler/LOADER` is
+   ;; not set and thus this will return the current thread's context classloader, which is usually just the System
+   ;; classloader.
+   ;;
+   ;; The base loader is what Clojure ultimately uses to loading namespaces with `require` so adding URLs to it is
+   ;; they way to go, if we can)
+   (or
+    (when-let [base-loader (RT/baseLoader)]
+      (when (instance? DynamicClassLoader base-loader)
+        (log/debug (trs "Using Clojure base loader as shared context classloader: {0}" base-loader))
+        base-loader))
+    ;; Otherwise if we need to create our own go ahead and do it
+    ;;
+    ;; Make a new classloader using the current thread's context classloader as it's parent. In cases where we hit
+    ;; this condition (i.e., when running from the uberjar), the current thread's context classloader should be the
+    ;; system classloader. Since it will be the same for other threads too it doesn't matter if we ignore *their*
+    ;; context classloaders by giving them this one. No other places in the codebase should be modifying classloaders
+    ;; anyway.
+    (let [new-classloader (DynamicClassLoader. (.getContextClassLoader (Thread/currentThread)))]
+      (log/debug (trs "Using NEWLY CREATED classloader as shared context classloader: {0}" new-classloader))
+      new-classloader))))
 
 
 (defn- has-classloader-as-ancestor?
@@ -46,13 +55,15 @@
     true
 
     classloader
-    (recur (.getParent classloader) ancestor)))
+    (recur (.getParent classloader) ancestor)
+
+    :else
+    false))
 
 (defn- has-shared-context-classloader-as-ancestor?
   "True if the `shared-context-classloader` has been set and it is an ancestor of `classloader`."
   [^ClassLoader classloader]
-  (when (realized? shared-context-classloader)
-    (has-classloader-as-ancestor? classloader @shared-context-classloader)))
+  (has-classloader-as-ancestor? classloader @shared-context-classloader))
 
 
 (defn ^ClassLoader the-classloader
@@ -63,35 +74,15 @@
   before calling `require`, to ensure the context classloader for the current thread is one that has access to the JARs
   we've added to the classpath."
   []
-  (let [current-thread-context-classloader (.getContextClassLoader (Thread/currentThread))]
-    (cond
-      ;; if the context classloader already has the classloader we'll add URLs to as an ancestor return it as-is
-      (has-shared-context-classloader-as-ancestor? current-thread-context-classloader)
-      current-thread-context-classloader
-
-      ;; Otherwise we'll have to create our own new context classloader. We'll use the same one for all the threads
-      ;; that need it. Check and see if we've already made one; if so, we can return that as-is
-      (realized? shared-context-classloader)
-      (u/prog1 @shared-context-classloader
-        (log/debug (trs "Setting current thread context classloader to shared classloader {0}..." <>))
-        (.setContextClassLoader (Thread/currentThread) <>))
-
-      ;; Otherwise if we need to create our own and it HAS NOT been done yet go ahead and do it
-      :else
-      (do
-        ;; Make a new classloader using the current thread's context classloader as it's parent. In cases where we hit
-        ;; this condition (i.e., when running from the uberjar), the current thread's context classloader should be
-        ;; the system classloader. Since it will be the same for other threads too it doesn't matter if we ignore
-        ;; *their* context classloaders by giving them this one. No other places in the codebase should be modifying
-        ;; classloaders anyway.
-        (deliver shared-context-classloader (DynamicClassLoader. current-thread-context-classloader))
-        ;; it's important that we deref the promise again here instead of using the one we just created because it is
-        ;; possible thru a race condition that somebody else delivered the promise before we did; in that case,
-        ;; Clojure ignores subsequent calls to `deliver`. Dereffing the promise guarantees that we'll get the actual
-        ;; value of it rather than one that ends up getting discarded
-        (log/debug (trs "Setting current thread context classloader to NEWLY CREATED classloader {0}..."
-                        @shared-context-classloader))
-        (.setContextClassLoader (Thread/currentThread) @shared-context-classloader)))))
+  (or
+   ;; if the context classloader already has the classloader we'll add URLs to as an ancestor return it as-is
+   (let [current-thread-context-classloader (.getContextClassLoader (Thread/currentThread))]
+     (when (has-shared-context-classloader-as-ancestor? current-thread-context-classloader)
+       current-thread-context-classloader))
+   ;; otherwise set the current thread's context classloader to the shared context classloader
+   (u/prog1 @shared-context-classloader
+     (log/debug (trs "Setting current thread context classloader to shared classloader {0}..." <>))
+     (.setContextClassLoader (Thread/currentThread) <>))))
 
 
 (defn- classloader-hierarchy

test/metabase/driver_test.clj

 (ns metabase.driver-test
-  (:require [expectations :refer :all]
-            [metabase.driver :as driver]))
+  (:require [expectations :refer [expect]]
+            [metabase.driver :as driver]
+            [metabase.plugins.classloader :as classloader]))
 
 (driver/register! ::test-driver)
 
@@ -22,3 +23,12 @@
 (expect
   'metabase.driver-test
   (#'driver/driver->expected-namespace ::toucans))
+
+;; calling `the-driver` should set the context classloader, important because driver plugin code exists there but not
+;; elsewhere
+(expect
+  @@#'classloader/shared-context-classloader
+  (do
+    (.setContextClassLoader (Thread/currentThread) (ClassLoader/getSystemClassLoader))
+    (driver/the-driver :h2)
+    (.getContextClassLoader (Thread/currentThread))))

test/metabase/plugins/classloader_test.clj

+(ns metabase.plugins.classloader-test
+  (:require [expectations :refer [expect]]
+            [metabase.plugins.classloader :as classloader])
+  (:import clojure.lang.DynamicClassLoader))
+
+;; make sure we correctly detect when the current thread has the shared dynamic classloader as an ancestor
+(expect
+  false
+  (do
+    (.setContextClassLoader (Thread/currentThread) (ClassLoader/getSystemClassLoader))
+    (#'classloader/has-shared-context-classloader-as-ancestor? (.getContextClassLoader (Thread/currentThread)))))
+
+(expect
+  (do
+    (.setContextClassLoader (Thread/currentThread) @@#'classloader/shared-context-classloader)
+    (#'classloader/has-shared-context-classloader-as-ancestor? (.getContextClassLoader (Thread/currentThread)))))
+
+(expect
+  (do
+    (.setContextClassLoader (Thread/currentThread) (DynamicClassLoader. @@#'classloader/shared-context-classloader))
+    (#'classloader/has-shared-context-classloader-as-ancestor? (.getContextClassLoader (Thread/currentThread)))))
+
+;; if the current thread does NOT have a context classloader that is a descendent of the shared context classloader,
+;; calling `the-classloader` should set it as a side-effect
+(expect
+  @@#'classloader/shared-context-classloader
+  (do
+    (.setContextClassLoader (Thread/currentThread) (ClassLoader/getSystemClassLoader))
+    (classloader/the-classloader)
+    (.getContextClassLoader (Thread/currentThread))))
+
+;; if current thread context classloader === the shared context classloader it should be kept as-is
+(expect
+  @@#'classloader/shared-context-classloader
+  (do
+    (.setContextClassLoader (Thread/currentThread) @@#'classloader/shared-context-classloader)
+    (classloader/the-classloader)
+    (.getContextClassLoader (Thread/currentThread))))
+
+;; if current thread context classloader is a *descendant* the shared context classloader it should be kept as-is
+(let [descendant-classloader (DynamicClassLoader. @@#'classloader/shared-context-classloader)]
+  (expect
+    descendant-classloader
+    (do
+      (.setContextClassLoader (Thread/currentThread) descendant-classloader)
+      (classloader/the-classloader)
+      (.getContextClassLoader (Thread/currentThread)))))