Merge pull request #3034 from finos/fix-multi-server-threadsafe

texodus · web-flow · commit 1d44e93ce8c0 · 2025-07-13T12:46:43.000-04:00
Fix thread safety of multiple `Server` instances in the same process
diff --git a/cpp/perspective/src/cpp/computed_expression.cpp b/cpp/perspective/src/cpp/computed_expression.cpp
@@ -16,24 +16,6 @@
 
 namespace perspective {
 
-// Change ExprTk's default compilation options to only check for correctness
-// of brackets and sequences. ExprTk defaults will replace "true" and "false"
-// with 1 and 0, which we don't want. Using the tokens "true" and "false"
-// will raise a syntax error, which is the correct behavior.
-std::size_t t_computed_expression_parser::PARSER_COMPILE_OPTIONS =
-    exprtk::parser<t_tscalar>::settings_t::e_joiner
-    + exprtk::parser<t_tscalar>::settings_t::e_numeric_check
-    + exprtk::parser<t_tscalar>::settings_t::e_bracket_check
-    + exprtk::parser<t_tscalar>::settings_t::e_sequence_check;
-// exprtk::parser<t_tscalar>::settings_t::e_commutative_check;
-// exprtk::parser<t_tscalar>::settings_t::e_strength_reduction;
-
-std::shared_ptr<exprtk::parser<t_tscalar>>
-    t_computed_expression_parser::PARSER =
-        std::make_shared<exprtk::parser<t_tscalar>>(
-            t_computed_expression_parser::PARSER_COMPILE_OPTIONS
-        );
-
 computed_function::bucket t_computed_expression_parser::BUCKET_FN =
     computed_function::bucket();
 
@@ -160,13 +142,13 @@ t_computed_expression::compute(
 
     expr_definition.register_symbol_table(sym_table);
 
-    if (!t_computed_expression_parser::PARSER->compile(
+    if (!m_computed_expression_parser.m_parser->compile(
             m_parsed_expression_string, expr_definition
         )) {
         std::stringstream ss;
         ss << "[t_computed_expression::compute] Failed to parse expression: `"
            << m_parsed_expression_string << "`, failed with error: "
-           << t_computed_expression_parser::PARSER->error() << '\n';
+           << m_computed_expression_parser.m_parser->error() << '\n';
 
         PSP_COMPLAIN_AND_ABORT(ss.str());
     }
@@ -227,9 +209,22 @@ t_computed_expression::get_dtype() const {
  * t_computed_expression_parser
  */
 
-void
-t_computed_expression_parser::init() {
-    t_computed_expression_parser::PARSER->settings()
+t_computed_expression_parser::t_computed_expression_parser() {
+    // Change ExprTk's default compilation options to only check for correctness
+    // of brackets and sequences. ExprTk defaults will replace "true" and
+    // "false" with 1 and 0, which we don't want. Using the tokens "true" and
+    // "false" will raise a syntax error, which is the correct behavior.
+
+    m_parser = std::make_shared<exprtk::parser<t_tscalar>>(
+        exprtk::parser<t_tscalar>::settings_t::e_joiner
+        + exprtk::parser<t_tscalar>::settings_t::e_numeric_check
+        + exprtk::parser<t_tscalar>::settings_t::e_bracket_check
+        + exprtk::parser<t_tscalar>::settings_t::e_sequence_check
+        // exprtk::parser<t_tscalar>::settings_t::e_commutative_check;
+        // exprtk::parser<t_tscalar>::settings_t::e_strength_reduction;
+    );
+
+    m_parser->settings()
         .disable_control_structure(
             exprtk::parser<t_tscalar>::settings_store::e_ctrl_repeat_loop
         )
@@ -255,7 +250,7 @@ t_computed_expression_parser::precompute(
     const std::shared_ptr<t_schema>& schema,
     t_expression_vocab& vocab,
     t_regex_mapping& regex_mapping
-) {
+) const {
     exprtk::symbol_table<t_tscalar> sym_table;
     sym_table.add_constants();
 
@@ -299,14 +294,12 @@ t_computed_expression_parser::precompute(
     exprtk::expression<t_tscalar> expr_definition;
     expr_definition.register_symbol_table(sym_table);
 
-    if (!t_computed_expression_parser::PARSER->compile(
-            parsed_expression_string, expr_definition
-        )) {
+    if (!m_parser->compile(parsed_expression_string, expr_definition)) {
         std::stringstream ss;
         ss << "[t_computed_expression_parser::precompute] Failed to parse "
               "expression: `"
-           << parsed_expression_string << "`, failed with error: "
-           << t_computed_expression_parser::PARSER->error() << '\n';
+           << parsed_expression_string
+           << "`, failed with error: " << m_parser->error() << '\n';
         PSP_COMPLAIN_AND_ABORT(ss.str());
     }
 
@@ -334,7 +327,7 @@ t_computed_expression_parser::get_dtype(
     t_expression_error& error,
     t_expression_vocab& vocab,
     t_regex_mapping& regex_mapping
-) {
+) const {
     exprtk::symbol_table<t_tscalar> sym_table;
     sym_table.add_constants();
 
@@ -391,14 +384,11 @@ t_computed_expression_parser::get_dtype(
     exprtk::expression<t_tscalar> expr_definition;
     expr_definition.register_symbol_table(sym_table);
 
-    if (!t_computed_expression_parser::PARSER->compile(
-            parsed_expression_string, expr_definition
-        )) {
+    if (!m_parser->compile(parsed_expression_string, expr_definition)) {
         // Error count should always be above 0 if there is a compile error -
         // We simply take the first error and return it.
-        if (t_computed_expression_parser::PARSER->error_count() > 0) {
-            auto parser_error =
-                t_computed_expression_parser::PARSER->get_error(0);
+        if (m_parser->error_count() > 0) {
+            auto parser_error = m_parser->get_error(0);
 
             // Given an error object and an expression, `update_error` maps the
             // error to a line and column number inside the expression.
diff --git a/cpp/perspective/src/cpp/computed_function.cpp b/cpp/perspective/src/cpp/computed_function.cpp
@@ -1182,13 +1182,13 @@ bucket::operator()(t_parameter_list parameters) {
     t_scalar_view temp_val(gt_val);
     val.set(temp_val());
 
+    // Bucket by numeric value
+    t_scalar_view temp_unit2(gt_unit);
+    unit.set(temp_unit2());
+
     if (val.is_numeric()) {
         rval.m_type = DTYPE_FLOAT64;
 
-        // Bucket by numeric value
-        t_scalar_view temp_unit(gt_unit);
-        unit.set(temp_unit());
-
         // type-check
         if (!unit.is_numeric() || val.m_status == STATUS_CLEAR
             || unit.m_status == STATUS_CLEAR) {
@@ -1206,10 +1206,10 @@ bucket::operator()(t_parameter_list parameters) {
     }
 
     // Must be a datetime - second parameter is a string
-    t_string_view temp_string(gt_unit);
-    std::string unit_str = std::string(temp_string.begin(), temp_string.end());
+    const auto unit_str = std::string(unit.get<const char*>());
     char temp_unit = 0;
-    auto len = unit_str.size();
+    const auto len = unit_str.size();
+
     unsigned long multiplicity;
     t_date_bucket_unit date_unit;
     if (len == 0) {
@@ -1245,13 +1245,6 @@ bucket::operator()(t_parameter_list parameters) {
     // type-check multiplicity
     switch (date_unit) {
         case t_date_bucket_unit::SECONDS:
-            if (multiplicity != 1 && multiplicity != 5 && multiplicity != 10
-                && multiplicity != 15 && multiplicity != 20
-                && multiplicity != 30) {
-                rval.m_status = STATUS_CLEAR;
-                return rval;
-            }
-            break;
         case t_date_bucket_unit::MINUTES:
             if (multiplicity != 1 && multiplicity != 5 && multiplicity != 10
                 && multiplicity != 15 && multiplicity != 20
@@ -1270,11 +1263,6 @@ bucket::operator()(t_parameter_list parameters) {
             break;
         case t_date_bucket_unit::DAYS:
             // TODO: day multiplicity.
-            if (multiplicity != 1) {
-                rval.m_status = STATUS_CLEAR;
-                return rval;
-            }
-            break;
         case t_date_bucket_unit::WEEKS:
             // TODO: week multiplicity
             if (multiplicity != 1) {
diff --git a/cpp/perspective/src/cpp/server.cpp b/cpp/perspective/src/cpp/server.cpp
@@ -307,7 +307,7 @@ re_intern_strings(std::string&& expression) {
 static auto
 re_unintern_some_exprs(std::string&& expression) {
     static const RE2 interned_param(
-        "(?:bucket|match|match_all|search|indexof|replace|replace_all)\\("
+        "(?:match|match_all|search|indexof|replace|replace_all)\\("
         "(?:.*?,\\s*(intern\\(('.*?')\\)))"
     );
     static const RE2 intern_match("intern\\(('.*?')\\)");
@@ -1291,12 +1291,6 @@ coerce_to(const t_dtype dtype, const A& val) {
 
 std::vector<ProtoServerResp<ProtoServer::Response>>
 ProtoServer::_handle_request(std::uint32_t client_id, Request&& req) {
-    static bool is_init_expr = false;
-    if (!is_init_expr) {
-        t_computed_expression_parser::init();
-        is_init_expr = true;
-    }
-
     std::vector<ProtoServerResp<ProtoServer::Response>> proto_resp;
     // proto::Response resp_env;
 
@@ -1810,7 +1804,7 @@ ProtoServer::_handle_request(std::uint32_t client_id, Request&& req) {
                 t_regex_mapping& regex_mapping = *expression_regex_mapping;
 
                 std::shared_ptr<t_computed_expression> computed_expression =
-                    t_computed_expression_parser::precompute(
+                    m_computed_expression_parser.precompute(
                         expr.expression_alias,
                         expr.expression,
                         expr.parse_expression_string,
diff --git a/cpp/perspective/src/cpp/table.cpp b/cpp/perspective/src/cpp/table.cpp
@@ -13,6 +13,7 @@
 #include "perspective/arrow_loader.h"
 #include "perspective/base.h"
 #include "perspective/column.h"
+#include "perspective/computed_expression.h"
 #include "perspective/data_table.h"
 #include "perspective/raw_types.h"
 #include "perspective/schema.h"
@@ -49,6 +50,7 @@ Table::Table(
     m_limit(limit),
     m_index(std::move(index)),
     m_gnode_set(false) {
+
     validate_columns(m_column_names);
 }
 
@@ -152,7 +154,7 @@ Table::validate_expressions(
 
         const auto& column_ids = std::get<3>(expr);
 
-        t_dtype expression_dtype = t_computed_expression_parser::get_dtype(
+        t_dtype expression_dtype = m_computed_expression_parser.get_dtype(
             expression_alias,
             expression_string,
             parsed_expression_string,
diff --git a/cpp/perspective/src/include/perspective/computed_expression.h b/cpp/perspective/src/include/perspective/computed_expression.h
@@ -40,48 +40,11 @@ struct PERSPECTIVE_EXPORT t_expression_error {
     t_index m_column;
 };
 
-/**
- * @brief Contains the metadata for a single expression and the methods which
- * will compute the expression's output.
- */
-class PERSPECTIVE_EXPORT t_computed_expression {
-public:
-    PSP_NON_COPYABLE(t_computed_expression);
-
-    t_computed_expression(
-        std::string expression_alias,
-        std::string expression_string,
-        std::string parsed_expression_string,
-        const std::vector<std::pair<std::string, std::string>>& column_ids,
-        t_dtype dtype
-    );
-
-    void compute(
-        const std::shared_ptr<t_data_table>& source_table,
-        const t_gstate::t_mapping& pkey_map,
-        const std::shared_ptr<t_data_table>& destination_table,
-        t_expression_vocab& vocab,
-        t_regex_mapping& regex_mapping
-    ) const;
-
-    const std::string& get_expression_alias() const;
-    const std::string& get_expression_string() const;
-    const std::string& get_parsed_expression_string() const;
-    const std::vector<std::pair<std::string, std::string>>&
-    get_column_ids() const;
-    t_dtype get_dtype() const;
-
-private:
-    std::string m_expression_alias;
-    std::string m_expression_string;
-    std::string m_parsed_expression_string;
-    std::vector<std::pair<std::string, std::string>> m_column_ids;
-    t_dtype m_dtype;
-};
+class PERSPECTIVE_EXPORT t_computed_expression;
 
 class PERSPECTIVE_EXPORT t_computed_expression_parser {
 public:
-    static void init();
+    t_computed_expression_parser();
 
     /**
      * @brief Given expression strings, validate the expression's dtype and
@@ -100,7 +63,7 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
      * @param schema
      * @return std::shared_ptr<t_computed_expression>
      */
-    static std::shared_ptr<t_computed_expression> precompute(
+    std::shared_ptr<t_computed_expression> precompute(
         const std::string& expression_alias,
         const std::string& expression_string,
         const std::string& parsed_expression_string,
@@ -110,7 +73,7 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
         const std::shared_ptr<t_schema>& schema,
         t_expression_vocab& vocab,
         t_regex_mapping& regex_mapping
-    );
+    ) const;
 
     /**
      * @brief Returns the dtype of the given expression, or `DTYPE_NONE`
@@ -132,7 +95,7 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
      * @param error_string
      * @return t_dtype
      */
-    static t_dtype get_dtype(
+    t_dtype get_dtype(
         const std::string& expression_alias,
         const std::string& expression_string,
         const std::string& parsed_expression_string,
@@ -143,12 +106,9 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
         t_expression_error& error,
         t_expression_vocab& vocab,
         t_regex_mapping& regex_mapping
-    );
-
-    static std::shared_ptr<exprtk::parser<t_tscalar>> PARSER;
+    ) const;
 
-    // Applied to the parser
-    static std::size_t PARSER_COMPILE_OPTIONS;
+    std::shared_ptr<exprtk::parser<t_tscalar>> m_parser;
 
     // Static computed functions have no state
     static computed_function::bucket BUCKET_FN;
@@ -176,6 +136,46 @@ class PERSPECTIVE_EXPORT t_computed_expression_parser {
     static t_tscalar FALSE_SCALAR;
 };
 
+/**
+ * @brief Contains the metadata for a single expression and the methods which
+ * will compute the expression's output.
+ */
+class PERSPECTIVE_EXPORT t_computed_expression {
+public:
+    PSP_NON_COPYABLE(t_computed_expression);
+
+    t_computed_expression(
+        std::string expression_alias,
+        std::string expression_string,
+        std::string parsed_expression_string,
+        const std::vector<std::pair<std::string, std::string>>& column_ids,
+        t_dtype dtype
+    );
+
+    void compute(
+        const std::shared_ptr<t_data_table>& source_table,
+        const t_gstate::t_mapping& pkey_map,
+        const std::shared_ptr<t_data_table>& destination_table,
+        t_expression_vocab& vocab,
+        t_regex_mapping& regex_mapping
+    ) const;
+
+    const std::string& get_expression_alias() const;
+    const std::string& get_expression_string() const;
+    const std::string& get_parsed_expression_string() const;
+    const std::vector<std::pair<std::string, std::string>>&
+    get_column_ids() const;
+    t_dtype get_dtype() const;
+
+private:
+    std::string m_expression_alias;
+    std::string m_expression_string;
+    std::string m_parsed_expression_string;
+    t_computed_expression_parser m_computed_expression_parser;
+    std::vector<std::pair<std::string, std::string>> m_column_ids;
+    t_dtype m_dtype;
+};
+
 /**
  * @brief a `t_schema`-like container for validated expression results that
  * offers fast lookups.
diff --git a/cpp/perspective/src/include/perspective/server.h b/cpp/perspective/src/include/perspective/server.h
@@ -663,6 +663,7 @@ namespace server {
         static std::uint32_t m_client_id;
         bool m_realtime_mode;
         ServerResources m_resources;
+        t_computed_expression_parser m_computed_expression_parser;
     };
 
 } // namespace server
diff --git a/cpp/perspective/src/include/perspective/table.h b/cpp/perspective/src/include/perspective/table.h
@@ -11,6 +11,7 @@
 // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
 
 #pragma once
+#include "perspective/computed_expression.h"
 #include <perspective/first.h>
 #include <perspective/exports.h>
 #include <perspective/base.h>
@@ -268,6 +269,7 @@ class PERSPECTIVE_EXPORT Table {
     std::shared_ptr<t_gnode> m_gnode;
     std::vector<std::string> m_column_names;
     std::vector<t_dtype> m_data_types;
+    t_computed_expression_parser m_computed_expression_parser;
 
     /**
      * @brief The row number at which we start to write into the Table.
diff --git a/rust/perspective-python/perspective/tests/multi_threaded/test_multi_threaded.py b/rust/perspective-python/perspective/tests/multi_threaded/test_multi_threaded.py
