From e8201e38b7816eadacc5e111e1c7234fff6cf6c4 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Fri, 10 May 2024 23:22:09 +0700
Subject: [PATCH] Thread safe static viz (#42494)
---
src/metabase/pulse/render/js_engine.clj | 14 +++++---
src/metabase/pulse/render/js_svg.clj | 1 -
test/metabase/pulse/render/body_test.clj | 32 +++++++++++++++++--
test/metabase/pulse/render/js_engine_test.clj | 10 ++++++
4 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/src/metabase/pulse/render/js_engine.clj b/src/metabase/pulse/render/js_engine.clj
index 634c298723c..916a0cce3aa 100644
--- a/src/metabase/pulse/render/js_engine.clj
+++ b/src/metabase/pulse/render/js_engine.clj
@@ -46,10 +46,16 @@
(defn execute-fn-name
"Executes `js-fn-name` in js context with args"
^Value [^Context context js-fn-name & args]
- (let [fn-ref (.eval context "js" js-fn-name)
- args (into-array Object args)]
- (assert (.canExecute fn-ref) (str "cannot execute " js-fn-name))
- (.execute fn-ref args)))
+ ;; TODO: locking context is not ideal, but contexts are currently being shared with all threads and GraalVM doesn't
+ ;; support concurrent execution for js.
+ ;; There is a couple of idea we can try:
+ ;; - put a thread pool around context initialization
+ ;; - init a new context for each thread
+ (locking context
+ (let [fn-ref (.eval context "js" js-fn-name)
+ args (into-array Object args)]
+ (assert (.canExecute fn-ref) (str "cannot execute " js-fn-name))
+ (.execute fn-ref args))))
(defn execute-fn
"fn-ref should be an executable org.graalvm.polyglot.Value return from a js engine. Invoke this function with args."
diff --git a/src/metabase/pulse/render/js_svg.clj b/src/metabase/pulse/render/js_svg.clj
index 7ab07f8ba1e..94ea37ac7ce 100644
--- a/src/metabase/pulse/render/js_svg.clj
+++ b/src/metabase/pulse/render/js_svg.clj
@@ -37,7 +37,6 @@
(def ^:private static-viz-context-delay
"Delay containing a graal js context. It has the chart bundle and the above `src-api` in its environment suitable
for creating charts."
- ;; todo is this thread safe? Should we have a resource pool on top of this? Or create them fresh for each invocation
(delay (load-viz-bundle (js/context))))
(defn- context
diff --git a/test/metabase/pulse/render/body_test.clj b/test/metabase/pulse/render/body_test.clj
index 8c76c19439c..47aa3ca8d41 100644
--- a/test/metabase/pulse/render/body_test.clj
+++ b/test/metabase/pulse/render/body_test.clj
@@ -8,6 +8,7 @@
[hickory.select :as hik.s]
[metabase.formatter :as formatter]
[metabase.models :refer [Card]]
+ [metabase.pulse :as pulse]
[metabase.pulse.render.body :as body]
[metabase.pulse.render.test-util :as render.tu]
[metabase.pulse.util :as pu]
@@ -692,7 +693,7 @@
[content-to-match]
(fn [loc]
(let [{:keys [content]} (zip/node loc)]
- (= content content-to-match ))))
+ (= content content-to-match))))
(defn- parse-transform [s]
(let [numbers (-> (re-find #"matrix\((.+)\)" s)
@@ -719,7 +720,7 @@
:Gizmo {:axis dir}
:Widget {:axis dir}},
:graph.dimensions ["PRICE" "CATEGORY"],
- :graph.metrics ["count"]}) ]
+ :graph.metrics ["count"]})]
(mt/with-temp [:model/Card {left-card-id :id} {:display :bar
:visualization_settings (viz "left")
:dataset_query q}
@@ -751,3 +752,30 @@
:e)]
(is (= 1 (count axis-label-element)))
(is (< 200 axis-y-transform)))))))))
+
+(defn- render-card
+ [render-type card data]
+ (body/render render-type :attachment (pulse/defaulted-timezone card) card nil data))
+
+(defn execute-n-times-in-parallel
+ "Execute f n times in parallel and derefs all the results."
+ [f n]
+ (mapv deref (for [_ (range n)]
+ (future (f)))))
+
+(deftest render-cards-are-thread-safe-test-for-js-visualization
+ (mt/with-temp [:model/Card card {:dataset_query (mt/mbql-query orders
+ {:aggregation [[:count]]
+ :breakout [$orders.created_at]
+ :limit 1})
+ :display :line
+ :visualization_settings {:graph.dimensions ["CREATED_AT"]
+ :graph.metrics ["count"]}}]
+ (let [data (:data (qp/process-query (:dataset_query card)))]
+ (is (every? some? (execute-n-times-in-parallel #(render-card :javascript_visualization card data) 3))))))
+
+(deftest render-cards-are-thread-safe-test-for-table
+ (mt/with-temp [:model/Card card {:dataset_query (mt/mbql-query venues {:limit 1})
+ :display :table}]
+ (let [data (:data (qp/process-query (:dataset_query card)))]
+ (is (every? some? (execute-n-times-in-parallel #(render-card :table card data) 3))))))
diff --git a/test/metabase/pulse/render/js_engine_test.clj b/test/metabase/pulse/render/js_engine_test.clj
index e57f777f8c5..9f194ee7536 100644
--- a/test/metabase/pulse/render/js_engine_test.clj
+++ b/test/metabase/pulse/render/js_engine_test.clj
@@ -1,6 +1,7 @@
(ns metabase.pulse.render.js-engine-test
(:require
[clojure.test :refer :all]
+ [metabase.pulse.render.body-test :as body-test]
[metabase.pulse.render.js-engine :as js]))
(set! *warn-on-reflection* true)
@@ -16,3 +17,12 @@
"curried function test")
(let [curried (js/execute-fn-name context "curry_plus" 1)]
(is (= 3 (.asLong (js/execute-fn curried 2))))))))
+
+(deftest thread-safe-execute-fn-name-test
+ (testing "execute-fn-name is thread safe"
+ (let [context (js/context)]
+ (js/load-js-string context "function plus (x, y) { return x + y }" "plus test")
+ (is (= (repeat 10 2)
+ (body-test/execute-n-times-in-parallel
+ #(.asLong (js/execute-fn-name context "plus" 1 1))
+ 10))))))
--
GitLab