From 4e64447671ae530661c946c2e4f6b863213c7b33 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Tue, 25 Aug 2026 18:48:36 -0700 Subject: [PATCH] Enable free-threaded Python and improve concurrency in CEL Python. - Declare free-threading compatibility on pybind11 modules (`mod_gil_not_used`) and BUILD targets (`freethreading_compatible`). - Introduce `FreeThreadingMutex`, a zero-cost abstraction that compiles away in GIL-enabled builds and uses `absl::Mutex` in free-threaded mode (`Py_GIL_DISABLED`). - Protect lazy `cel_program_` compilation in `PyCelExpression` with `FreeThreadingMutex`. - Protect Python object caching and lazy element resolution in `PyCelValue` and list/map accessors with `FreeThreadingMutex`. - Protect `PyMessageFactory::message_classes_` with `FreeThreadingMutex`. - Extract `BuildCompiler` and `BuildRuntime` to construct compiler/runtime objects without holding mutex locks, using double-checked locking in `GetCompiler` and `GetRuntime`. - Eagerly resolve `variable_types_` during `PyCelEnvInternal` initialization, making `GetVariableType` lock-free and thread-safe. - Release GIL during CPU-bound policy compilation and YAML parsing. - Avoid unconditional `gil_scoped_acquire` across destructors and error conversions when GIL state is already held (`PyGILState_Check`). - Initialize pybind11 internals at module import time to prevent DSO internals races in multi-threaded environments. - Prevent reference leaks in custom function invocation by releasing arguments and temporary results. - Add `test_freethreaded.sh` script and concurrency tests in `cel_parallel_test.py` and `cel_test.py`. ### Performance Benchmarks (`cel_parallel_test`, `-c opt`) | Benchmark Stage / Test Case | Standard Mode (GIL) | Free-Threaded Mode (No GIL) | Speedup / Impact | | :--- | :---: | :---: | :---: | | **Multi-Threaded Compilation** (1,000 items, 8 threads) | 75.74 ms | 23.14 ms | **3.27x faster** (69.5% faster) | | **Sequential Compilation** (1,000 items) | 61.77 ms | 63.28 ms | 0.98x | | **Multi-Threaded Evaluation** (10,000 items, 8 threads) | 234.79 ms | 138.88 ms | **1.69x faster** (40.9% faster) | | **Sequential Evaluation** (10,000 items) | 90.68 ms | 112.74 ms | 0.80x | PiperOrigin-RevId: 970936071 --- cel_expr_python/BUILD | 5 + cel_expr_python/cel_extension.h | 4 +- cel_expr_python/cel_parallel_test.py | 41 +++++++ cel_expr_python/cel_test.py | 27 +++++ cel_expr_python/free_threading_mutex.h | 85 +++++++++++++++ cel_expr_python/py_cel_env.cc | 1 + cel_expr_python/py_cel_env_internal.cc | 114 +++++++++++++------- cel_expr_python/py_cel_env_internal.h | 27 ++--- cel_expr_python/py_cel_expression.cc | 59 ++++++---- cel_expr_python/py_cel_expression.h | 14 ++- cel_expr_python/py_cel_function.cc | 8 +- cel_expr_python/py_cel_function_decl.cc | 3 - cel_expr_python/py_cel_module.cc | 4 +- cel_expr_python/py_cel_type.cc | 1 + cel_expr_python/py_cel_value.cc | 125 ++++++++++++++++------ cel_expr_python/py_cel_value.h | 37 +++---- cel_expr_python/py_descriptor_database.cc | 11 +- cel_expr_python/py_descriptor_database.h | 1 + cel_expr_python/py_error_status.cc | 12 ++- cel_expr_python/py_error_status.h | 10 ++ cel_expr_python/py_message_factory.cc | 78 +++++++++----- cel_expr_python/py_message_factory.h | 6 +- test_freethreaded.sh | 64 +++++++++++ 23 files changed, 565 insertions(+), 172 deletions(-) create mode 100644 cel_expr_python/free_threading_mutex.h create mode 100755 test_freethreaded.sh diff --git a/cel_expr_python/BUILD b/cel_expr_python/BUILD index cbc08ca..4072870 100644 --- a/cel_expr_python/BUILD +++ b/cel_expr_python/BUILD @@ -29,6 +29,7 @@ pybind_library( "py_message_factory.cc", ], hdrs = [ + "free_threading_mutex.h", "py_cel_activation.h", "py_cel_arena.h", "py_cel_env.h", @@ -46,6 +47,7 @@ pybind_library( "py_descriptor_database.h", "py_message_factory.h", ], + tags = ["freethreading_compatible"], visibility = [":__subpackages__"], deps = [ ":cel_extension", @@ -113,6 +115,7 @@ pybind_extension( "//visibility:public", ], deps = [ + ":cel_extension", ":cel_pybind_lib", ], ) @@ -127,6 +130,7 @@ pybind_library( "cel_extension.h", "py_error_status.h", ], + tags = ["freethreading_compatible"], visibility = ["//visibility:public"], deps = [ ":status_macros", @@ -161,6 +165,7 @@ py_test( "//testing:proto2_test_all_types_py_pb2", "@com_google_absl_py//absl/testing:absltest", "@com_google_protobuf//:protobuf", + "@com_google_protobuf//:protobuf_python", ] + select({ "@platforms//os:windows": [], "//conditions:default": [":cel"], diff --git a/cel_expr_python/cel_extension.h b/cel_expr_python/cel_extension.h index 48a7151..e8672cd 100644 --- a/cel_expr_python/cel_extension.h +++ b/cel_expr_python/cel_extension.h @@ -80,14 +80,14 @@ class CelExtension { // CEL_EXTENSION_MODULE(sample_cel_ext, SampleCelExtension); // #define CEL_EXTENSION_MODULE(module_name, class_name) \ - PYBIND11_MODULE(module_name, m) { \ + PYBIND11_MODULE(module_name, m, pybind11::mod_gil_not_used()) { \ pybind11::module_::import(CEL_MODULE_NAME); \ pybind11::class_(m, #class_name) \ .def(pybind11::init<>()); \ } #define CEL_VERSIONED_EXTENSION_MODULE(module_name, class_name) \ - PYBIND11_MODULE(module_name, m) { \ + PYBIND11_MODULE(module_name, m, pybind11::mod_gil_not_used()) { \ pybind11::module_::import(CEL_MODULE_NAME); \ pybind11::class_(m, #class_name) \ .def(pybind11::init<>()) \ diff --git a/cel_expr_python/cel_parallel_test.py b/cel_expr_python/cel_parallel_test.py index 479f2aa..5a41fbe 100644 --- a/cel_expr_python/cel_parallel_test.py +++ b/cel_expr_python/cel_parallel_test.py @@ -199,6 +199,47 @@ def testMultiThreadedCompilation(self): def testSequentialCompilation(self): self._test_compile(multi_threaded=False) + def testSharedValueConcurrentAccess(self): + expr_scalar = self.env.compile("var_int * 2") + expr_list = self.env.compile("[var_int, var_int + 1, var_int + 2]") + expr_map = self.env.compile("{'key': var_str, 'value': var_int}") + + def run_concurrent_value_test(n: int): + val_scalar = expr_scalar.eval(data={"var_int": n}) + val_list = expr_list.eval(data={"var_int": n}) + val_map = expr_map.eval(data={"var_str": f"k_{n}", "var_int": n}) + + def read_values(_): + # Concurrently access plain_value, value, type, and repr + # on shared instances + self.assertEqual(val_scalar.plain_value(), n * 2) + self.assertEqual(val_scalar.value(), n * 2) + self.assertEqual(val_scalar.type(), cel.Type.INT) + self.assertNotEmpty(str(val_scalar)) + + plain_list = val_list.plain_value() + self.assertEqual(plain_list, [n, n + 1, n + 2]) + list_accessors = val_list.value() + for idx, item in enumerate(list_accessors): + self.assertEqual(item.plain_value(), n + idx) + self.assertEqual(item.value(), n + idx) + self.assertEqual(item.type(), cel.Type.INT) + self.assertNotEmpty(str(item)) + + plain_map = val_map.plain_value() + self.assertEqual(plain_map, {"key": f"k_{n}", "value": n}) + map_accessors = val_map.value() + self.assertEqual(map_accessors["key"].plain_value(), f"k_{n}") + self.assertEqual(map_accessors["key"].value(), f"k_{n}") + self.assertEqual(map_accessors["value"].plain_value(), n) + self.assertEqual(map_accessors["value"].value(), n) + + with concurrent.futures.ThreadPoolExecutor(max_workers=8) as executor: + list(executor.map(read_values, range(50))) + + for i in range(10): + run_concurrent_value_test(i) + if __name__ == "__main__": absltest.main() diff --git a/cel_expr_python/cel_test.py b/cel_expr_python/cel_test.py index 2add977..9eb6100 100644 --- a/cel_expr_python/cel_test.py +++ b/cel_expr_python/cel_test.py @@ -21,6 +21,7 @@ import sys from typing import Any +from google.protobuf import message_factory from google.protobuf import duration_pb2 as duration_pb from google.protobuf import timestamp_pb2 as timestamp_pb from absl.testing import absltest @@ -337,6 +338,32 @@ def testProto_unexpectedType(self): r" .*. \(Expected cel.expr.conformance.proto2.TestAllTypes\)", ) + def testProto_cannotFindMessageClass(self): + orig_get_message_class = message_factory.GetMessageClass + + def failing_get_message_class(descriptor): + del descriptor + raise TypeError( + "Couldn't build proto class because dependency X is missing" + ) + + try: + message_factory.GetMessageClass = failing_get_message_class + env = cel.NewEnv(options=self.options) + expr = env.compile( + "cel.expr.conformance.proto2.TestAllTypes{single_string: 'hello'}" + ) + res = expr.eval(env.Activation()) + with self.assertRaisesRegex( + TypeError, "Couldn't find message class for type" + ): + res.plain_value() + del res + del expr + del env + finally: + message_factory.GetMessageClass = orig_get_message_class + def testEvalList(self): res: cel.Value = self._eval( "[1, 'CEL', true]", expected_return_type=cel.Type.LIST diff --git a/cel_expr_python/free_threading_mutex.h b/cel_expr_python/free_threading_mutex.h new file mode 100644 index 0000000..2501f46 --- /dev/null +++ b/cel_expr_python/free_threading_mutex.h @@ -0,0 +1,85 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_CEL_PYTHON_FREE_THREADING_MUTEX_H_ +#define THIRD_PARTY_CEL_PYTHON_FREE_THREADING_MUTEX_H_ + +#include // IWYU pragma: keep - Needed for Py_GIL_DISABLED + +#include "absl/base/attributes.h" +#include "absl/base/thread_annotations.h" + +#ifndef Py_GIL_DISABLED +#else +#include "absl/synchronization/mutex.h" +#endif + +namespace cel_python { + +// Zero-cost mutex wrapper that compiles away to nothing in standard GIL builds, +// and uses absl::Mutex in free-threaded builds (Py_GIL_DISABLED). +class ABSL_LOCKABLE ABSL_ATTRIBUTE_WARN_UNUSED FreeThreadingMutex { + public: + FreeThreadingMutex() = default; + FreeThreadingMutex(const FreeThreadingMutex&) = delete; + FreeThreadingMutex& operator=(const FreeThreadingMutex&) = delete; + +#ifndef Py_GIL_DISABLED + // In GIL-enabled builds, this mutex compiles away to zero-cost no-ops while + // retaining thread-safety annotations for Clang static analysis. + // + // Relationship with the GIL: + // - For operations accessing Python state or cached PyObjects (such as + // PyCelValue::Value() or PyMessageFactory::GetMessageClass()), mutual + // exclusion between threads is provided by the Python GIL itself. + // - For pure C++ operations (such as PyCelEnv::Compile() or deserialization), + // the GIL is intentionally released (via py::gil_scoped_release) to enable + // parallel execution and prevent deadlocks with DescriptorPool's mutex. + // Objects may therefore be constructed or moved while the GIL is NOT held. + // - Consequently, Lock() and Unlock() do not assert PyGILState_Check(), + // allowing lock acquisition and move semantics to function safely whether + // the GIL is currently held or released. Call sites that strictly require + // the GIL explicitly verify or acquire it themselves. + void Lock() ABSL_EXCLUSIVE_LOCK_FUNCTION() {} + void Unlock() ABSL_UNLOCK_FUNCTION() {} +#else + // Free-threaded build: real mutex + void Lock() ABSL_EXCLUSIVE_LOCK_FUNCTION() { mutex_.Lock(); } + void Unlock() ABSL_UNLOCK_FUNCTION() { mutex_.Unlock(); } + + private: + absl::Mutex mutex_; +#endif +}; + +// RAII lock guard for FreeThreadingMutex. +class ABSL_SCOPED_LOCKABLE FreeThreadingLockGuard { + public: + explicit FreeThreadingLockGuard(FreeThreadingMutex& mutex) + ABSL_EXCLUSIVE_LOCK_FUNCTION(mutex) + : mutex_(mutex) { + mutex_.Lock(); + } + ~FreeThreadingLockGuard() ABSL_UNLOCK_FUNCTION() { mutex_.Unlock(); } + + FreeThreadingLockGuard(const FreeThreadingLockGuard&) = delete; + FreeThreadingLockGuard& operator=(const FreeThreadingLockGuard&) = delete; + + private: + FreeThreadingMutex& mutex_; +}; + +} // namespace cel_python + +#endif // THIRD_PARTY_CEL_PYTHON_FREE_THREADING_MUTEX_H_ diff --git a/cel_expr_python/py_cel_env.cc b/cel_expr_python/py_cel_env.cc index 68310be..6838bef 100644 --- a/cel_expr_python/py_cel_env.cc +++ b/cel_expr_python/py_cel_env.cc @@ -212,6 +212,7 @@ PyCelExpression PyCelEnv::Compile(const std::string& cel_expr, } PyCelExpression PyCelEnv::Deserialize(const std::string& serialized_expr) { + py::gil_scoped_release gil_release; return ThrowIfError(PyCelExpression::Deserialize(env_, serialized_expr)); } diff --git a/cel_expr_python/py_cel_env_internal.cc b/cel_expr_python/py_cel_env_internal.cc index cd2a58e..4bb32f8 100644 --- a/cel_expr_python/py_cel_env_internal.cc +++ b/cel_expr_python/py_cel_env_internal.cc @@ -20,12 +20,12 @@ #include #include +#include "absl/base/call_once.h" #include "absl/container/flat_hash_map.h" #include "absl/log/absl_check.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/synchronization/mutex.h" #include "checker/type_checker_builder.h" #include "common/container.h" #include "common/function_descriptor.h" @@ -34,6 +34,7 @@ #include "compiler/compiler.h" #include "env/config.h" #include "env/env.h" +#include "env/env_runtime.h" #include "env/env_std_extensions.h" #include "env/runtime_std_extensions.h" #include "env/type_info.h" @@ -107,8 +108,24 @@ PyCelEnvInternal::PyCelEnvInternal( return extension->ConfigureRuntime(runtime_builder, runtime_options); }); } + + // PyCelType::FromCelType performs a deep copy and does not keep a + // reference to any of the arena backed cel::Type instances, so it is safe to + // use a local arena. + google::protobuf::Arena arena; + for (const cel::Config::VariableConfig& variable_config : + env_config_.GetConfig().GetVariableConfigs()) { + auto status_or_type = cel::TypeInfoToType(variable_config.type_info, + descriptor_pool_.get(), &arena); + if (status_or_type.ok()) { + variable_types_[variable_config.name] = + PyCelType::FromCelType(*status_or_type); + } + } } +PyCelEnvInternal::~PyCelEnvInternal() = default; + absl::StatusOr> PyCelEnvInternal::NewCelEnvInternal( const PyCelEnvConfig& env_config, const PyCelOptions& options, @@ -230,12 +247,8 @@ PyCelEnvInternal::NewCelEnvInternal( std::move(extension_handles), impls)); } -absl::StatusOr PyCelEnvInternal::GetCompiler() const { - absl::MutexLock lock(mutex_); - if (compiler_) { - return compiler_.get(); - } - +absl::StatusOr> PyCelEnvInternal::BuildCompiler() + const { const cel::Config& config = env_config_.GetConfig(); CEL_PYTHON_ASSIGN_OR_RETURN( @@ -259,28 +272,19 @@ absl::StatusOr PyCelEnvInternal::GetCompiler() const { } checker_builder.SetExpressionContainer(std::move(container)); - // Convert variable types from cel::TypeInfo to PyCelType. - google::protobuf::Arena* arena = checker_builder.arena(); - for (const cel::Config::VariableConfig& variable_config : - config.GetVariableConfigs()) { - CEL_PYTHON_ASSIGN_OR_RETURN( - cel::Type cel_type, cel::TypeInfoToType(variable_config.type_info, - descriptor_pool_.get(), arena)); - PyCelType py_cel_type = PyCelType::FromCelType(cel_type); - variable_types_[variable_config.name] = py_cel_type; - } - - CEL_PYTHON_ASSIGN_OR_RETURN(compiler_, compiler_builder->Build()); - return compiler_.get(); + return compiler_builder->Build(); } -absl::StatusOr PyCelEnvInternal::GetRuntime( - RuntimeMode runtime_mode) const { - absl::MutexLock lock(mutex_); - if (auto it = runtimes_.find(runtime_mode); it != runtimes_.end()) { - return it->second.get(); +absl::StatusOr PyCelEnvInternal::GetCompiler() const { + absl::call_once(compiler_once_, [this] { compiler_ = BuildCompiler(); }); + if (!compiler_.ok()) { + return compiler_.status(); } + return (*compiler_).get(); +} +absl::StatusOr> PyCelEnvInternal::BuildRuntime( + RuntimeMode runtime_mode) const { cel::RuntimeOptions opts; opts.container = env_config_.GetConfig().GetContainerConfig().name; opts.enable_empty_wrapper_null_unboxing = true; @@ -298,6 +302,10 @@ absl::StatusOr PyCelEnvInternal::GetRuntime( CEL_PYTHON_RETURN_IF_ERROR(cel::EnableReferenceResolver( builder, cel::ReferenceResolverEnabled::kAlways)); + // The local arena is only used as scratch space for intermediate cel::Type + // objects in TypeInfoToType. Parameters only retain cel::Kind (enum), and + // return types are converted to self-contained PyCelType value objects. + google::protobuf::Arena arena; for (const cel::Config::FunctionConfig& function_config : GetEnvConfig().GetConfig().GetFunctionConfigs()) { for (const cel::Config::FunctionOverloadConfig& overload_config : @@ -306,14 +314,20 @@ absl::StatusOr PyCelEnvInternal::GetRuntime( if (it == function_impls_.end()) { continue; } - py::object py_function = it->second; + py::object py_function; + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + py_function = it->second; + } else { + py_function = it->second; + } std::vector param_kinds; param_kinds.reserve(overload_config.parameters.size()); for (const cel::Config::TypeInfo& parameter : overload_config.parameters) { CEL_PYTHON_ASSIGN_OR_RETURN( cel::Type type, - cel::TypeInfoToType(parameter, descriptor_pool_.get(), &arena_)); + cel::TypeInfoToType(parameter, descriptor_pool_.get(), &arena)); param_kinds.push_back(static_cast(type.kind())); } cel::FunctionDescriptor descriptor( @@ -322,23 +336,41 @@ absl::StatusOr PyCelEnvInternal::GetRuntime( CEL_PYTHON_ASSIGN_OR_RETURN( cel::Type return_type, cel::TypeInfoToType(overload_config.return_type, - descriptor_pool_.get(), &arena_)); + descriptor_pool_.get(), &arena)); CEL_PYTHON_RETURN_IF_ERROR(builder.function_registry().Register( - descriptor, std::make_unique( - function_config.name, - PyCelType::FromCelType(return_type), py_function))); + descriptor, + std::make_unique( + function_config.name, PyCelType::FromCelType(return_type), + std::move(py_function)))); } } - CEL_PYTHON_ASSIGN_OR_RETURN(std::unique_ptr runtime, - std::move(builder).Build()); - const cel::Runtime* runtime_ptr = runtime.get(); - runtimes_[runtime_mode] = std::move(runtime); - return runtime_ptr; + return std::move(builder).Build(); +} + +absl::StatusOr PyCelEnvInternal::GetRuntime( + RuntimeMode runtime_mode) const { + switch (runtime_mode) { + case kStandard: + absl::call_once(standard_runtime_once_, + [this] { standard_runtime_ = BuildRuntime(kStandard); }); + if (!standard_runtime_.ok()) { + return standard_runtime_.status(); + } + return (*standard_runtime_).get(); + case kStandardIgnoreWarnings: + absl::call_once(standard_ignore_warnings_runtime_once_, [this] { + standard_ignore_warnings_runtime_ = + BuildRuntime(kStandardIgnoreWarnings); + }); + if (!standard_ignore_warnings_runtime_.ok()) { + return standard_ignore_warnings_runtime_.status(); + } + return (*standard_ignore_warnings_runtime_).get(); + } } const PyCelType& PyCelEnvInternal::GetVariableType( const std::string& name) const { - absl::MutexLock lock(mutex_); auto it = variable_types_.find(name); if (it != variable_types_.end()) { return it->second; @@ -360,8 +392,12 @@ CelExtensionHandle::CelExtensionHandle(CelExtensionHandle&& other) CelExtensionHandle::~CelExtensionHandle() { if (py_extension_ != nullptr) { - py::gil_scoped_acquire acquire; - Py_DECREF(py_extension_); + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + Py_DECREF(py_extension_); + } else { + Py_DECREF(py_extension_); + } } } diff --git a/cel_expr_python/py_cel_env_internal.h b/cel_expr_python/py_cel_env_internal.h index fc69b34..95bfd14 100644 --- a/cel_expr_python/py_cel_env_internal.h +++ b/cel_expr_python/py_cel_env_internal.h @@ -22,18 +22,15 @@ #include #include -#include "absl/base/thread_annotations.h" +#include "absl/base/call_once.h" #include "absl/container/flat_hash_map.h" #include "absl/status/status.h" #include "absl/status/statusor.h" -#include "absl/synchronization/mutex.h" #include "common/container.h" #include "compiler/compiler.h" #include "env/env.h" #include "env/env_runtime.h" #include "runtime/runtime.h" -#include "runtime/runtime_builder.h" -#include "runtime/runtime_options.h" #include "cel_expr_python/cel_extension.h" #include "cel_expr_python/py_cel_env_config.h" #include "cel_expr_python/py_cel_function.h" @@ -42,7 +39,6 @@ #include "cel_expr_python/py_cel_type.h" #include "cel_expr_python/py_descriptor_database.h" #include "cel_expr_python/py_message_factory.h" -#include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/dynamic_message.h" #include "google/protobuf/message.h" @@ -74,7 +70,7 @@ class CelExtensionHandle { // the python side. class PyCelEnvInternal { public: - ~PyCelEnvInternal() = default; + ~PyCelEnvInternal(); static absl::StatusOr> NewCelEnvInternal( const PyCelEnvConfig& env_config, const PyCelOptions& options, PyObject* py_descriptor_pool, @@ -122,8 +118,10 @@ class PyCelEnvInternal { std::vector extension_handles, absl::flat_hash_map& function_impls); - mutable absl::Mutex mutex_; - mutable google::protobuf::Arena arena_ ABSL_GUARDED_BY(mutex_); + absl::StatusOr> BuildCompiler() const; + absl::StatusOr> BuildRuntime( + RuntimeMode runtime_mode) const; + cel::Env cel_env_; cel::EnvRuntime cel_env_runtime_; PyCelEnvConfig env_config_; @@ -132,13 +130,16 @@ class PyCelEnvInternal { std::shared_ptr descriptor_pool_; mutable google::protobuf::DynamicMessageFactory message_factory_; std::shared_ptr py_message_factory_; - mutable absl::flat_hash_map variable_types_ - ABSL_GUARDED_BY(mutex_); + absl::flat_hash_map variable_types_; std::vector extensions_; absl::flat_hash_map function_impls_; - mutable std::unique_ptr compiler_ ABSL_GUARDED_BY(mutex_); - mutable absl::flat_hash_map> - runtimes_ ABSL_GUARDED_BY(mutex_); + mutable absl::once_flag compiler_once_; + mutable absl::StatusOr> compiler_; + mutable absl::once_flag standard_runtime_once_; + mutable absl::StatusOr> standard_runtime_; + mutable absl::once_flag standard_ignore_warnings_runtime_once_; + mutable absl::StatusOr> + standard_ignore_warnings_runtime_; }; } // namespace cel_python diff --git a/cel_expr_python/py_cel_expression.cc b/cel_expr_python/py_cel_expression.cc index 1a7b557..53084d4 100644 --- a/cel_expr_python/py_cel_expression.cc +++ b/cel_expr_python/py_cel_expression.cc @@ -43,6 +43,7 @@ #include "parser/parser_interface.h" #include "runtime/embedder_context.h" #include "runtime/runtime.h" +#include "cel_expr_python/free_threading_mutex.h" #include "cel_expr_python/py_cel_activation.h" #include "cel_expr_python/py_cel_arena.h" #include "cel_expr_python/py_cel_env_internal.h" @@ -110,7 +111,7 @@ absl::StatusOr PyCelExpression::Compile( if (disable_check) { CEL_PYTHON_ASSIGN_OR_RETURN(auto s, cel::NewSource(cel_expr, "")); - PY_CEL_PYTHON_ASSIGN_OR_RETURN(auto ast, compiler->GetParser().Parse(*s)); + CEL_PYTHON_ASSIGN_OR_RETURN(auto ast, compiler->GetParser().Parse(*s)); ParsedExpr parsed_expr; CEL_PYTHON_RETURN_IF_ERROR(cel::AstToParsedExpr(*ast, &parsed_expr)); return PyCelExpression(parsed_expr, env); @@ -127,6 +128,15 @@ absl::StatusOr PyCelExpression::Compile( return PyCelExpression(checked_expr, env); } +PyCelExpression::PyCelExpression(PyCelExpression&& other) noexcept { + FreeThreadingLockGuard lock(other.mutex_); + expr_ = std::move(other.expr_); + env_ = std::move(other.env_); + cel_program_ = std::move(other.cel_program_); +} + +PyCelExpression::~PyCelExpression() = default; + PyCelType PyCelExpression::GetReturnType() { if (!std::holds_alternative(expr_)) { return PyCelType::Dyn(); @@ -144,36 +154,42 @@ PyCelType PyCelExpression::GetReturnType() { return PyCelType::FromTypeProto(it->second); } +absl::StatusOr PyCelExpression::GetProgram() { + FreeThreadingLockGuard lock(mutex_); + if (cel_program_) { + return cel_program_.get(); + } + if (std::holds_alternative(expr_)) { + CEL_PYTHON_ASSIGN_OR_RETURN( + const cel::Runtime* runtime, + env_->GetRuntime(PyCelEnvInternal::kStandardIgnoreWarnings)); + CEL_PYTHON_ASSIGN_OR_RETURN( + cel_program_, cel::extensions::ProtobufRuntimeAdapter::CreateProgram( + *runtime, std::get(expr_))); + } else { + CEL_PYTHON_ASSIGN_OR_RETURN(const cel::Runtime* runtime, + env_->GetRuntime(PyCelEnvInternal::kStandard)); + CEL_PYTHON_ASSIGN_OR_RETURN( + cel_program_, cel::extensions::ProtobufRuntimeAdapter::CreateProgram( + *runtime, std::get(expr_))); + } + return cel_program_.get(); +} + absl::StatusOr PyCelExpression::Eval( const PyCelActivation& activation) { ABSL_CHECK(PyGILState_Check()); - if (cel_program_ == nullptr) { - if (std::holds_alternative(expr_)) { - PY_CEL_PYTHON_ASSIGN_OR_RETURN( - const cel::Runtime* runtime, - env_->GetRuntime(PyCelEnvInternal::kStandardIgnoreWarnings)); - PY_CEL_PYTHON_ASSIGN_OR_RETURN( - cel_program_, cel::extensions::ProtobufRuntimeAdapter::CreateProgram( - *runtime, std::get(expr_))); - } else { - PY_CEL_PYTHON_ASSIGN_OR_RETURN( - const cel::Runtime* runtime, - env_->GetRuntime(PyCelEnvInternal::kStandard)); - PY_CEL_PYTHON_ASSIGN_OR_RETURN( - cel_program_, cel::extensions::ProtobufRuntimeAdapter::CreateProgram( - *runtime, std::get(expr_))); - } - } + CEL_PYTHON_ASSIGN_OR_RETURN(const cel::Program* program, GetProgram()); std::shared_ptr arena = activation.GetArena(); std::shared_ptr env = activation.GetEnv(); cel::EmbedderContext embedder_context = cel::EmbedderContext::From(&env); cel::EvaluateOptions options; options.message_factory = env->GetMessageFactory(); options.embedder_context = &embedder_context; - PY_CEL_PYTHON_ASSIGN_OR_RETURN( + CEL_PYTHON_ASSIGN_OR_RETURN( cel::Value result, - cel_program_->Evaluate(arena->GetArena(), *activation.GetActivation(), - std::move(options))); + program->Evaluate(arena->GetArena(), *activation.GetActivation(), + std::move(options))); return PyCelValue(result, arena, std::move(env)); } @@ -190,7 +206,6 @@ std::string PyCelExpression::Serialize() const { absl::StatusOr PyCelExpression::Deserialize( const std::shared_ptr& env, const std::string& serialized_expr) { - ABSL_CHECK(PyGILState_Check()); google::protobuf::Any any; if (!any.ParseFromString(serialized_expr)) { return absl::InvalidArgumentError( diff --git a/cel_expr_python/py_cel_expression.h b/cel_expr_python/py_cel_expression.h index e4f59aa..acc1287 100644 --- a/cel_expr_python/py_cel_expression.h +++ b/cel_expr_python/py_cel_expression.h @@ -22,8 +22,10 @@ #include "cel/expr/checked.pb.h" #include "cel/expr/syntax.pb.h" +#include "absl/base/thread_annotations.h" #include "absl/status/statusor.h" #include "runtime/runtime.h" +#include "cel_expr_python/free_threading_mutex.h" #include "cel_expr_python/py_cel_activation.h" #include "cel_expr_python/py_cel_type.h" #include "cel_expr_python/py_cel_value.h" @@ -38,14 +40,15 @@ class PyCelExpression { public: static void DefinePythonBindings(pybind11::module& m); - PyCelExpression(PyCelExpression&& other) = default; + PyCelExpression(PyCelExpression&& other) noexcept; + ~PyCelExpression(); PyCelExpression(const cel::expr::ParsedExpr& parsed_expr, std::shared_ptr env) - : expr_(std::move(parsed_expr)), env_(std::move(env)) {} + : expr_(parsed_expr), env_(std::move(env)) {} PyCelExpression(const cel::expr::CheckedExpr& checked_expr, std::shared_ptr env) - : expr_(std::move(checked_expr)), env_(std::move(env)) {} + : expr_(checked_expr), env_(std::move(env)) {} PyCelType GetReturnType(); @@ -62,10 +65,13 @@ class PyCelExpression { const std::string& serialized_expr); private: + absl::StatusOr GetProgram(); + std::variant expr_; std::shared_ptr env_; - std::unique_ptr cel_program_; + mutable FreeThreadingMutex mutex_; + std::unique_ptr cel_program_ ABSL_GUARDED_BY(mutex_); }; } // namespace cel_python diff --git a/cel_expr_python/py_cel_function.cc b/cel_expr_python/py_cel_function.cc index c4e7a63..5ddd2b4 100644 --- a/cel_expr_python/py_cel_function.cc +++ b/cel_expr_python/py_cel_function.cc @@ -95,12 +95,14 @@ absl::StatusOr PyCelFunctionAdapter::Invoke( /*plain_value=*/true)); } PyObject* result = PyObject_CallObject(py_function_.ptr(), py_args); + Py_DECREF(py_args); absl::Status status = PyErr_toStatus(); if (!status.ok()) { + Py_XDECREF(result); return cel::ErrorValue(status); } - return PyObjectToCelValue( + absl::StatusOr cel_result = PyObjectToCelValue( result, return_type_, [this]() { return absl::StrFormat( @@ -108,6 +110,8 @@ absl::StatusOr PyCelFunctionAdapter::Invoke( PyUnicode_AsUTF8(PyObject_Repr(py_function_.ptr()))); }, env, context.arena()); -}; + Py_XDECREF(result); + return cel_result; +} } // namespace cel_python diff --git a/cel_expr_python/py_cel_function_decl.cc b/cel_expr_python/py_cel_function_decl.cc index 382fdae..b5f084b 100644 --- a/cel_expr_python/py_cel_function_decl.cc +++ b/cel_expr_python/py_cel_function_decl.cc @@ -16,13 +16,10 @@ #include #include -#include #include #include "env/config.h" -#include "env/type_info.h" #include "cel_expr_python/py_cel_overload.h" -#include "cel_expr_python/py_cel_type.h" #include #include diff --git a/cel_expr_python/py_cel_module.cc b/cel_expr_python/py_cel_module.cc index aca47fd..8879e00 100644 --- a/cel_expr_python/py_cel_module.cc +++ b/cel_expr_python/py_cel_module.cc @@ -24,11 +24,13 @@ #include "cel_expr_python/py_cel_python_extension.h" #include "cel_expr_python/py_cel_type.h" #include "cel_expr_python/py_cel_value.h" +#include "cel_expr_python/py_error_status.h" #include namespace cel_python { -PYBIND11_MODULE(cel, m) { +PYBIND11_MODULE(cel, m, pybind11::mod_gil_not_used()) { + InitPyErrorStatus(); m.doc() = "Python bindings for CEL."; PyCelArena::DefinePythonBindings(m); diff --git a/cel_expr_python/py_cel_type.cc b/cel_expr_python/py_cel_type.cc index be49498..52e1750 100644 --- a/cel_expr_python/py_cel_type.cc +++ b/cel_expr_python/py_cel_type.cc @@ -29,6 +29,7 @@ #include "absl/status/statusor.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" +#include "common/ast.h" #include "common/kind.h" #include "common/signature.h" #include "common/type.h" diff --git a/cel_expr_python/py_cel_value.cc b/cel_expr_python/py_cel_value.cc index 131de8d..df26c46 100644 --- a/cel_expr_python/py_cel_value.cc +++ b/cel_expr_python/py_cel_value.cc @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -36,6 +37,7 @@ #include "common/type.h" #include "common/value.h" #include "common/value_kind.h" +#include "cel_expr_python/free_threading_mutex.h" #include "cel_expr_python/py_cel_arena.h" #include "cel_expr_python/py_cel_env_internal.h" #include "cel_expr_python/py_cel_type.h" @@ -58,11 +60,19 @@ void PyCelValue::DefinePythonBindings(py::module& m) { .def("type", &PyCelValue::Type) .def("value", [](PyCelValue& self) { - return py::reinterpret_borrow(self.Value()); + PyObject* obj = self.Value(); + if (obj == nullptr) { + throw py::error_already_set(); + } + return py::reinterpret_borrow(obj); }) .def("plain_value", [](PyCelValue& self) { - return py::reinterpret_borrow(self.PlainValue()); + PyObject* obj = self.PlainValue(); + if (obj == nullptr) { + throw py::error_already_set(); + } + return py::reinterpret_borrow(obj); }) .def("__repr__", &PyCelValue::ToString); @@ -79,12 +89,28 @@ PyCelValue::PyCelValue(cel::Value& cel_value, std::shared_ptr arena, arena_(std::move(arena)), env_(std::move(env)) {} +PyCelValue::PyCelValue(PyCelValue&& other) noexcept + : object_(nullptr), plain_object_(nullptr) { + FreeThreadingLockGuard lock(other.mutex_); + cel_value_ = std::move(other.cel_value_); + arena_ = std::move(other.arena_); + env_ = std::move(other.env_); + object_ = other.object_; + plain_object_ = other.plain_object_; + other.object_ = nullptr; + other.plain_object_ = nullptr; +} + PyCelValue::~PyCelValue() { if (object_ || plain_object_) { - auto gil_state = PyGILState_Ensure(); - Py_XDECREF(object_); - Py_XDECREF(plain_object_); - PyGILState_Release(gil_state); + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + Py_XDECREF(object_); + Py_XDECREF(plain_object_); + } else { + Py_XDECREF(object_); + Py_XDECREF(plain_object_); + } } } @@ -92,12 +118,13 @@ PyCelType PyCelValue::Type() { return PyCelType::ForCelValue(cel_value_); } PyObject* PyCelValue::Value() { ABSL_CHECK(PyGILState_Check()); + FreeThreadingLockGuard lock(mutex_); if (object_) { return object_; } object_ = CelValueToPyObject(cel_value_, env_, arena_, /*plain_value=*/false); - if (object_ == nullptr) { + if (object_ == nullptr && !PyErr_Occurred()) { PyErr_SetString(PyExc_AssertionError, "Cannot create object"); } @@ -106,12 +133,13 @@ PyObject* PyCelValue::Value() { PyObject* PyCelValue::PlainValue() { ABSL_CHECK(PyGILState_Check()); + FreeThreadingLockGuard lock(mutex_); if (plain_object_) { return plain_object_; } plain_object_ = CelValueToPyObject(cel_value_, env_, arena_, /*plain_value=*/true); - if (plain_object_ == nullptr) { + if (plain_object_ == nullptr && !PyErr_Occurred()) { PyErr_SetString(PyExc_AssertionError, "Cannot create object"); } @@ -127,9 +155,12 @@ PyCelValueProvider::PyCelValueProvider(std::string name, PyObject* value, } PyCelValueProvider::~PyCelValueProvider() { - auto gil_state = PyGILState_Ensure(); - Py_DECREF(py_object_); - PyGILState_Release(gil_state); + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + Py_DECREF(py_object_); + } else { + Py_DECREF(py_object_); + } } cel::Value PyCelValueProvider::Provide( @@ -145,7 +176,15 @@ cel::Value PyCelValueProvider::Provide( return *converted_value; } -void PyCelListItemAccessor::ResolveElement() { +PyCelListItemAccessor::PyCelListItemAccessor( + PyCelListItemAccessor&& other) noexcept + : PyCelValue(std::move(other)), index_(other.index_) { + FreeThreadingLockGuard lock(other.mutex_); + resolved_ = other.resolved_; + element_value_ = std::move(other.element_value_); +} + +void PyCelListItemAccessor::ResolveElementLocked() { if (resolved_) { return; } @@ -163,47 +202,61 @@ void PyCelListItemAccessor::ResolveElement() { } PyCelType PyCelListItemAccessor::Type() { - auto gil_state = PyGILState_Ensure(); - ResolveElement(); - PyGILState_Release(gil_state); + FreeThreadingLockGuard lock(mutex_); + ResolveElementLocked(); return PyCelType::ForCelValue(element_value_); } PyObject* PyCelListItemAccessor::Value() { ABSL_CHECK(PyGILState_Check()); + FreeThreadingLockGuard lock(mutex_); if (object_) { return object_; } - ResolveElement(); + ResolveElementLocked(); object_ = CelValueToPyObject(element_value_, env_, arena_, /*plain_value=*/false); + if (object_ == nullptr && !PyErr_Occurred()) { + PyErr_SetString(PyExc_AssertionError, "Cannot create object"); + } return object_; } PyObject* PyCelListItemAccessor::PlainValue() { ABSL_CHECK(PyGILState_Check()); + FreeThreadingLockGuard lock(mutex_); if (plain_object_) { return plain_object_; } - ResolveElement(); + ResolveElementLocked(); plain_object_ = CelValueToPyObject(element_value_, env_, arena_, /*plain_value=*/true); + if (plain_object_ == nullptr && !PyErr_Occurred()) { + PyErr_SetString(PyExc_AssertionError, "Cannot create object"); + } return plain_object_; } std::string PyCelListItemAccessor::ToString() { - auto gil_state = PyGILState_Ensure(); - ResolveElement(); - std::string result = element_value_.DebugString(); - PyGILState_Release(gil_state); - return result; + FreeThreadingLockGuard lock(mutex_); + ResolveElementLocked(); + return element_value_.DebugString(); +} + +PyCelMapItemAccessor::PyCelMapItemAccessor( + PyCelMapItemAccessor&& other) noexcept + : PyCelValue(std::move(other)) { + FreeThreadingLockGuard lock(other.mutex_); + key_ = std::move(other.key_); + element_value_ = std::move(other.element_value_); + resolved_ = other.resolved_; } -void PyCelMapItemAccessor::ResolveElement() { +void PyCelMapItemAccessor::ResolveElementLocked() { if (resolved_) { return; } @@ -220,44 +273,49 @@ void PyCelMapItemAccessor::ResolveElement() { } PyCelType PyCelMapItemAccessor::Type() { - auto gil_state = PyGILState_Ensure(); - ResolveElement(); - PyGILState_Release(gil_state); + FreeThreadingLockGuard lock(mutex_); + ResolveElementLocked(); return PyCelType::ForCelValue(element_value_); } PyObject* PyCelMapItemAccessor::Value() { ABSL_CHECK(PyGILState_Check()); + FreeThreadingLockGuard lock(mutex_); if (object_) { return object_; } - ResolveElement(); + ResolveElementLocked(); object_ = CelValueToPyObject(element_value_, env_, arena_, /*plain_value=*/false); + if (object_ == nullptr && !PyErr_Occurred()) { + PyErr_SetString(PyExc_AssertionError, "Cannot create object"); + } return object_; } PyObject* PyCelMapItemAccessor::PlainValue() { ABSL_CHECK(PyGILState_Check()); + FreeThreadingLockGuard lock(mutex_); if (plain_object_) { return plain_object_; } - ResolveElement(); + ResolveElementLocked(); plain_object_ = CelValueToPyObject(element_value_, env_, arena_, /*plain_value=*/true); + if (plain_object_ == nullptr && !PyErr_Occurred()) { + PyErr_SetString(PyExc_AssertionError, "Cannot create object"); + } return plain_object_; } std::string PyCelMapItemAccessor::ToString() { - auto gil_state = PyGILState_Ensure(); - ResolveElement(); - std::string result = element_value_.DebugString(); - PyGILState_Release(gil_state); - return result; + FreeThreadingLockGuard lock(mutex_); + ResolveElementLocked(); + return element_value_.DebugString(); } // This should be called with the GIL held. @@ -267,6 +325,7 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, bool plain_value) { switch (cel_value.kind()) { case cel::ValueKind::kNull: { + Py_INCREF(Py_None); return Py_None; } case cel::ValueKind::kBool: { diff --git a/cel_expr_python/py_cel_value.h b/cel_expr_python/py_cel_value.h index c79a680..5dae8e4 100644 --- a/cel_expr_python/py_cel_value.h +++ b/cel_expr_python/py_cel_value.h @@ -21,17 +21,17 @@ #include #include +#include "absl/base/thread_annotations.h" #include "absl/functional/function_ref.h" #include "absl/status/statusor.h" #include "common/value.h" +#include "cel_expr_python/free_threading_mutex.h" #include "cel_expr_python/py_cel_type.h" #include "google/protobuf/arena.h" #include namespace cel_python { -namespace py = ::pybind11; - class PyCelArena; class PyCelEnvInternal; class PyMessageFactory; @@ -47,13 +47,13 @@ class PyCelValue { PyCelValue(cel::Value& cel_value, std::shared_ptr arena, std::shared_ptr env); - // Move constructor and assignment. - PyCelValue(PyCelValue&& other) noexcept = default; - PyCelValue& operator=(PyCelValue&& other) noexcept = default; + // Move constructor. + PyCelValue(PyCelValue&& other) noexcept; - // Disallow copying. + // Disallow copying and move assignment. PyCelValue(const PyCelValue&) = delete; PyCelValue& operator=(const PyCelValue&) = delete; + PyCelValue& operator=(PyCelValue&&) = delete; virtual ~PyCelValue(); @@ -67,9 +67,10 @@ class PyCelValue { PyMessageFactory* py_message_factory); protected: + mutable FreeThreadingMutex mutex_; cel::Value cel_value_; - PyObject* object_; - PyObject* plain_object_; + PyObject* object_ ABSL_GUARDED_BY(mutex_); + PyObject* plain_object_ ABSL_GUARDED_BY(mutex_); std::shared_ptr arena_; std::shared_ptr env_; }; @@ -83,22 +84,21 @@ class PyCelListItemAccessor : public PyCelValue { : PyCelValue(celValue, std::move(arena), std::move(env)), index_(index) {} // Move constructor. - PyCelListItemAccessor(PyCelListItemAccessor&& other) noexcept = default; + PyCelListItemAccessor(PyCelListItemAccessor&& other) noexcept; ~PyCelListItemAccessor() override = default; - // Extracts the element at the given index from the list and caches the - // result. This is called on demand when the python side accesses the value. - void ResolveElement(); PyCelType Type() override; PyObject* Value() override; PyObject* PlainValue() override; std::string ToString() override; private: + void ResolveElementLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + int index_; - bool resolved_ = false; - cel::Value element_value_; + bool resolved_ ABSL_GUARDED_BY(mutex_) = false; + cel::Value element_value_ ABSL_GUARDED_BY(mutex_); }; // Variant of PyCelValue that is used to access a specific value from a map. @@ -111,20 +111,21 @@ class PyCelMapItemAccessor : public PyCelValue { key_(std::move(key)) {} // Move constructor. - PyCelMapItemAccessor(PyCelMapItemAccessor&& other) noexcept = default; + PyCelMapItemAccessor(PyCelMapItemAccessor&& other) noexcept; ~PyCelMapItemAccessor() override = default; - void ResolveElement(); PyCelType Type() override; PyObject* Value() override; PyObject* PlainValue() override; std::string ToString() override; private: + void ResolveElementLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + cel::Value key_; - cel::Value element_value_; - bool resolved_ = false; + cel::Value element_value_ ABSL_GUARDED_BY(mutex_); + bool resolved_ ABSL_GUARDED_BY(mutex_) = false; }; PyObject* CelValueToPyObject(const cel::Value& cel_value, diff --git a/cel_expr_python/py_descriptor_database.cc b/cel_expr_python/py_descriptor_database.cc index f6e54ef..bbbb29f 100644 --- a/cel_expr_python/py_descriptor_database.cc +++ b/cel_expr_python/py_descriptor_database.cc @@ -39,8 +39,15 @@ PyDescriptorDatabase::PyDescriptorDatabase(PyObject* py_descriptor_pool) } PyDescriptorDatabase::~PyDescriptorDatabase() { - py::gil_scoped_acquire acquire; - Py_XDECREF(py_descriptor_pool_); + if (py_descriptor_pool_ == nullptr) { + return; + } + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + Py_XDECREF(py_descriptor_pool_); + } else { + Py_XDECREF(py_descriptor_pool_); + } } // Find a file by file name. Fills in in *output and returns true if found. diff --git a/cel_expr_python/py_descriptor_database.h b/cel_expr_python/py_descriptor_database.h index 5f621f4..3a4a45e 100644 --- a/cel_expr_python/py_descriptor_database.h +++ b/cel_expr_python/py_descriptor_database.h @@ -22,6 +22,7 @@ #include // IWYU pragma: keep - Needed for string_view in OSS #include "google/protobuf/descriptor.pb.h" +#include "absl/strings/string_view.h" #include "google/protobuf/descriptor_database.h" namespace cel_python { diff --git a/cel_expr_python/py_error_status.cc b/cel_expr_python/py_error_status.cc index 4565c0e..06dbd59 100644 --- a/cel_expr_python/py_error_status.cc +++ b/cel_expr_python/py_error_status.cc @@ -103,8 +103,7 @@ static absl::Status& PendingPyError() { return *pending_py_error; } -absl::Status PyErr_toStatus() { - py::gil_scoped_acquire acquire; +static absl::Status PyErr_toStatusInternal() { PyObject* py_error = PyErr_Occurred(); if (!py_error) { absl::Status status = PendingPyError(); @@ -135,8 +134,15 @@ absl::Status PyErr_toStatus() { return status; } +absl::Status PyErr_toStatus() { + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + return PyErr_toStatusInternal(); + } + return PyErr_toStatusInternal(); +} + void PyErr_noteAndClear() { - py::gil_scoped_acquire acquire; if (!PyErr_Occurred()) { return; } diff --git a/cel_expr_python/py_error_status.h b/cel_expr_python/py_error_status.h index 2ecea98..574dbbe 100644 --- a/cel_expr_python/py_error_status.h +++ b/cel_expr_python/py_error_status.h @@ -31,6 +31,16 @@ namespace cel_python { +// Eagerly initializes pybind11 internal data structures (type casters, +// exception translation tables, and instance maps) during module +// initialization. +// +// This guarantees that pybind11's global internals are initialized on the main +// thread at import time, preventing data races or lazy-initialization +// contention when Status/exception conversions occur concurrently across +// multiple threads. +inline void InitPyErrorStatus() { pybind11::detail::get_internals(); } + std::runtime_error StatusToException(const absl::Status& status); void ThrowIfError(const absl::Status& status); diff --git a/cel_expr_python/py_message_factory.cc b/cel_expr_python/py_message_factory.cc index d3475f6..080b33b 100644 --- a/cel_expr_python/py_message_factory.cc +++ b/cel_expr_python/py_message_factory.cc @@ -21,9 +21,13 @@ #include "absl/log/absl_check.h" #include "absl/log/absl_log.h" +#include "cel_expr_python/free_threading_mutex.h" +#include namespace cel_python { +namespace py = pybind11; + PyMessageFactory::PyMessageFactory(PyObject* descriptor_pool) { py_descriptor_pool_ = descriptor_pool; if (py_descriptor_pool_ == nullptr) { @@ -52,17 +56,26 @@ PyMessageFactory::~PyMessageFactory() { return; } - auto gil_state = PyGILState_Ensure(); - Py_XDECREF(py_descriptor_pool_); - Py_XDECREF(py_func_GetMessageClass_); - Py_XDECREF(py_func_MergeFromString_); - for (auto const& [key, py_obj] : message_classes_) { - Py_XDECREF(py_obj); + if (!PyGILState_Check()) { + py::gil_scoped_acquire acquire; + Py_XDECREF(py_descriptor_pool_); + Py_XDECREF(py_func_GetMessageClass_); + Py_XDECREF(py_func_MergeFromString_); + for (auto const& [key, py_obj] : message_classes_) { + Py_XDECREF(py_obj); + } + } else { + Py_XDECREF(py_descriptor_pool_); + Py_XDECREF(py_func_GetMessageClass_); + Py_XDECREF(py_func_MergeFromString_); + for (auto const& [key, py_obj] : message_classes_) { + Py_XDECREF(py_obj); + } } - PyGILState_Release(gil_state); } PyObject* PyMessageFactory::GetMessageClass(const std::string& message_type) { + ABSL_CHECK(PyGILState_Check()); if (py_descriptor_pool_ == nullptr) { PyErr_Format(PyExc_TypeError, "Message type not found: %s, descriptor pool is unavailable.", @@ -70,31 +83,38 @@ PyObject* PyMessageFactory::GetMessageClass(const std::string& message_type) { return nullptr; } - auto it = message_classes_.find(message_type); - if (it != message_classes_.end()) { - return it->second; - } else { - PyObject* descriptor = - PyObject_CallMethod(py_descriptor_pool_, "FindMessageTypeByName", "s", - message_type.c_str()); - if (!descriptor) { - PyErr_Format(PyExc_TypeError, "Message type not found: %s", - message_type.c_str()); - return nullptr; - } - PyObject* message_class = - PyObject_CallFunction(py_func_GetMessageClass_, "O", descriptor); - Py_DECREF(descriptor); - - if (!message_class) { - PyErr_Format(PyExc_TypeError, "Couldn't find message class for type: %s", - message_type.c_str()); - return nullptr; + { + FreeThreadingLockGuard lock(mutex_); + auto it = message_classes_.find(message_type); + if (it != message_classes_.end()) { + return it->second; } + } - message_classes_[message_type] = message_class; - return message_class; + PyObject* descriptor = PyObject_CallMethod( + py_descriptor_pool_, "FindMessageTypeByName", "s", message_type.c_str()); + if (!descriptor) { + PyErr_Format(PyExc_TypeError, "Message type not found: %s", + message_type.c_str()); + return nullptr; + } + PyObject* message_class = + PyObject_CallFunction(py_func_GetMessageClass_, "O", descriptor); + Py_DECREF(descriptor); + + if (!message_class) { + PyErr_Format(PyExc_TypeError, "Couldn't find message class for type: %s", + message_type.c_str()); + return nullptr; + } + + FreeThreadingLockGuard lock(mutex_); + auto [it, inserted] = message_classes_.emplace(message_type, message_class); + if (!inserted) { + Py_DECREF(message_class); + return it->second; } + return message_class; } PyObject* PyMessageFactory::FromString(const std::string& message_type, diff --git a/cel_expr_python/py_message_factory.h b/cel_expr_python/py_message_factory.h index 4811ed3..bb1991d 100644 --- a/cel_expr_python/py_message_factory.h +++ b/cel_expr_python/py_message_factory.h @@ -20,7 +20,9 @@ #include +#include "absl/base/thread_annotations.h" #include "absl/container/flat_hash_map.h" +#include "cel_expr_python/free_threading_mutex.h" namespace cel_python { @@ -37,7 +39,9 @@ class PyMessageFactory { PyObject* py_descriptor_pool_; PyObject* py_func_GetMessageClass_; // NOLINT - Python function name. PyObject* py_func_MergeFromString_; // NOLINT - Python function name. - absl::flat_hash_map message_classes_; + mutable FreeThreadingMutex mutex_; + absl::flat_hash_map message_classes_ + ABSL_GUARDED_BY(mutex_); }; } // namespace cel_python diff --git a/test_freethreaded.sh b/test_freethreaded.sh new file mode 100755 index 0000000..a5c9f82 --- /dev/null +++ b/test_freethreaded.sh @@ -0,0 +1,64 @@ +#!/bin/bash +# +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Runs CEL Python tests in free-threaded mode (PEP 703 / Python 3.13t). +# Supports both Blaze (Google internal) and Bazel (open-source) toolchains. + +set -euo pipefail + +# Detect whether we're using Blaze or Bazel. +if [ -n "${BLAZE_BIN:-}" ]; then + BUILD_TOOL="${BLAZE_BIN}" + IS_BLAZE=true +elif [ -n "${BAZEL_BIN:-}" ]; then + BUILD_TOOL="${BAZEL_BIN}" + IS_BLAZE=false +elif command -v blaze >/dev/null 2>&1 && [ -d "third_party/cel/python" ]; then + BUILD_TOOL="blaze" + IS_BLAZE=true +elif command -v bazel >/dev/null 2>&1; then + BUILD_TOOL="bazel" + IS_BLAZE=false +elif command -v bazelisk >/dev/null 2>&1; then + BUILD_TOOL="bazelisk" + IS_BLAZE=false +elif command -v blaze >/dev/null 2>&1; then + BUILD_TOOL="blaze" + IS_BLAZE=true +else + echo "Error: Neither blaze nor bazel found in PATH." >&2 + exit 1 +fi + +if [ "${IS_BLAZE}" = true ]; then + FREETHREADED_FLAG="--//third_party/bazel_rules/rules_python/python/config_settings:py_freethreaded=yes" + DEFAULT_TARGETS=("//third_party/cel/python/...") +else + FREETHREADED_FLAG="--@rules_python//python/config_settings:py_freethreaded=yes" + DEFAULT_TARGETS=("//...") +fi + +if [ "$#" -eq 0 ]; then + exec "${BUILD_TOOL}" test \ + "${FREETHREADED_FLAG}" \ + --test_env=PYTHON_GIL=0 \ + "${DEFAULT_TARGETS[@]}" +else + exec "${BUILD_TOOL}" test \ + "${FREETHREADED_FLAG}" \ + --test_env=PYTHON_GIL=0 \ + "$@" +fi